Re: [Xen-devel] [PATCH] xen: domain_update_node_affinity: Correct the ASSERT

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.08.14 at 17:04, <julien.grall@xxxxxxxxxx> wrote:
> 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, <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));
>>>>           /* 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?

I would indeed favor that over the altered assertion.

Jan


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