[PATCH] xen: domain_update_node_affinity: Correct the ASSERT

[PATCH] xen: domain_update_node_affinity: Correct the ASSERT

[Julien Grall]

The commit "move domain to cpupool0 before destroying it" make Xen crashes
when a domain is destroyed with d->vcpus allocated but no VCPU initialized.

Assertion '!cpumask_empty(dom_cpumask)' failed at domain.c:452
Xen call trace:
    [<00207bd8>] domain_update_node_affinity+0x10c/0x238 (PC)
    [<00000004>] 00000004 (LR)
    [<00226870>] sched_move_domain+0x3cc/0x42c
    [<0020925c>] domain_kill+0xc8/0x178
    [<00206a0c>] do_domctl+0xaac/0x15e4
    [<002529c0>] do_trap_hypervisor+0xc5c/0xf94
    [<002559f0>] return_from_trap+0/0x4

Fix the ASSERT to check if d->vcpu is allocated and VCPU 0 is initialized.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>

---
    This patch should be backported to Xen 4.4
---
 xen/common/domain.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index d7a84cf..188b769 100644
--- 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);
 
-- 
1.7.10.4

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

[Jan Beulich]

>>> 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?

Jan

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

[Julien Grall]

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

Regards,

-- 
Julien Grall

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

[Jan Beulich]

>>> 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?

Jan

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

[Dario Faggioli]

[-- 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

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

[Andrew Cooper]

On 25/07/14 16:30, Julien Grall wrote:
> The commit "move domain to cpupool0 before destroying it" make Xen crashes
> when a domain is destroyed with d->vcpus allocated but no VCPU initialized.
>
> Assertion '!cpumask_empty(dom_cpumask)' failed at domain.c:452
> Xen call trace:
>     [<00207bd8>] domain_update_node_affinity+0x10c/0x238 (PC)
>     [<00000004>] 00000004 (LR)
>     [<00226870>] sched_move_domain+0x3cc/0x42c
>     [<0020925c>] domain_kill+0xc8/0x178
>     [<00206a0c>] do_domctl+0xaac/0x15e4
>     [<002529c0>] do_trap_hypervisor+0xc5c/0xf94
>     [<002559f0>] return_from_trap+0/0x4
>
> Fix the ASSERT to check if d->vcpu is allocated and VCPU 0 is initialized.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
>
> ---
>     This patch should be backported to Xen 4.4
> ---
>  xen/common/domain.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index d7a84cf..188b769 100644
> --- 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));

You don't actually care for the vcpu pointers themselves.  You only care
whether the domain has vcpus.

d->max_vcpus == 0 is a more appropriate check.

~Andrew

>          /* And compute the intersection between hard, online and soft */
>          cpumask_and(dom_cpumask_soft, dom_cpumask_soft, dom_cpumask);
>  

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

[Julien Grall]

On 07/25/2014 04:44 PM, Andrew Cooper wrote:
> On 25/07/14 16:30, Julien Grall wrote:
>> The commit "move domain to cpupool0 before destroying it" make Xen crashes
>> when a domain is destroyed with d->vcpus allocated but no VCPU initialized.
>>
>> Assertion '!cpumask_empty(dom_cpumask)' failed at domain.c:452
>> Xen call trace:
>>     [<00207bd8>] domain_update_node_affinity+0x10c/0x238 (PC)
>>     [<00000004>] 00000004 (LR)
>>     [<00226870>] sched_move_domain+0x3cc/0x42c
>>     [<0020925c>] domain_kill+0xc8/0x178
>>     [<00206a0c>] do_domctl+0xaac/0x15e4
>>     [<002529c0>] do_trap_hypervisor+0xc5c/0xf94
>>     [<002559f0>] return_from_trap+0/0x4
>>
>> Fix the ASSERT to check if d->vcpu is allocated and VCPU 0 is initialized.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Cc: George Dunlap <george.dunlap@citrix.com>
>> Cc: Dario Faggioli <dario.faggioli@citrix.com>
>> Cc: Juergen Gross <jgross@suse.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Tim Deegan <tim@xen.org>
>>
>> ---
>>     This patch should be backported to Xen 4.4
>> ---
>>  xen/common/domain.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index d7a84cf..188b769 100644
>> --- 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));
> 
> You don't actually care for the vcpu pointers themselves.  You only care
> whether the domain has vcpus.
> 
> d->max_vcpus == 0 is a more appropriate check.

Actually no... max_vcpus has been set before Xen is allocating
alloc_vcpu (see DOMCTL_max_vpus in common common/domctl.c).

