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

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: dunlapg@xxxxxxxxx [mailto:dunlapg@xxxxxxxxx] 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@xxxxxxxxxxxxx
> 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@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.

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?

I think calling pi_ctxt_switch_to() in __context_switch() is needed when
we are switching to a non-idle vCPU (we need change the PI state of the
target vCPU), and the call is not needed when switching to idle vCPU.
So if the above understanding is correct, I think you suggestion below
is really good, it makes things clearer.

> 
> 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"?

I think it is a good point.

Thanks,
Feng

> 
> (Keeping in mind I totally may have missed something...)
> 
>  -George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel