| Message ID | 20220825181210.284283-2-vschneid@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sched, net: NUMA-aware CPU spreading interface | expand |
+ 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.
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@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... 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.
On Sun, Aug 28, 2022 at 10:35:38AM +0200, Sander Vanheule wrote: ... > > 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. Aha, and it kills me how we have such a mess in a very core subsystem. We have 3 problems here: - mess with nr_cpumask_bits and nr_cpu_ids; - ineffectiveness of cpumask routines when nr_cpumask_bits > nr_cpu_ids; - runtime nature of nr_cpu_ids, even for those embedded systems with taught memory constraints. So that if we just drop nr_cpumask_bits, it will add 2.5K to the Image. I think that dropping nr_cpumask_bits is our only choice, and to avoid Image bloating for embedded users, we can hint the kernel that NR_CPUS is an exact number, so that it will skip setting it in runtime. I added a EXACT_NR_CPUS option for this, which works like this: #if (NR_CPUS == 1) || defined(CONFIG_EXACT_NR_CPUS) #define nr_cpu_ids ((unsigned int)NR_CPUS) #else extern unsigned int nr_cpu_ids; #endif /* Deprecated */ #define nr_cpumask_bits nr_cpu_ids I tried it with arm64 4-CPU build. When the EXACT_NR_CPUS is enabled, the difference is: add/remove: 3/4 grow/shrink: 46/729 up/down: 652/-46952 (-46300) Total: Before=25670945, After=25624645, chg -0.18% Looks quite impressive to me. I'll send a patch soon. Thanks, Yury
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h index bd047864c7ac..1414ce8cd003 100644 --- a/include/linux/cpumask.h +++ b/include/linux/cpumask.h @@ -574,7 +574,7 @@ static inline bool cpumask_empty(const struct cpumask *srcp) */ static inline bool cpumask_full(const struct cpumask *srcp) { - return bitmap_full(cpumask_bits(srcp), nr_cpumask_bits); + return bitmap_full(cpumask_bits(srcp), nr_cpu_ids); } /**
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> --- include/linux/cpumask.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)