[PATCH 0/4] cpumask: cleanup nr_cpu_ids vs nr_cpumask_bits mess

[PATCH 0/4] cpumask: cleanup nr_cpu_ids vs nr_cpumask_bits mess

[Yury Norov]

cpumask subsystem uses nr_cpu_ids and nr_cpumask_bits interchangeably
despite that the variables have different meaning and purpose. It makes
some cpumask functions broken.

This series cleans that mess and adds new config FORCE_NR_CPUS that
allows to optimize cpumask subsystem if the number of CPUs is known
at compile-time.

Yury Norov (4):
  smp: add set_nr_cpu_ids()
  lib/cpumask: delete misleading comment
  lib/cpumask: deprecate nr_cpumask_bits
  lib/cpumask: add FORCE_NR_CPUS config option

 arch/loongarch/kernel/setup.c |  2 +-
 arch/mips/kernel/setup.c      |  2 +-
 arch/x86/kernel/smpboot.c     |  4 ++--
 arch/x86/xen/smp_pv.c         |  2 +-
 include/linux/cpumask.h       | 15 +++------------
 include/linux/smp.h           |  9 +++++++++
 kernel/smp.c                  |  6 ++++--
 lib/Kconfig                   | 10 ++++++++++
 8 files changed, 31 insertions(+), 19 deletions(-)

-- 
2.34.1

[PATCH 1/4] smp: add set_nr_cpu_ids()

[Yury Norov]

In preparation for support of compile-time nr_cpu_ids, add a setter for
the variable.

This is a no-op for all arches.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 arch/loongarch/kernel/setup.c | 2 +-
 arch/mips/kernel/setup.c      | 2 +-
 arch/x86/kernel/smpboot.c     | 4 ++--
 arch/x86/xen/smp_pv.c         | 2 +-
 include/linux/smp.h           | 5 +++++
 kernel/smp.c                  | 4 ++--
 6 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/loongarch/kernel/setup.c b/arch/loongarch/kernel/setup.c
index 8f5c2f9a1a83..18a81edd3ac5 100644
--- a/arch/loongarch/kernel/setup.c
+++ b/arch/loongarch/kernel/setup.c
@@ -346,7 +346,7 @@ static void __init prefill_possible_map(void)
 	for (; i < NR_CPUS; i++)
 		set_cpu_possible(i, false);
 
-	nr_cpu_ids = possible;
+	set_nr_cpu_ids(possible);
 }
 #endif
 
diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 2ca156a5b231..e8a0759cb4d0 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -750,7 +750,7 @@ static void __init prefill_possible_map(void)
 	for (; i < NR_CPUS; i++)
 		set_cpu_possible(i, false);
 
-	nr_cpu_ids = possible;
+	set_nr_cpu_ids(possible);
 }
 #else
 static inline void prefill_possible_map(void) {}
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index f24227bc3220..3f3ea0287f69 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1316,7 +1316,7 @@ static void __init smp_sanity_check(void)
 			nr++;
 		}
 
-		nr_cpu_ids = 8;
+		set_nr_cpu_ids(8);
 	}
 #endif
 
@@ -1569,7 +1569,7 @@ __init void prefill_possible_map(void)
 		possible = i;
 	}
 
-	nr_cpu_ids = possible;
+	set_nr_cpu_ids(possible);
 
 	pr_info("Allowing %d CPUs, %d hotplug CPUs\n",
 		possible, max_t(int, possible - num_processors, 0));
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index ba7af2eca755..480be82e9b7b 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -179,7 +179,7 @@ static void __init _get_smp_config(unsigned int early)
 	 * hypercall to expand the max number of VCPUs an already
 	 * running guest has. So cap it up to X. */
 	if (subtract)
-		nr_cpu_ids = nr_cpu_ids - subtract;
+		set_nr_cpu_ids(nr_cpu_ids - subtract);
 #endif
 
 }
diff --git a/include/linux/smp.h b/include/linux/smp.h
index a80ab58ae3f1..c9e7a9abef2f 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -181,6 +181,11 @@ static inline int get_boot_cpu_id(void)
 	return __boot_cpu_id;
 }
 
+static inline void set_nr_cpu_ids(unsigned int nr)
+{
+	nr_cpu_ids = nr;
+}
+
 #else /* !SMP */
 
 static inline void smp_send_stop(void) { }
diff --git a/kernel/smp.c b/kernel/smp.c
index 650810a6f29b..27d4ed0aee55 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -1070,7 +1070,7 @@ static int __init nrcpus(char *str)
 	int nr_cpus;
 
 	if (get_option(&str, &nr_cpus) && nr_cpus > 0 && nr_cpus < nr_cpu_ids)