So if Xen fails to allocate VCPU0, the hypervisor will still crash when
the domain is killed.

Regards,

-- 
Julien Grall

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

[Andrew Cooper]

On 25/07/14 16:48, Julien Grall wrote:
> On 07/25/2014 04:44 PM, Andrew Cooper wrote:
>> On 25/07/14 16:30, Julien Grall wrote:
>>> The commit "move domain to cpupool0 before destroying it" make Xen crashes
>>> when a domain is destroyed with d->vcpus allocated but no VCPU initialized.
>>>
>>> Assertion '!cpumask_empty(dom_cpumask)' failed at domain.c:452
>>> Xen call trace:
>>>     [<00207bd8>] domain_update_node_affinity+0x10c/0x238 (PC)
>>>     [<00000004>] 00000004 (LR)
>>>     [<00226870>] sched_move_domain+0x3cc/0x42c
>>>     [<0020925c>] domain_kill+0xc8/0x178
>>>     [<00206a0c>] do_domctl+0xaac/0x15e4
>>>     [<002529c0>] do_trap_hypervisor+0xc5c/0xf94
>>>     [<002559f0>] return_from_trap+0/0x4
>>>
>>> Fix the ASSERT to check if d->vcpu is allocated and VCPU 0 is initialized.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>> Cc: George Dunlap <george.dunlap@citrix.com>
>>> Cc: Dario Faggioli <dario.faggioli@citrix.com>
>>> Cc: Juergen Gross <jgross@suse.com>
>>> Cc: Ian Campbell <ian.campbell@citrix.com>
>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Keir Fraser <keir@xen.org>
>>> Cc: Tim Deegan <tim@xen.org>
>>>
>>> ---
>>>     This patch should be backported to Xen 4.4
>>> ---
>>>  xen/common/domain.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>> index d7a84cf..188b769 100644
>>> --- 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));
>> You don't actually care for the vcpu pointers themselves.  You only care
>> whether the domain has vcpus.
>>
>> d->max_vcpus == 0 is a more appropriate check.
> Actually no... max_vcpus has been set before Xen is allocating
> alloc_vcpu (see DOMCTL_max_vpus in common common/domctl.c).
>
> So if Xen fails to allocate VCPU0, the hypervisor will still crash when
> the domain is killed.
>
> Regards,
>

Ah yes - I had mis-remembered the problem at hand.  Sorry for the noise.

~Andrew

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

[Dario Faggioli]

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

On ven, 2014-07-25 at 16:30 +0100, Julien Grall wrote:
> The commit "move domain to cpupool0 before destroying it" make Xen crashes
> when a domain is destroyed with d->vcpus allocated but no VCPU initialized.
> 
The title of the commit is certainly useful. Perhaps the beginning of
the hash would have been too.

> Assertion '!cpumask_empty(dom_cpumask)' failed at domain.c:452
> Xen call trace:
>     [<00207bd8>] domain_update_node_affinity+0x10c/0x238 (PC)
>     [<00000004>] 00000004 (LR)
>     [<00226870>] sched_move_domain+0x3cc/0x42c
>     [<0020925c>] domain_kill+0xc8/0x178
>     [<00206a0c>] do_domctl+0xaac/0x15e4
>     [<002529c0>] do_trap_hypervisor+0xc5c/0xf94
>     [<002559f0>] return_from_trap+0/0x4
> 
> Fix the ASSERT to check if d->vcpu is allocated and VCPU 0 is initialized.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> 
In any case:

Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

-- 
<<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

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

[Julien Grall]

Hi Dario,

On 07/28/2014 06:25 PM, Dario Faggioli wrote:
> On ven, 2014-07-25 at 16:30 +0100, Julien Grall wrote:
>> The commit "move domain to cpupool0 before destroying it" make Xen crashes
>> when a domain is destroyed with d->vcpus allocated but no VCPU initialized.
>>
> The title of the commit is certainly useful. Perhaps the beginning of
> the hash would have been too.

I didn't add the commit. Because it will be backported to Xen 4.4.

Would it be fine to point to a Xen 4.5 commit in Xen 4.4 branch?

>> Assertion '!cpumask_empty(dom_cpumask)' failed at domain.c:452
>> Xen call trace:
>>     [<00207bd8>] domain_update_node_affinity+0x10c/0x238 (PC)
>>     [<00000004>] 00000004 (LR)
>>     [<00226870>] sched_move_domain+0x3cc/0x42c
>>     [<0020925c>] domain_kill+0xc8/0x178
>>     [<00206a0c>] do_domctl+0xaac/0x15e4
>>     [<002529c0>] do_trap_hypervisor+0xc5c/0xf94
>>     [<002559f0>] return_from_trap+0/0x4
>>
>> Fix the ASSERT to check if d->vcpu is allocated and VCPU 0 is initialized.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Cc: George Dunlap <george.dunlap@citrix.com>
>> Cc: Dario Faggioli <dario.faggioli@citrix.com>
>> Cc: Juergen Gross <jgross@suse.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Tim Deegan <tim@xen.org>
>>
> In any case:
> 
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Thanks!

