Re: [PATCH] xen: domain_update_node_affinity: Correct the ASSERT

From: Dario Faggioli <dario.faggioli@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Juergen Gross <JGross@suse.com>, Keir Fraser <keir@xen.org>,
	ian.campbell@citrix.com, tim@xen.org,
	Julien Grall <julien.grall@linaro.org>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	stefano.stabellini@citrix.com, xen-devel@lists.xenproject.org
Subject: Re: [PATCH] xen: domain_update_node_affinity: Correct the ASSERT
Date: Mon, 28 Jul 2014 19:23:10 +0200	[thread overview]
Message-ID: <1406568190.4038.15.camel@Solace> (raw)
In-Reply-To: <53D29B15020000780002620F@mail.emea.novell.com>

[-- Attachment #1.1: Type: text/plain, Size: 1980 bytes --]

On ven, 2014-07-25 at 16:59 +0100, Jan Beulich wrote:
> >>> On 25.07.14 at 17:44, <julien.grall@linaro.org> wrote:
> > Hi Jan,
> > 
> > On 07/25/2014 04:42 PM, Jan Beulich wrote:
> >>>>> On 25.07.14 at 17:30, <julien.grall@linaro.org> 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));
> >> 
> >> Wouldn't it be better for the function to bail early in that case?
> > 
> > I've no idea. I mostly followed the advice on this thread:
> > 
> > http://www.gossamer-threads.com/lists/xen/devel/340233 
> 
> Yeah, I recall that discussion. But looking at the function, nothing
> useful will be done when the domain has no vCPU yet. Dario?
> 
For sure nothing useful happens if the for_each_vcpu() loop never
executes, because of lack of vcpus.

Functionally wise, bailing or going ahead, but avoiding the ASSERT to
trigger (as this patch does) is exactly the same.

I'm not sure which approach I personally prefer... If calling the
function without any allocated vcpus is a "legitimate" and frequent
enough use case, I'd say let's bail explicitly. If it's a corner case,
then I think this patch is ok.

It looks to me like we're in the latter situation (corner case), so I'm
actually fine with this patch. That's why I'm acking it, but really,
it's a matter of taste.. FWIW, I'd ack a version that bails too.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel