[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen: domain_update_node_affinity: Correct the ASSERT
On ven, 2014-07-25 at 16:59 +0100, Jan Beulich wrote: > >>> On 25.07.14 at 17:44, <julien.grall@xxxxxxxxxx> wrote: > > Hi Jan, > > > > On 07/25/2014 04:42 PM, Jan Beulich wrote: > >>>>> On 25.07.14 at 17:30, <julien.grall@xxxxxxxxxx> 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:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |