Re: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted interrupts

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.09.15 at 18:56, <George.Dunlap@xxxxxxxxxxxxx> wrote:
> On Mon, Sep 7, 2015 at 1:54 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> On 25.08.15 at 03:57, <feng.wu@xxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -1573,6 +1573,22 @@ static void __context_switch(void)
>>>      per_cpu(curr_vcpu, cpu) = n;
>>>  }
>>>
>>> +static inline void pi_ctxt_switch_from(struct vcpu *prev)
>>> +{
>>> +    /*
>>> +     * When switching from non-idle to idle, we only do a lazy context 
>>> switch.
>>> +     * However, in order for posted interrupt (if available and enabled) to
>>> +     * work properly, we at least need to update the descriptors.
>>> +     */
>>> +    if ( prev->arch.pi_ctxt_switch_from && !is_idle_vcpu(prev) )
>>> +        prev->arch.pi_ctxt_switch_from(prev);
>>> +}
>>> +
>>> +static inline void pi_ctxt_switch_to(struct vcpu *next)
>>> +{
>>> +    if ( next->arch.pi_ctxt_switch_to && !is_idle_vcpu(next) )
>>> +        next->arch.pi_ctxt_switch_to(next);
>>> +}
>>>
>>>  void context_switch(struct vcpu *prev, struct vcpu *next)
>>>  {
>>> @@ -1605,9 +1621,12 @@ void context_switch(struct vcpu *prev, struct vcpu 
>>> *next)
>>>
>>>      set_current(next);
>>>
>>> +    pi_ctxt_switch_from(prev);
>>> +
>>>      if ( (per_cpu(curr_vcpu, cpu) == next) ||
>>>           (is_idle_domain(nextd) && cpu_online(cpu)) )
>>>      {
>>> +        pi_ctxt_switch_to(next);
>>>          local_irq_enable();
>>
>> This placement, if really intended that way, needs explanation (in a
>> comment) and perhaps even renaming of the involved symbols, as
>> looking at it from a general perspective it seems wrong (with
>> pi_ctxt_switch_to() excluding idle vCPU-s it effectively means you
>> want this only when switching back to what got switched out lazily
>> before, i.e. this would be not something to take place on an arbitrary
>> context switch). As to possible alternative names - maybe make the
>> hooks ctxt_switch_prepare() and ctxt_switch_cancel()?
> 
> Why on earth is this more clear than what he had before?
> 
> In the first call, he's not "preparing" anything -- he's actually
> switching the PI context out for prev.  And in the second call, he's
> not "cancelling" anything -- he's actually switching the PI context in
> for next.  The names you suggest are actively confusing, not helpful.

While I think later discussion on this thread moved in a good direction,
I still think I should reply here (even if late): To me, the use of
pi_ctxt_switch_to() in the patch fragment still seen above is very
much the cancellation of the immediately preceding pi_ctxt_switch_from(),
as it's the "we don't want to do anything else" path that it gets put
into.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel