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

[Yury Norov <yury.norov@gmail.com>]

+ 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@redhat.com>

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

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.

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?

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