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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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.

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.

That doesn't apply to the PI state -- for one, nothing is copied from
the processor; and for two, pi_ctxt_switch_from() is called
unconditionally anyway.

Would it make more sense to call pi_context_switch(prev, next) just
after "set_current"?

(Keeping in mind I totally may have missed something...)

 -George

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