Regards,

-- 
Julien Grall

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

[Jan Beulich]

>>> On 28.07.14 at 19:31, <julien.grall@linaro.org> wrote:
> On 07/28/2014 06:25 PM, Dario Faggioli wrote:
>> On ven, 2014-07-25 at 16:30 +0100, Julien Grall wrote:
>>> The commit "move domain to cpupool0 before destroying it" make Xen crashes
>>> when a domain is destroyed with d->vcpus allocated but no VCPU initialized.
>>>
>> The title of the commit is certainly useful. Perhaps the beginning of
>> the hash would have been too.
> 
> I didn't add the commit. Because it will be backported to Xen 4.4.
> 
> Would it be fine to point to a Xen 4.5 commit in Xen 4.4 branch?

Yes. Backports are (supposed to be) identifiable (via some kind of
reference to the backported commit), which would implicitly mean
any commit IDs referenced in the description refer to the master
branch. Furthermore, if someone wanted to look up the
referenced commit, having the (beginning of the) hash in hands
makes this a much more immediate action than just the title; I
believe such a lookup would even succeed on any of the stable
branches, as - luckily - we decided to make them branches rather
than standalone trees.

Jan

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

[Julien Grall]

Hi Jan,

On 07/29/2014 07:40 AM, Jan Beulich wrote:
>>>> On 28.07.14 at 19:31, <julien.grall@linaro.org> wrote:
>> On 07/28/2014 06:25 PM, Dario Faggioli wrote:
>>> On ven, 2014-07-25 at 16:30 +0100, Julien Grall wrote:
>>>> The commit "move domain to cpupool0 before destroying it" make Xen crashes
>>>> when a domain is destroyed with d->vcpus allocated but no VCPU initialized.
>>>>
>>> The title of the commit is certainly useful. Perhaps the beginning of
>>> the hash would have been too.
>>
>> I didn't add the commit. Because it will be backported to Xen 4.4.
>>
>> Would it be fine to point to a Xen 4.5 commit in Xen 4.4 branch?
> 
> Yes. Backports are (supposed to be) identifiable (via some kind of
> reference to the backported commit), which would implicitly mean
> any commit IDs referenced in the description refer to the master
> branch. Furthermore, if someone wanted to look up the
> referenced commit, having the (beginning of the) hash in hands
> makes this a much more immediate action than just the title; I
> believe such a lookup would even succeed on any of the stable
> branches, as - luckily - we decided to make them branches rather
> than standalone trees.

Thank you for the explanation. I will send a v2 today with the commit ID
of xen unstable in the message.

Regards,

-- 
Julien Grall

[PATCH] xen: domain_update_node_affinity: Correct the ASSERT

[Julien Grall]

The commit bac6334b5 "move domain to cpupool0 before destroying it" make Xen
crashes when a domain is destroyed with d->vcpus allocated but no VCPU
initialized.

Assertion '!cpumask_empty(dom_cpumask)' failed at domain.c:452
Xen call trace:
    [<00207bd8>] domain_update_node_affinity+0x10c/0x238 (PC)
    [<00000004>] 00000004 (LR)
    [<00226870>] sched_move_domain+0x3cc/0x42c
    [<0020925c>] domain_kill+0xc8/0x178
    [<00206a0c>] do_domctl+0xaac/0x15e4
    [<002529c0>] do_trap_hypervisor+0xc5c/0xf94
    [<002559f0>] return_from_trap+0/0x4

Fix the ASSERT to check if d->vcpu is allocated and VCPU 0 is initialized.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>

---
    This patch should be backported to Xen 4.4

    Changes in v2:
        - Add specify the offended commit ID in the message
---
 xen/common/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index d7a84cf..188b769 100644
--- 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);
 
-- 
1.9.3

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

[Jan Beulich]

>>> On 01.08.14 at 16:52, <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));
>          /* 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.)

Jan

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

[George Dunlap]

On 08/01/2014 04:12 PM, Jan Beulich wrote:
>>>> On 01.08.14 at 16:52, <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));
>>           /* 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.

  -George

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

[Julien Grall]

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@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));
>>>           /* 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

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

[Jan Beulich]

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