-		nr_cpu_ids = nr_cpus;
+		set_nr_cpu_ids(nr_cpus);
 
 	return 0;
 }
@@ -1095,7 +1095,7 @@ EXPORT_SYMBOL(nr_cpu_ids);
 /* An arch may set nr_cpu_ids earlier if needed, so this would be redundant */
 void __init setup_nr_cpu_ids(void)
 {
-	nr_cpu_ids = find_last_bit(cpumask_bits(cpu_possible_mask),NR_CPUS) + 1;
+	set_nr_cpu_ids(find_last_bit(cpumask_bits(cpu_possible_mask), NR_CPUS) + 1);
 }
 
 /* Called by boot processor to activate the rest. */
-- 
2.34.1

[PATCH 2/4] lib/cpumask: delete misleading comment

[Yury Norov]

The comment says that HOTPLUG config option enables all cpus in
cpu_possible_mask up to NR_CPUs. This is wrong. Even if HOTPLUG is
enabled, the mask is populated on boot with respect to ACPI/DT records.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 include/linux/cpumask.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index bd047864c7ac..f70bf9ffa9d3 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -67,10 +67,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.
-- 
2.34.1

[PATCH 3/4] lib/cpumask: deprecate nr_cpumask_bits

[Yury Norov]

Cpumask code is written in assumption that when CONFIG_CPUMASK_OFFSTACK
is enabled, all cpumasks have boot-time defined size, otherwise the size
is always NR_CPUS.

The latter is wrong because the number of possible cpus is always
calculated on boot, and it may be less than NR_CPUS.

On my 4-cpu arm64 VM the nr_cpu_ids is 4, as expected, and nr_cpumask_bits
is 256, which corresponds to NR_CPUS. This not only leads to useless
traversing of cpumask bits greater than 4, this also makes some cpumask
routines fail.

For example, cpumask_full(0b1111000..000) would erroneously return false
in the example above because tail bits in the mask are all unset.

This patch deprecates nr_cpumask_bits and wires it to nr_cpu_ids
unconditionally, so that cpumask routines will not waste time traversing
unused part of cpu masks. It also fixes cpumask_full() and similar
routines.

As a side effect, because now a length of cpumasks is defined at run-time
even if CPUMASK_OFFSTACK is disabled, compiler can't optimize corresponding
functions.

It increases kernel size by ~2.5KB if OFFSTACK is off. This is addressed in
the following patch.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 include/linux/cpumask.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index f70bf9ffa9d3..b7b863cf8831 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -41,13 +41,8 @@ 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. */
+/* Deprecated. Always use nr_cpu_ids. */
 #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
-- 
2.34.1

[PATCH 4/4] lib/cpumask: add FORCE_NR_CPUS config option

[Yury Norov]

The size of cpumasks is hard-limited by compile-time parameter NR_CPUS,
but defined at boot-time when kernel parses ACPI/DT tables, and stored in
nr_cpu_ids. In many practical cases, number of CPUs for a target is known
at compile time, and can be provided with NR_CPUS.

In that case, compiler may be instructed to rely on NR_CPUS as on actual
number of CPUs, not an upper limit. It allows to optimize many cpumask
routines and significantly shrink size of the kernel image.

This patch adds FORCE_NR_CPUS option to teach the compiler to rely on
NR_CPUS and enable corresponding optimizations.

If FORCE_NR_CPUS=y, kernel will not set nr_cpu_ids at boot, but only check
that the actual number of possible CPUs is equal to NR_CPUS, and WARN if
that doesn't hold.

The new option is especially useful in embedded applications because
kernel configurations are unique for each SoC, the number of CPUs is
constant and known well, and memory limitations are typically harder.

For my 4-CPU ARM64 build with NR_CPUS=4, FORCE_NR_CPUS=y saves 46KB:
  add/remove: 3/4 grow/shrink: 46/729 up/down: 652/-46952 (-46300)

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 include/linux/cpumask.h |  4 ++--
 include/linux/smp.h     |  4 ++++
 kernel/smp.c            |  2 ++
 lib/Kconfig             | 10 ++++++++++
 4 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index b7b863cf8831..702c05329853 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -35,8 +35,8 @@ typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
  */
 #define cpumask_pr_args(maskp)		nr_cpu_ids, cpumask_bits(maskp)
 
-#if NR_CPUS == 1
-#define nr_cpu_ids		1U
+#if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
+#define nr_cpu_ids ((unsigned int)NR_CPUS)
 #else
 extern unsigned int nr_cpu_ids;
 #endif
