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

[Jürgen Groß <jgross@xxxxxxxx>]

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@xxxxxxxx>
> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <george.dunlap@xxxxxxxxxx>
> Cc: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Julien Grall <julien@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> ---
> 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: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature