[PATCH] xen: fix for_each_cpu when NR_CPUS=1

[PATCH] xen: fix for_each_cpu when NR_CPUS=1

[Dario Faggioli]

When running an hypervisor build with NR_CPUS=1 for_each_cpu does not
take into account whether the bit of the CPU is set or not in the
provided mask.

This means that whatever we have in the bodies of these loops is always
done once, even if the mask was empty and it should never be done. This
is clearly a bug and was in fact causing an assert to trigger in credit2
code.

Removing the special casing of NR_CPUS == 1 makes things work again.

Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
---
I'm not really sure whether this should be 4.15 material.

It's definitely a bug, IMO. The risk is also pretty low, considering
that no one should really run Xen in this configuration (NR_CPUS=1, I
mean). Which is also the reason why it's probably not really important
that we fix it at this point of the release cycle.
---
 xen/include/xen/cpumask.h |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h
index 256b60b106..e69589fc08 100644
--- a/xen/include/xen/cpumask.h
+++ b/xen/include/xen/cpumask.h
@@ -368,15 +368,10 @@ static inline void free_cpumask_var(cpumask_var_t mask)
 #define FREE_CPUMASK_VAR(m) free_cpumask_var(m)
 #endif
 
-#if NR_CPUS > 1
 #define for_each_cpu(cpu, mask)			\
 	for ((cpu) = cpumask_first(mask);	\
 	     (cpu) < nr_cpu_ids;		\
 	     (cpu) = cpumask_next(cpu, mask))
-#else /* NR_CPUS == 1 */
-#define for_each_cpu(cpu, mask)			\
-	for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)(mask))
-#endif /* NR_CPUS */
 
 /*
  * The following particular system cpumasks and operations manage

Re: [PATCH] xen: fix for_each_cpu when NR_CPUS=1

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2241 bytes --]

On 11.03.21 10:40, Dario Faggioli wrote:
> When running an hypervisor build with NR_CPUS=1 for_each_cpu does not
> take into account whether the bit of the CPU is set or not in the
> provided mask.
> 
> This means that whatever we have in the bodies of these loops is always
> done once, even if the mask was empty and it should never be done. This
> is clearly a bug and was in fact causing an assert to trigger in credit2
> code.
> 
> Removing the special casing of NR_CPUS == 1 makes things work again.
> 
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Ian Jackson <iwj@xenproject.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Wei Liu <wl@xen.org>
> ---
> I'm not really sure whether this should be 4.15 material.
> 
> It's definitely a bug, IMO. The risk is also pretty low, considering
> that no one should really run Xen in this configuration (NR_CPUS=1, I
> mean). Which is also the reason why it's probably not really important
> that we fix it at this point of the release cycle.
> ---
>   xen/include/xen/cpumask.h |    5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h
> index 256b60b106..e69589fc08 100644
> --- a/xen/include/xen/cpumask.h
> +++ b/xen/include/xen/cpumask.h
> @@ -368,15 +368,10 @@ static inline void free_cpumask_var(cpumask_var_t mask)
>   #define FREE_CPUMASK_VAR(m) free_cpumask_var(m)
>   #endif
>   
> -#if NR_CPUS > 1
>   #define for_each_cpu(cpu, mask)			\
>   	for ((cpu) = cpumask_first(mask);	\
>   	     (cpu) < nr_cpu_ids;		\
>   	     (cpu) = cpumask_next(cpu, mask))
> -#else /* NR_CPUS == 1 */
> -#define for_each_cpu(cpu, mask)			\
> -	for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)(mask))
> -#endif /* NR_CPUS */

Wouldn't it make sense to drop the other NR_CPUS == 1 optimization
further down, too?

IMO this is adding clutter for no real gain, as NR_CPUS == 1 Xen
hypervisor builds aiming at high performance are probably not
existing anywhere in the universe.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] xen: fix for_each_cpu when NR_CPUS=1

[Jan Beulich]

On 11.03.2021 10:40, Dario Faggioli wrote:
> When running an hypervisor build with NR_CPUS=1 for_each_cpu does not
> take into account whether the bit of the CPU is set or not in the
> provided mask.
> 
> This means that whatever we have in the bodies of these loops is always
> done once, even if the mask was empty and it should never be done. This
> is clearly a bug and was in fact causing an assert to trigger in credit2
> code.
> 
> Removing the special casing of NR_CPUS == 1 makes things work again.
> 
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>

Doesn't this want a Reported-by: Roger?

Reviewed-by: Jan Beulich <jbeulich@suse.com>

And FTR I don't really mind the other NR_CPUS == 1 piece of logic to
remain there.

> I'm not really sure whether this should be 4.15 material.
> 
> It's definitely a bug, IMO. The risk is also pretty low, considering
> that no one should really run Xen in this configuration (NR_CPUS=1, I
> mean). Which is also the reason why it's probably not really important
> that we fix it at this point of the release cycle.

I agree; I'll put it in the 4.16 bucket.

Jan

Re: [PATCH] xen: fix for_each_cpu when NR_CPUS=1

[Dario Faggioli]

[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]

