Re: [PATCH v3 1/9] cpumask: Make cpumask_full() check for nr_cpu_ids bits

[Sander Vanheule]

Hi Yury, Valentin,

On Thu, 2022-08-25 at 13:49 -0700, Yury Norov wrote:
> + Sander Vanheule
> 
> On Thu, Aug 25, 2022 at 07:12:02PM +0100, Valentin Schneider wrote:
> > Consider a system with 4 CPUs and:
> >   CONFIG_NR_CPUS=64
> >   CONFIG_CPUMASK_OFFSTACK=n
> > 
> > In this situation, we have:
> >   nr_cpumask_bits == NR_CPUS == 64
> >   nr_cpu_ids = 4
> > 
> > Per smp.c::setup_nr_cpu_ids(), nr_cpu_ids <= NR_CPUS, so we want
> > cpumask_full() to check for nr_cpu_ids bits set.
> > 
> > This issue is currently pointed out by the cpumask KUnit tests:
> > 
> >   [   14.072028]     # test_cpumask_weight: EXPECTATION FAILED at
> > lib/test_cpumask.c:57
> >   [   14.072028]     Expected cpumask_full(((const struct cpumask
> > *)&__cpu_possible_mask)) to be true, but is false
> >   [   14.079333]     not ok 1 - test_cpumask_weight
> > 
> > Signed-off-by: Valentin Schneider <vschneid@xxxxxxxxxx>
> 
> It's really a puzzle, and some of my thoughts are below. So. 
> 
> This is a question what for we need nr_cpumask_bits while we already
> have nr_cpu_ids. When OFFSTACK is ON, they are obviously the same.
> When it's of - the nr_cpumask_bits is an alias to NR_CPUS.
> 
> I tried to wire the nr_cpumask_bits to nr_cpu_ids unconditionally, and
> it works even when OFFSTACK is OFF, no surprises.
> 
> I didn't find any discussions describing what for we need nr_cpumask_bits,
> and the code adding it dates to a very long ago.
> 
> If I alias nr_cpumask_bits to nr_cpu_ids unconditionally on my VM with
> NR_CPUs == 256 and nr_cpu_ids == 4, there's obviously a clear win in
> performance, but the Image size gets 2.5K bigger. Probably that's the
> reason for what nr_cpumask_bits was needed...

I think it makes sense to have a compile-time-constant value for nr_cpumask_bits
in some cases. For example on embedded platforms, where every opportunity to
save a few kB should be used, or cases where NR_CPUS <= BITS_PER_LONG.

> 
> There's also a very old misleading comment in cpumask.h:
> 
>  *  If HOTPLUG is enabled, then cpu_possible_mask is forced to have
>  *  all NR_CPUS bits set, otherwise it is just the set of CPUs that
>  *  ACPI reports present at boot.
> 
> It lies, and I checked with x86_64 that cpu_possible_mask is populated
> during boot time with 0b1111, if I create a 4-cpu VM. Hence, the
> nr_cpu_ids is 4, while NR_CPUS == 256.
> 
> Interestingly, there's no a single user of the cpumask_full(),
> obviously, because it's broken. This is really a broken dead code.
> 
> Now that we have a test that checks sanity of cpumasks, this mess
> popped up.
> 
> Your fix doesn't look correct, because it fixes one function, and
> doesn't touch others. For example, the cpumask subset() may fail
> if src1p will have set bits after nr_cpu_ids, while cpumask_full()
> will be returning true.

It appears the documentation for cpumask_full() is also incorrect, because it
claims to check if all CPUs < nr_cpu_ids are set. Meanwhile, the implementation
checks if all CPUs < nr_cpumask_bits are set.

cpumask_weight() has a similar issue, and maybe also other cpumask_*() functions
(I didn't check in detail yet).

> 
> In -next, there is an update from Sander for the cpumask test that
> removes this check, and probably if you rebase on top of -next, you
> can drop this and 2nd patch of your series.
> 
> What about proper fix? I think that a long time ago we didn't have
> ACPI tables for possible cpus, and didn't populate cpumask_possible
> from that, so the
> 
>         #define nr_cpumask_bits NR_CPUS
> 
> worked well. Now that we have cpumask_possible partially filled,
> we have to always
> 
>         #define nr_cpumask_bits nr_cpu_ids
> 
> and pay +2.5K price in size even if OFFSTACK is OFF. At least, it wins
> at runtime...
> 
> Any thoughts?

It looks like both nr_cpumask_bits and nr_cpu_ids are used in a number of places
outside of lib/cpumask.c. Documentation for cpumask_*() functions almost always
refers to nr_cpu_ids as a highest valid value.

Perhaps nr_cpumask_bits should become an variable for internal cpumask usage,
and external users should only use nr_cpu_ids? The changes in 6.0 are my first
real interaction with cpumask, so it's possible that there are things I'm
missing here.

That being said, some of the cpumask tests compare results to nr_cpumask_bits,
so those should then probably be fixed to compare against nr_cpu_ids instead.

Best,
Sander

> 
> ---
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 5e2b10fb4975..0f044d93ad01 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -41,13 +41,7 @@ typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); }
> cpumask_t;
>  extern unsigned int nr_cpu_ids;
>  #endif
>  
> -#ifdef CONFIG_CPUMASK_OFFSTACK
> -/* Assuming NR_CPUS is huge, a runtime limit is more efficient.  Also,
> - * not all bits may be allocated. */
>  #define nr_cpumask_bits        nr_cpu_ids
> -#else
> -#define nr_cpumask_bits        ((unsigned int)NR_CPUS)
> -#endif
>  
>  /*
>   * The following particular system cpumasks and operations manage
> @@ -67,10 +61,6 @@ extern unsigned int nr_cpu_ids;
>   *  cpu_online_mask is the dynamic subset of cpu_present_mask,
>   *  indicating those CPUs available for scheduling.
>   *
> - *  If HOTPLUG is enabled, then cpu_possible_mask is forced to have
> - *  all NR_CPUS bits set, otherwise it is just the set of CPUs that
> - *  ACPI reports present at boot.
> - *
>   *  If HOTPLUG is enabled, then cpu_present_mask varies dynamically,
>   *  depending on what ACPI reports as currently plugged in, otherwise
>   *  cpu_present_mask is just a copy of cpu_possible_mask.