diff --git a/include/linux/smp.h b/include/linux/smp.h
index c9e7a9abef2f..e9e74e08c886 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -183,7 +183,11 @@ static inline int get_boot_cpu_id(void)
 
 static inline void set_nr_cpu_ids(unsigned int nr)
 {
+#ifdef CONFIG_FORCE_NR_CPUS
+	WARN_ON(nr_cpu_ids != nr);
+#else
 	nr_cpu_ids = nr;
+#endif
 }
 
 #else /* !SMP */
diff --git a/kernel/smp.c b/kernel/smp.c
index 27d4ed0aee55..9cd1d02221ba 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -1088,9 +1088,11 @@ static int __init maxcpus(char *str)
 
 early_param("maxcpus", maxcpus);
 
+#ifndef CONFIG_FORCE_NR_CPUS
 /* Setup number of possible processor ids */
 unsigned int nr_cpu_ids __read_mostly = NR_CPUS;
 EXPORT_SYMBOL(nr_cpu_ids);
+#endif
 
 /* An arch may set nr_cpu_ids earlier if needed, so this would be redundant */
 void __init setup_nr_cpu_ids(void)
diff --git a/lib/Kconfig b/lib/Kconfig
index dc1ab2ed1dc6..5de23382e5f3 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -527,6 +527,16 @@ config CPUMASK_OFFSTACK
 	  them on the stack.  This is a bit more expensive, but avoids
 	  stack overflow.
 
+config FORCE_NR_CPUS
+       bool "NR_CPUS is set to an actual number of CPUs"
+       depends on SMP
+       default n
+       help
+         Say Yes if you have NR_CPUS set to an actual number of possible
+         CPUs in your system, not to a default value. This forces the core
+         code to rely on compile-time value and optimize kernel routines
+         better.
+
 config CPU_RMAP
 	bool
 	depends on SMP
-- 
2.34.1

Re: [PATCH 4/4] lib/cpumask: add FORCE_NR_CPUS config option

[Andy Shevchenko]

On Mon, Aug 29, 2022 at 09:57:48AM -0700, Yury Norov wrote:
> The size of cpumasks is hard-limited by compile-time parameter NR_CPUS,
> but defined at boot-time when kernel parses ACPI/DT tables, and stored in
> nr_cpu_ids. In many practical cases, number of CPUs for a target is known
> at compile time, and can be provided with NR_CPUS.
> 
> In that case, compiler may be instructed to rely on NR_CPUS as on actual
> number of CPUs, not an upper limit. It allows to optimize many cpumask
> routines and significantly shrink size of the kernel image.
> 
> This patch adds FORCE_NR_CPUS option to teach the compiler to rely on
> NR_CPUS and enable corresponding optimizations.
> 
> If FORCE_NR_CPUS=y, kernel will not set nr_cpu_ids at boot, but only check
> that the actual number of possible CPUs is equal to NR_CPUS, and WARN if
> that doesn't hold.
> 
> The new option is especially useful in embedded applications because
> kernel configurations are unique for each SoC, the number of CPUs is
> constant and known well, and memory limitations are typically harder.
> 
> For my 4-CPU ARM64 build with NR_CPUS=4, FORCE_NR_CPUS=y saves 46KB:
>   add/remove: 3/4 grow/shrink: 46/729 up/down: 652/-46952 (-46300)

...

> +       default n

Redundant. 'n' _is_ default.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] lib/cpumask: add FORCE_NR_CPUS config option

[Yury Norov]

+ Vivek Goyal <vgoyal@redhat.com>
+ Yinghai Lu <yinghai@kernel.org>
+ H. Peter Anvin <hpa@linux.intel.com>