On Thu, 2021-03-11 at 12:28 +0100, Jan Beulich wrote:
> On 11.03.2021 10:40, Dario Faggioli wrote:
> > 
> > Removing the special casing of NR_CPUS == 1 makes things work again.
> > 
> > Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> 
> Doesn't this want a Reported-by: Roger?
> 
It definitely does! And I even forgot to Cc him... Sorry Roger :-(

Will you add it, or do you want me to resubmit with it?

> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
Thanks

> And FTR I don't really mind the other NR_CPUS == 1 piece of logic to
> remain there.
> 
Ok. I agree with Juergen that they're totally useless, but at least
they're not wrong.

Oh, BTW, since you mentioned in your other email the fact that this
comes from Linux, I've had a look there and there's a comment in their
cpumask.h file, under the NR_CPUS==1 define, looking like this:

/* Uniprocessor.  Assume all masks are "1". */

https://elixir.bootlin.com/linux/latest/source/include/linux/cpumask.h#L149

Of course, that does not make the fact that for_each_cpu and
for_each_cpu_not are identical less weird, and the whole thing still
does not make sense, at least not to me.

I'm wondering whether or not it is worth to report this to them too, as
I have the impression that they just don't care.

Thanks and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

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

Re: [PATCH] xen: fix for_each_cpu when NR_CPUS=1

[Roger Pau Monné]

On Thu, Mar 11, 2021 at 05:21:16PM +0100, Dario Faggioli wrote:
> On Thu, 2021-03-11 at 12:28 +0100, Jan Beulich wrote:
> > On 11.03.2021 10:40, Dario Faggioli wrote:
> > > 
> > > Removing the special casing of NR_CPUS == 1 makes things work again.
> > > 
> > > Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> > 
> > Doesn't this want a Reported-by: Roger?
> > 
> It definitely does! And I even forgot to Cc him... Sorry Roger :-(

No problem! Thanks for sending the patch.

> Will you add it, or do you want me to resubmit with it?
> 
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > 
> Thanks
> 
> > And FTR I don't really mind the other NR_CPUS == 1 piece of logic to
> > remain there.
> > 
> Ok. I agree with Juergen that they're totally useless, but at least
> they're not wrong.
> 
> Oh, BTW, since you mentioned in your other email the fact that this
> comes from Linux, I've had a look there and there's a comment in their
> cpumask.h file, under the NR_CPUS==1 define, looking like this:
> 
> /* Uniprocessor.  Assume all masks are "1". */
> 
> https://elixir.bootlin.com/linux/latest/source/include/linux/cpumask.h#L149
> 
> Of course, that does not make the fact that for_each_cpu and
> for_each_cpu_not are identical less weird, and the whole thing still
> does not make sense, at least not to me.
> 
> I'm wondering whether or not it is worth to report this to them too, as
> I have the impression that they just don't care.

I would report it, worse case they will just ignore, but it would be
nice to get it fixed there also, so that someone else doesn't have to
discover the same brokenness.

Roger.

Re: [PATCH] xen: fix for_each_cpu when NR_CPUS=1

[Jan Beulich]

On 11.03.2021 17:21, Dario Faggioli wrote:
> On Thu, 2021-03-11 at 12:28 +0100, Jan Beulich wrote:
>> On 11.03.2021 10:40, Dario Faggioli wrote:
>>>
>>> Removing the special casing of NR_CPUS == 1 makes things work again.
>>>
>>> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
>>
>> Doesn't this want a Reported-by: Roger?
>>
> It definitely does! And I even forgot to Cc him... Sorry Roger :-(
> 
> Will you add it, or do you want me to resubmit with it?

No need to, I've taken note.

Jan

Re: [PATCH] xen: fix for_each_cpu when NR_CPUS=1

[Ian Jackson]

Dario Faggioli writes ("[PATCH] xen: fix for_each_cpu when NR_CPUS=1"):
> When running an hypervisor build with NR_CPUS=1 for_each_cpu does not
> take into account whether the bit of the CPU is set or not in the
> provided mask.
> 
> This means that whatever we have in the bodies of these loops is always
> done once, even if the mask was empty and it should never be done. This
> is clearly a bug and was in fact causing an assert to trigger in credit2
> code.
> 
> Removing the special casing of NR_CPUS == 1 makes things work again.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

> I'm not really sure whether this should be 4.15 material.
> 
> It's definitely a bug, IMO. The risk is also pretty low, considering
> that no one should really run Xen in this configuration (NR_CPUS=1, I
> mean). Which is also the reason why it's probably not really important
> that we fix it at this point of the release cycle.

Given that it clearly only affects NR_CPUS==1, I think the risk/reward
tradeoff is unambiguously positive.

> -#if NR_CPUS > 1
>  #define for_each_cpu(cpu, mask)			\
>  	for ((cpu) = cpumask_first(mask);	\

Just a thought: does cpumask_first work on an empty mask ?

Ian.

Re: [PATCH] xen: fix for_each_cpu when NR_CPUS=1

[Jan Beulich]

On 12.03.2021 15:03, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH] xen: fix for_each_cpu when NR_CPUS=1"):
>> -#if NR_CPUS > 1
>>  #define for_each_cpu(cpu, mask)			\
>>  	for ((cpu) = cpumask_first(mask);	\
> 
> Just a thought: does cpumask_first work on an empty mask ?

I'm sure it does, yes - it'll return a value >= nr_cpu_ids in
this case. If it didn't work, NR_CPUS > 1 would also be badly
affected.

Jan