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 06:07 AM, Wu, Feng wrote:
> 
> 
>> -----Original Message-----
>> From: Wu, Feng
>> Sent: Thursday, September 17, 2015 2:16 PM
>> To: George Dunlap; Jan Beulich
>> Cc: Tian, Kevin; Keir Fraser; Andrew Cooper; Dario Faggioli;
>> xen-devel@lists.xen.org; Wu, Feng
>> Subject: RE: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for
>> VT-d posted interrupts
>>
>>
>>
>>> -----Original Message-----
>>> From: dunlapg@gmail.com [mailto:dunlapg@gmail.com] On Behalf Of
>> George
>>> Dunlap
>>> Sent: Thursday, September 17, 2015 12:57 AM
>>> To: Jan Beulich
>>> Cc: Wu, Feng; Tian, Kevin; Keir Fraser; Andrew Cooper; Dario Faggioli;
>>> xen-devel@lists.xen.org
>>> Subject: Re: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for
>>> VT-d posted interrupts
>>>
>>> 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.
>>>
>>> But before talking about how to make things more clear, one side
>>> question -- do we need to actually call pi_ctxt_switch_to() in
>>> __context_switch()?
>>>
>>> The only other place __context_switch() is called is
>>> from__sync_local_execstate().  But the only reason that needs to be
>>> called is because sometimes we *don't* call __context_switch(), and so
>>> there are things on the cpu that aren't copied into the vcpu struct.
>>
>> Thanks for the comments!
>>
>> From my understanding, __sync_local_execstate() can only get called
>> in the following two cases:
>> #1) this_cpu(curr_vcpu) == current, in this case, __context_switch() is
>> not called.
>> #2) this_cpu(curr_vcpu) != current, and current == idle_vcpu, that means
>> we just switched from a non-idle vCPU to idle vCPU, so here we need to
>> call __context_switch() to copy things to the original vcpu struct.
>>
>> Please correct me if the above understanding is wrong or incomplete?
> 
> Hi George / Dario,
> 
> Could you please confirm the above understanding is correct? (In fact, it is
> Related to lazy context switch, right?) if so I can continue with the
> pi_context_switch() way George suggested.

Yes, that's the general idea.  Normally, you can access the registers of
a non-running vcpu from the vcpu struct.  But if we've done a lazy
context switch, that's not true -- so to access those registers properly
we need to go through and do the full context switch *on that pcpu*.

Since we need to do the full context switch for PI every time, there
should never be any "local" state which needs to be synced.

I think at this point you should probably just give it a try. :-)

 -George