Re: [Xen-devel] [RFC v2 13/15] Update Posted-Interrupts Descriptor during vCPU scheduling

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Tuesday, June 16, 2015 5:25 PM
> To: Wu, Feng
> Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian, Kevin;
> Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: RE: [RFC v2 13/15] Update Posted-Interrupts Descriptor during vCPU
> scheduling
> 
> >>> On 16.06.15 at 10:56, <feng.wu@xxxxxxxxx> wrote:
> 
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Tuesday, June 16, 2015 4:29 PM
> >> To: Wu, Feng
> >> Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian, Kevin;
> >> Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx
> >> Subject: RE: [RFC v2 13/15] Update Posted-Interrupts Descriptor during
> vCPU
> >> scheduling
> >>
> >> >>> On 16.06.15 at 10:13, <feng.wu@xxxxxxxxx> wrote:
> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >> Sent: Tuesday, June 16, 2015 3:53 PM
> >> >> >>> On 16.06.15 at 02:17, <feng.wu@xxxxxxxxx> wrote:
> >> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >> >> Sent: Wednesday, June 10, 2015 2:44 PM
> >> >> >> >>> On 08.05.15 at 11:07, <feng.wu@xxxxxxxxx> wrote:
> >> >> >> > +
> >> >> >> > +           /*
> >> >> >> > +            * Delete the vCPU from the related block list
> >> >> >> > +            * if we are resuming from blocked state
> >> >> >> > +            */
> >> >> >> > +           spin_lock_irqsave(&per_cpu(blocked_vcpu_lock,
> >> >> >> > +                             v->pre_pcpu), flags);
> >> >> >> > +           list_del(&v->blocked_vcpu_list);
> >> >> >> > +           spin_unlock_irqrestore(&per_cpu(blocked_vcpu_lock,
> >> >> >> > +                                  v->pre_pcpu), flags);
> >> >> >> > +        }
> >> >> >> > +        break;
> >> >> >> > +
> >> >> >> > +    case RUNSTATE_blocked:
> >> >> >> > +        /*
> >> >> >> > +         * The vCPU is blocked on the block list.
> >> >> >> > +         * Add the blocked vCPU on the list of the
> >> >> >> > +         * vcpu->pre_pcpu, which is the destination
> >> >> >> > +         * of the wake-up notification event.
> >> >> >> > +         */
> >> >> >> > +        v->pre_pcpu = v->processor;
> >> >> >>
> >> >> >> Is latching this upon runstate change really enough? I.e. what about
> >> >> >> the v->processor changes that sched_move_domain() or individual
> >> >> >> schedulers do? Or if it really just matters on which CPU's blocked 
> >> >> >> list
> >> >> >> the vCPU is (while its ->processor changing subsequently doesn't
> >> >> >> matter) I'd like to see the field named more after its purpose (e.g.
> >> >> >> pi_block_cpu; list and lock should btw also have a connection to PI
> >> >> >> in their names).
> >> >> >
> >> >> > Yes, It doesn't matter if vCPU changes. The key point is that we put
> >> >> > the vCPU on a pCPU list and we change the NDST field to this pCPU,
> >> >> > then the wakeup notification event will get there. You are right, I
> >> >> > need to rename them to reflect the real purpose of it.
> >> >> >
> >> >> >>
> >> >> >> In the end, if the placement on a list followed v->processor, you
> >> >> >> would likely get away without the extra new field. Are there
> >> >> >> synchronization constraints speaking against such a model?
> >> >> >
> >> >> > I don't understand this quit well. Do you mean using 'v->processor'
> >> >> > as the pCPU list for the blocked vCPUs? Then what about 'v->processor'
> >> >> > changes, seems we cannot handle this case.
> >> >>
> >> >> That was the question - does anything speak against such a model?
> >> >
> >> > Do you mean still using v->processor as the pCPU to store the blocked
> vCPU?
> >>
> >> Not "to store", but "to indicate", but yes, the question still is regarding
> >> the need for the new field. I'm fine with adding the field if there is a
> >> proper explanation for it being needed; I'm not going to agree to add
> >> new fields when existing ones can serve the purpose.
> >
> > Fair enough. The basic reason for adding this new field is 'v->processor' 
> > can
> > be changed behind, so we lost control of it. This is straightforward way I
> > can think of right now.
> 
> This is the obvious part you state. What needs to be explained is
> why it would be impossible (or a t least a bad idea) to move the
> vCPU between blocked lists when its v->processor changes.

The key point here is that we put the blocked vCPU in the pCPU list and
update the 'NDST' field in posted-interrupt descriptor for the wakeup
notification event. Here are some reasons why using 'v->processor' as
the pCPU to store the blocked vCPUs is not a good idea:

There may be many places where 'v->processor' gets changed, we need
to find all of them and in each place, see if the vCPU is currently blocked, if
it is, remove it from the old pCPU list and insert to the new pCPU list (need
spinlock operations), we need also update the 'NDST' filed for the wakeup
notification event. Besides that, the individual scheduler can change
'v->processor', which means we may need to add some code specific to PI
purpose to the scheduler code, I don't think this is a good way. On the other
hand, if you use the current solution, we can get the following benefit:
- The logic is clear and simple.
- We only need to update the posted-interrupt descriptor before the vCPU
is blocked, i.e. once the 'NDST' filed is updated before blocking the vCPU,
we don't need to change it even 'v->processor' is changed.
- We have a central place to operate the vCPU list, we don't need to care
missing some places.

Thanks,
Feng

> 
> Jan


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