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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.09.15 at 11:28, <george.dunlap@xxxxxxxxxx> wrote:
> On 09/21/2015 09:23 AM, Jan Beulich wrote:
>>>>> 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:
>>>>> @@ -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.

Oh, I'm sorry - you're right.

Jan


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