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: 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@xxxxxxxxxxxxx; Wu, Feng
> Subject: RE: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for
> VT-d posted interrupts
> 
> 
> 
> > -----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?

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.

Thanks,
Feng

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