From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: Fwd: [v3 14/15] Update Posted-Interrupts Descriptor during vCPU scheduling Date: Thu, 9 Jul 2015 13:53:11 +0100 Message-ID: <559E6EB7.3050609@eu.citrix.com> References: <1435123109-10481-15-git-send-email-feng.wu@intel.com> <55918214.4030102@citrix.com> <1435633087.25170.274.camel@citrix.com> <1435825253.25170.406.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Wu, Feng" Cc: "Tian, Kevin" , "keir@xen.org" , "andrew.cooper3@citrix.com" , Dario Faggioli , xen-devel , "jbeulich@suse.com" , "Zhang, Yang Z" List-Id: xen-devel@lists.xenproject.org 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 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