From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH] xen: avoid updating node affinity twice when removing a CPU from a cpupool Date: Fri, 13 Mar 2015 11:15:07 +0000 Message-ID: <5502C6BB.7090207@eu.citrix.com> References: <20150309164901.11859.95044.stgit@Solace.station> <550058BB.7000102@eu.citrix.com> <1426089874.21405.10.camel@citrix.com> <1426167922.7023.21.camel@citrix.com> <5501B5F80200007800069268@mail.emea.novell.com> <1426175556.7023.41.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1426175556.7023.41.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dario Faggioli , "JBeulich@suse.com" Cc: "JGross@suse.com" , "keir.xen@gmail.com" , "jtweaver@hawaii.edu" , George Dunlap , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 03/12/2015 03:52 PM, Dario Faggioli wrote: > On Thu, 2015-03-12 at 14:51 +0000, Jan Beulich wrote: >>>>> On 12.03.15 at 14:45, wrote: >>> Patch below, and attached. However, I think the correct thing to do >>> would be to just revert 93be8285 "update domU's node-affinity on the >>> cpupool_unassign_cpu() path", wouldn't it? >> >> Indeed - if the presented patch is what we want, it should be >> carried out as a revert. But you'll then want to explain why you >> did what you did there in the first place: >> > Because I thought it was necessary. ISTR I spotted the lack of symmetry > that George is also mentioning, by looking at its _assign_ counterpart, > and did not notice, at that time, that it was actually ok, as the update > happens already, although in schedule.c... > >> It surely wasn't without >> reason, >> > It was for a wrong reason. :-) > >> and hence I'd be afraid the revert would re-introduce >> another problem. That explanation should then probably go in >> as description for the revert. >> > I'm not sure I'm getting 100% of what you mean. Let me try: I'd say something like: ---- Revert c/s 93be8285 At the point this patch calls domain_update_node_affinity(), the vcpu hard affinities have not yet been updated; so calling it at this point can in some circumstances trigger an ASSERT(). domain_update_node_affinity() is already called in cpu_disable_scheduler(), so adding it to cpupool_unassign_cpu() is redundant. Simply reverting the patch is sufficient. --- -George