Re: [Xen-devel] Fwd: [v3 14/15] Update Posted-Interrupts Descriptor during vCPU scheduling

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

> -----Original Message-----
> From: George Dunlap [mailto:george.dunlap@xxxxxxxxxxxxx]
> Sent: Thursday, July 09, 2015 8:53 PM
> To: Wu, Feng
> Cc: Dario Faggioli; Tian, Kevin; keir@xxxxxxx; andrew.cooper3@xxxxxxxxxx;
> xen-devel; jbeulich@xxxxxxxx; Zhang, Yang Z
> Subject: Re: [Xen-devel] Fwd: [v3 14/15] Update Posted-Interrupts Descriptor
> during vCPU scheduling
> 
> On 07/09/2015 12:38 PM, Wu, Feng wrote:
> >
> >
> >> -----Original Message-----
> >> From: dunlapg@xxxxxxxxx [mailto:dunlapg@xxxxxxxxx] On Behalf Of
> George
> >> Dunlap
> >> Sent: Thursday, July 09, 2015 7:20 PM
> >> To: Wu, Feng
> >> Cc: Dario Faggioli; Tian, Kevin; keir@xxxxxxx; andrew.cooper3@xxxxxxxxxx;
> >> xen-devel; jbeulich@xxxxxxxx; Zhang, Yang Z
> >> Subject: Re: [Xen-devel] Fwd: [v3 14/15] Update Posted-Interrupts
> Descriptor
> >> during vCPU scheduling
> >>
> >> On Thu, Jul 9, 2015 at 4:09 AM, Wu, Feng <feng.wu@xxxxxxxxx> wrote:
> >>>> That does not necessarily means "we need to do something" in
> >>>> vcpu_runstate_change(). Actually, that's exactly what I'm asking: can
> >>>> you check whether this thing that you need doing can be done somewhere
> >>>> else than in vcpu_runstaete_change() ?
> >>>
> >>> Why do you think vcpu_runstaete_change() is not the right place to do
> this?
> >>
> >> Because what the vcpu_runstate_change() function does at the moment is
> >> *update the vcpu runstate variable*.  It doesn't actually change the
> >> runstate -- the runstate is changed in the various bits of code that
> >> call it; and it's not designed to be a generic place to put hooks on
> >> the runstate changing.
> >>
> >> I haven't done a thorough review of this yet, but at least looking
> >> through this patch, and skimming the titles, I don't see anywhere you
> >> handle migration -- what happens if a vcpu that's blocked / offline /
> >> runnable migrates from one cpu to another?  Is the information
> >> updated?
> >
> > Thanks for your review!
> 
> And I'd like to say -- sorry that I didn't notice this issue sooner; I
> know you've had your series posted for quite a while, but I didn't
> realize until last week that it actually involved the scheduler.  It's
> really my fault for not paying closer attention -- you did CC me in v2
> back in June.
> 
> > The migration is handled in arch_pi_desc_update() which is called
> > by vcpu_runstate_change().
> 
> Well as far as I can tell from looking at the code,
> vcpu_runstate_change() will not be called when migrating a vcpu which is
> already blocked.
> 
> Consider the following scenario:
> - v1 blocks on pcpu 0.
>  - vcpu_runstate_change() will do everything necessary for v1 on p0.
> - The scheduler does load balancing and moves v1 to p1, calling
> vcpu_migrate().  Because the vcpu is still blocked,
> vcpu_runstate_change() is not called.
> - A device interrupt is generated.
> 
> What happens to the interrupt?  Does everything still work properly, or
> will the device wake-up interrupt go to the wrong pcpu (p0 rather than p1)?

I think it works correctly. Before blocking, we save the v->processor, and
save the vcpu on this per-cpu list, even when the vCPU is migrated to
another pCPU, the wakeup notification event will go to the original one
(p0 in this case), this is what I want, in the list of p0, we can find and
unblock the blocked vCPU, this is the point.

Thanks,
Feng

> 
> >  or to add a set of architectural hooks (similar to
> >> the SCHED_OP() hooks) in the various places you need them.
> >
> > I don't have a picture of this method, but from your comments, seems
> > we need to put the logic to many different places, and must be very
> > careful so as to not miss some places. I think the above method
> > is more clear and straightforward, since we have a central place to
> > handle all the cases. Anyway, if you prefer to this one, it would be
> > highly appreciated if you can give a more detailed solution! Thank you!
> 
> Well you can check to make sure you've caught at least all the places
> you had before by searching for vcpu_runstate_change(). :-)
> 
> Using the callback method also can help prompt you to think about other
> times you may need to do something.  For instance, you might still
> consider searching for SCHED_OP() everywhere in schedule.c and seeing if
> that's a place you need to do something (similar to the migration thing
> above).
> 
> Anyway, the most detailed thing I can say at this time is to look at
> SCHED_OP() and see if doing  something like that, but for architectural
> callbacks, makes sense.
> 
> I'll come back and take a closer look a bit later.
> 
>  -George
> 

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