From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wu, Feng" Subject: Re: [RFC v2 13/15] Update Posted-Interrupts Descriptor during vCPU scheduling Date: Wed, 17 Jun 2015 06:52:47 +0000 Message-ID: References: <1431076038-7497-1-git-send-email-feng.wu@intel.com> <1431076038-7497-14-git-send-email-feng.wu@intel.com> <5577F8CD0200007800082E0E@mail.emea.novell.com> <557FF20B0200007800085441@mail.emea.novell.com> <557FFA8102000078000854D4@mail.emea.novell.com> <5580077602000078000855D6@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5580077602000078000855D6@mail.emea.novell.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: "Tian, Kevin" , "keir@xen.org" , "george.dunlap@eu.citrix.com" , "andrew.cooper3@citrix.com" , "xen-devel@lists.xen.org" , "Zhang, Yang Z" , "Wu, Feng" List-Id: xen-devel@lists.xenproject.org > -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Tuesday, June 16, 2015 5:25 PM > To: Wu, Feng > Cc: andrew.cooper3@citrix.com; george.dunlap@eu.citrix.com; Tian, Kevin; > Zhang, Yang Z; xen-devel@lists.xen.org; keir@xen.org > Subject: RE: [RFC v2 13/15] Update Posted-Interrupts Descriptor during vCPU > scheduling > > >>> On 16.06.15 at 10:56, wrote: > > > > >> -----Original Message----- > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: Tuesday, June 16, 2015 4:29 PM > >> To: Wu, Feng > >> Cc: andrew.cooper3@citrix.com; george.dunlap@eu.citrix.com; Tian, Kevin; > >> Zhang, Yang Z; xen-devel@lists.xen.org; keir@xen.org > >> Subject: RE: [RFC v2 13/15] Update Posted-Interrupts Descriptor during > vCPU > >> scheduling > >> > >> >>> On 16.06.15 at 10:13, wrote: > >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> >> Sent: Tuesday, June 16, 2015 3:53 PM > >> >> >>> On 16.06.15 at 02:17, wrote: > >> >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> >> >> Sent: Wednesday, June 10, 2015 2:44 PM > >> >> >> >>> On 08.05.15 at 11:07, 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