From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] xen: domain_update_node_affinity: Correct the ASSERT Date: Thu, 07 Aug 2014 16:04:27 +0100 Message-ID: <53E3957B.7000902@linaro.org> References: <1406904759-3833-1-git-send-email-julien.grall@linaro.org> <53DBCA8B020000780002886D@mail.emea.novell.com> <53DF9DC7.2000207@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XFPF4-0005cG-94 for xen-devel@lists.xenproject.org; Thu, 07 Aug 2014 15:04:34 +0000 Received: by mail-wg0-f48.google.com with SMTP id x13so4286528wgg.31 for ; Thu, 07 Aug 2014 08:04:32 -0700 (PDT) In-Reply-To: <53DF9DC7.2000207@eu.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: George Dunlap , Jan Beulich Cc: Juergen Gross , Keir Fraser , ian.campbell@citrix.com, tim@xen.org, Ian Jackson , George Dunlap , stefano.stabellini@citrix.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On 08/04/2014 03:50 PM, George Dunlap wrote: > On 08/01/2014 04:12 PM, Jan Beulich wrote: >>>>> On 01.08.14 at 16:52, wrote: >>> --- a/xen/common/domain.c >>> +++ b/xen/common/domain.c >>> @@ -449,7 +449,7 @@ void domain_update_node_affinity(struct domain *d) >>> } >>> /* Filter out non-online cpus */ >>> cpumask_and(dom_cpumask, dom_cpumask, online); >>> - ASSERT(!cpumask_empty(dom_cpumask)); >>> + ASSERT( !d->vcpu || !d->vcpu[0] || >>> !cpumask_empty(dom_cpumask)); >>> /* And compute the intersection between hard, online and >>> soft */ >>> cpumask_and(dom_cpumask_soft, dom_cpumask_soft, dom_cpumask); >>> >> >> Actually, with sched_move_domain() having >> >> /* Do we have vcpus already? If not, no need to update >> node-affinity */ >> if ( d->vcpu ) >> domain_update_node_affinity(d); >> >> it should really just be _that_ if() condition to get extended, and the >> ASSERT() left alone altogether. Or, if any other path can be proven >> to possibly reach the function with no vCPU allocated (I just went >> through them and didn't spot any), then it should really be an early >> bail from the function rather than a pointlessly complicated ASSERT() >> expression. (And for the record, your expression has a coding style >> violation anyway in that it begins with a space.) > > I think changing the if() was what Julien started with; but overall I > think that it makes more sense to update the assumption of the code in > question than to require all the callers to be careful not to trip over it. > > Doing an early bail might make sense as well. Ok. So which one should I choose? The early bail out? Regards, -- Julien Grall