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

[George Dunlap <george.dunlap@citrix.com>]

On 09/21/2015 09:23 AM, Jan Beulich wrote:
>>>> On 16.09.15 at 18:56, <George.Dunlap@eu.citrix.com> wrote:
>> On Mon, Sep 7, 2015 at 1:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 25.08.15 at 03:57, <feng.wu@intel.com> 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.

Either we have different understandings about what the code does, or I
don't understand what you're saying here.

The codepath in question will only be called if we're switching *into*
or *out of* the "lazy context swtich" -- i.e., switching from a vcpu to
the idle vcpu, but not saving or restoring state.

For the "switch into" case:
* prev will be a domain vcpu, next will be the idle vcpu
* pi_ctxt_switch_from(prev) will either change NV to 'pi_wake' or set SN
for prev's PI vectors (depending on whether prev is blocked or offline)
* pi_ctxt_switch_to(next) will do nothing, since next is the idle vcpu

For the "switching out" case:
* prev will be the idle vcpu, next will be a domain vcpu
* pi_ctxt_switch_from(prev) will do nothing, since prev is the idle vcpu
* pi_ctxt_switch_to(next) will clear SN and change the vector to
'posted_intr'

So there is no situation in which pi_ctxt_switch_to() "cancels" the
immediately preceding pi_ctxt_switch_from().  Either the preceding
from() did something, in which case to() does nothing; or the preceding
from() did nothing, in which case to() does something.

 -George