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

[George Dunlap <george.dunlap@eu.citrix.com>]

On 07/09/2015 12:38 PM, Wu, Feng wrote:
> 
> 
>> -----Original Message-----
>> From: dunlapg@gmail.com [mailto:dunlapg@gmail.com] On Behalf Of George
>> Dunlap
>> Sent: Thursday, July 09, 2015 7:20 PM
>> To: Wu, Feng
>> Cc: Dario Faggioli; Tian, Kevin; keir@xen.org; andrew.cooper3@citrix.com;
>> xen-devel; jbeulich@suse.com; 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@intel.com> 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)?

>  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