On Wed, Aug 31, 2022 at 02:33:41AM +0800, kernel test robot wrote:
> Hi Yury,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on tip/x86/core]
> [also build test WARNING on linus/master v6.0-rc3 next-20220830]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Yury-Norov/cpumask-cleanup-nr_cpu_ids-vs-nr_cpumask_bits-mess/20220830-010755
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git bc12b70f7d216b36bd87701349374a13e486f8eb
> config: x86_64-randconfig-m001 (https://download.01.org/0day-ci/archive/20220831/202208310215.C2IzssKr-lkp@intel.com/config)
> compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
> 
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
> 
> smatch warnings:
> arch/x86/kernel/apic/apic.c:2437 generic_processor_info() warn: always true condition '(num_processors >= (1) - 1) => (0-u32max >= 0)'

This is the code that woke up the smatch:
  /*
   * If boot cpu has not been detected yet, then only allow upto
   * nr_cpu_ids - 1 processors and keep one slot free for boot cpu
   */
  if (!boot_cpu_detected && num_processors >= nr_cpu_ids - 1 &&
      apicid != boot_cpu_physical_apicid) {
          int thiscpu = max + disabled_cpus - 1;

          pr_warn("APIC: NR_CPUS/possible_cpus limit of %i almost"
                  " reached. Keeping one slot for boot cpu."
                  "  Processor %d/0x%x ignored.\n", max, thiscpu, apicid);

          disabled_cpus++;
          return -ENODEV;
  }

It has been added in a patch 14cb6dcf0a023f597 ("x86, boot: Wait for
boot cpu to show up if nr_cpus limit is about to hit")

My patch adds an option FORCE_NR_CPUS that makes nr_cpu_ids a compile-time
defined.

Hence, the num_processors >= nr_cpus - 1,
may become: num_processors >= 0, if NR_CPUS == 1.

So the plain straightforward fix would be:

    if (!boot_cpu_detected &&
 #if (NR_CPUS > 1)
        num_processors >= nr_cpu_ids - 1 &&
 #endif
        apicid != boot_cpu_physical_apicid) { ... }

However, I have a feeling that all the logic above is not needed at
all on UP machines. If that's true, the '#if NR_CPUS > 1' should
protect the whole condition, or even bigger piece of the
generic_processor_info().

Guys, could you please comment on that?

Thanks,
Yury

Re: [PATCH 4/4] lib/cpumask: add FORCE_NR_CPUS config option

[Yury Norov]

On Tue, Aug 30, 2022 at 02:05:09PM -0700, Yury Norov wrote:
> + Vivek Goyal <vgoyal@redhat.com>
> + Yinghai Lu <yinghai@kernel.org>
> + H. Peter Anvin <hpa@linux.intel.com>
> 
> On Wed, Aug 31, 2022 at 02:33:41AM +0800, kernel test robot wrote:
> > Hi Yury,
> > 
> > I love your patch! Perhaps something to improve:
> > 
> > [auto build test WARNING on tip/x86/core]
> > [also build test WARNING on linus/master v6.0-rc3 next-20220830]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Yury-Norov/cpumask-cleanup-nr_cpu_ids-vs-nr_cpumask_bits-mess/20220830-010755
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git bc12b70f7d216b36bd87701349374a13e486f8eb
> > config: x86_64-randconfig-m001 (https://download.01.org/0day-ci/archive/20220831/202208310215.C2IzssKr-lkp@intel.com/config)
> > compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
> > 
> > If you fix the issue, kindly add following tag where applicable
> > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > smatch warnings:
> > arch/x86/kernel/apic/apic.c:2437 generic_processor_info() warn: always true condition '(num_processors >= (1) - 1) => (0-u32max >= 0)'
> 
> This is the code that woke up the smatch:
>   /*
>    * If boot cpu has not been detected yet, then only allow upto
>    * nr_cpu_ids - 1 processors and keep one slot free for boot cpu
>    */
>   if (!boot_cpu_detected && num_processors >= nr_cpu_ids - 1 &&
>       apicid != boot_cpu_physical_apicid) {
>           int thiscpu = max + disabled_cpus - 1;
> 
>           pr_warn("APIC: NR_CPUS/possible_cpus limit of %i almost"
>                   " reached. Keeping one slot for boot cpu."
>                   "  Processor %d/0x%x ignored.\n", max, thiscpu, apicid);
> 
>           disabled_cpus++;
>           return -ENODEV;
>   }
> 
> It has been added in a patch 14cb6dcf0a023f597 ("x86, boot: Wait for
> boot cpu to show up if nr_cpus limit is about to hit")
> 
> My patch adds an option FORCE_NR_CPUS that makes nr_cpu_ids a compile-time
> defined.
> 
> Hence, the num_processors >= nr_cpus - 1,
> may become: num_processors >= 0, if NR_CPUS == 1.
> 
> So the plain straightforward fix would be:
> 
>     if (!boot_cpu_detected &&
>  #if (NR_CPUS > 1)
>         num_processors >= nr_cpu_ids - 1 &&
>  #endif
>         apicid != boot_cpu_physical_apicid) { ... }
> 
> However, I have a feeling that all the logic above is not needed at
> all on UP machines. If that's true, the '#if NR_CPUS > 1' should
> protect the whole condition, or even bigger piece of the
> generic_processor_info().

As Dave Hansen said:

  I think it's a reasonable warning, but it's also not something we need
  to hack around.  We can surely only land in here with
  boot_cpu_detected==true if NR_CPUS==1, so the rest of the expression is
  moot.
  
  I don't think it's worth adding the #ifdef.

So, leaving things as they are.

If no other comments, moving the series into bitmap-for-next.

Thanks,
Yury