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

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

[Geert Uytterhoeven]

Hi Yury,

On Tue, Oct 18, 2022 at 10:21 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Sep 6, 2022 at 1:10 AM Yury Norov <yury.norov@gmail.com> 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)
> >
> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
>
> Thanks for your patch, which is now commit 6f9c07be9d020489
> ("lib/cpumask: add FORCE_NR_CPUS config option") in v6.1-rc1.
>
> FORCE_NR_CPUS is enabled for e.g. an allmodconfig kernel, which I
> believe now makes it unsafe to boot such a kernel on any system that
> does not have exactly CONFIG_NR_CPUS CPU cores?
>
> If my assumption is true, this really needs some protection to prevent
> enabling this option inadvertently, as it is quite common to boot
> allmodconfig kernels for testing.

Moreover, this cannot be used on all systems.  E.g. on Icicle Kit with
Microchip PolarFire SoC, CONFIG_NR_CPUS needs to be larger than 4,
as the system has actually 5 CPU cores (1xE51 and 4xU54), but Linux
runs only on 4 of them.  So you cannot use FORCE_NR_CPUS=y.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Andreas Schwab]

On Okt 18 2022, Geert Uytterhoeven wrote:

> Moreover, this cannot be used on all systems.  E.g. on Icicle Kit with
> Microchip PolarFire SoC, CONFIG_NR_CPUS needs to be larger than 4,
> as the system has actually 5 CPU cores (1xE51 and 4xU54), but Linux
> runs only on 4 of them.  So you cannot use FORCE_NR_CPUS=y.

But does Linux acually see the E51 core?  On the Hifive boards it is
disabled in the device tree, and the cpu probing just skips it,
effectively resulting in only four cpus.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Geert Uytterhoeven]

Hi Andreas,

On Tue, Oct 18, 2022 at 3:41 PM Andreas Schwab <schwab@suse.de> wrote:
> On Okt 18 2022, Geert Uytterhoeven wrote:
> > Moreover, this cannot be used on all systems.  E.g. on Icicle Kit with
> > Microchip PolarFire SoC, CONFIG_NR_CPUS needs to be larger than 4,
> > as the system has actually 5 CPU cores (1xE51 and 4xU54), but Linux
> > runs only on 4 of them.  So you cannot use FORCE_NR_CPUS=y.
>
> But does Linux acually see the E51 core?  On the Hifive boards it is
> disabled in the device tree, and the cpu probing just skips it,
> effectively resulting in only four cpus.

The E51 is indeed disabled in DT.
The CPU parts of arch/riscv/boot/dts/sifive/fu540-c000.dtsi and
arch/riscv/boot/dts/microchip/mpfs.dtsi arre very similar.
Do you get 4 CPUs on Hifive with CONFIG_NR_CPUS=4?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Yury Norov]

On Tue, Oct 18, 2022 at 03:50:31PM +0200, Geert Uytterhoeven wrote:
> Hi Andreas,
> 
> On Tue, Oct 18, 2022 at 3:41 PM Andreas Schwab <schwab@suse.de> wrote:
> > On Okt 18 2022, Geert Uytterhoeven wrote:
> > > Moreover, this cannot be used on all systems.  E.g. on Icicle Kit with
> > > Microchip PolarFire SoC, CONFIG_NR_CPUS needs to be larger than 4,
> > > as the system has actually 5 CPU cores (1xE51 and 4xU54), but Linux
> > > runs only on 4 of them.  So you cannot use FORCE_NR_CPUS=y.
> >
> > But does Linux acually see the E51 core?  On the Hifive boards it is
> > disabled in the device tree, and the cpu probing just skips it,
> > effectively resulting in only four cpus.
> 
> The E51 is indeed disabled in DT.
> The CPU parts of arch/riscv/boot/dts/sifive/fu540-c000.dtsi and
> arch/riscv/boot/dts/microchip/mpfs.dtsi arre very similar.
> Do you get 4 CPUs on Hifive with CONFIG_NR_CPUS=4?
> 
> Gr{oetje,eeting}s,

Hi Geert, Andreas,

Thanks for pointing that. Indeed, it should be disabled in
allmodconfig.

Linus also asked to make this option depending on CONFIG_EXPERT.
So, I'm working on a patch.

From general considerations, NR_CPUS defines length of cpumasks,
per-cpu arrays etc. If it's impossible to boot Linux on a specific
core, we don't need to reserve memory for all that. In other words,
number of possible CPUs in your example should be equal to 4.

When FORCE_NR_CPUS=y, the boot code still parses DT/ACPI tables for
actual numbers of CPUs, and if it doesn't equal to NR_CPUS, prints
a message in syslog. 

For those who choose FORCE_NR_CPUS, it's required to set NR_CPUS
to a value that matches to what's parsed from DT.

Can you please look at the draft below that disables FORCE_NR_CPUS
in allmodconfig? If it's OK with you, I'll send a patch. If you think
that there are architectures where it's not possible to set correct
NR_CPUS at compile time for some reason, I'll add ARCH_UNFORCE_NR_CPUS
option.

Thanks,
Yury
---
 lib/Kconfig | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/lib/Kconfig b/lib/Kconfig
index 9bbf8a4b2108..578cee3593d7 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -528,15 +528,33 @@ config CPUMASK_OFFSTACK
 	  them on the stack.  This is a bit more expensive, but avoids
 	  stack overflow.
 
+choice
+	prompt "Number of CPUs detection"
+	default UNFORCE_NR_CPUS
+        depends on SMP && EXPERT
+	help
+	  Select between boot-time and compile-time detection of number
+	  of CPUs. If it's possible to provide exact number of CPUs at
+	  compile-time, kernel code may be optimized better.
+
+	  For general-purpose kernel, choose "boot time" option.
+
+config UNFORCE_NR_CPUS
+	bool "Detect number of CPUs at boot time"
+	help
+	  Choose it if you build general-purpose kernel and want to rely
+	  on kernel to detect actual number of CPUs.
+
 config FORCE_NR_CPUS
        bool "NR_CPUS is set to an actual number of CPUs"
-       depends on SMP
        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.
 
+endchoice
+
 config CPU_RMAP
 	bool
 	depends on SMP
-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Andy Shevchenko]

On Tue, Oct 18, 2022 at 07:35:09AM -0700, Yury Norov wrote:
> On Tue, Oct 18, 2022 at 03:50:31PM +0200, Geert Uytterhoeven wrote:

...

> For those who choose FORCE_NR_CPUS, it's required to set NR_CPUS
> to a value that matches to what's parsed from DT.
> 
> Can you please look at the draft below that disables FORCE_NR_CPUS
> in allmodconfig? If it's OK with you, I'll send a patch. If you think
> that there are architectures where it's not possible to set correct
> NR_CPUS at compile time for some reason, I'll add ARCH_UNFORCE_NR_CPUS
> option.

Instead you may simply add

	depends on CONFIG_$ARCH/$MACHINE=n

and so on to the FORCE_NR_CPUS, no?

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Yury Norov]

On Tue, Oct 18, 2022 at 05:44:09PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 18, 2022 at 07:35:09AM -0700, Yury Norov wrote:
> > On Tue, Oct 18, 2022 at 03:50:31PM +0200, Geert Uytterhoeven wrote:
> 
> ...
> 
> > For those who choose FORCE_NR_CPUS, it's required to set NR_CPUS
> > to a value that matches to what's parsed from DT.
> > 
> > Can you please look at the draft below that disables FORCE_NR_CPUS
> > in allmodconfig? If it's OK with you, I'll send a patch. If you think
> > that there are architectures where it's not possible to set correct
> > NR_CPUS at compile time for some reason, I'll add ARCH_UNFORCE_NR_CPUS
> > option.
> 
> Instead you may simply add
> 
> 	depends on CONFIG_$ARCH/$MACHINE=n
> 
> and so on to the FORCE_NR_CPUS, no?

Yes, if there's just one machine like that. If there's many of them, the
'depends' list would be too long.

I hope there's no such a weird machines, and we don't need that at
all. Let's see what Geert will say.


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Geert Uytterhoeven]

Hi Yuri,

On Tue, Oct 18, 2022 at 5:01 PM Yury Norov <yury.norov@gmail.com> wrote:
> On Tue, Oct 18, 2022 at 05:44:09PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 18, 2022 at 07:35:09AM -0700, Yury Norov wrote:
> > > On Tue, Oct 18, 2022 at 03:50:31PM +0200, Geert Uytterhoeven wrote:
> >
> > ...
> >
> > > For those who choose FORCE_NR_CPUS, it's required to set NR_CPUS
> > > to a value that matches to what's parsed from DT.
> > >
> > > Can you please look at the draft below that disables FORCE_NR_CPUS
> > > in allmodconfig? If it's OK with you, I'll send a patch. If you think
> > > that there are architectures where it's not possible to set correct
> > > NR_CPUS at compile time for some reason, I'll add ARCH_UNFORCE_NR_CPUS
> > > option.
> >
> > Instead you may simply add
> >
> >       depends on CONFIG_$ARCH/$MACHINE=n
> >
> > and so on to the FORCE_NR_CPUS, no?
>
> Yes, if there's just one machine like that. If there's many of them, the
> 'depends' list would be too long.
>
> I hope there's no such a weird machines, and we don't need that at
> all. Let's see what Geert will say.

I haven't tried the patch from your other email yet, but I did try
CONFIG_NR_CPUS=4 and CONFIG_FORCE_NR_CPUS=y on
Icicle earlier today.

There was no warning, as the number of CPUs did match, but the
fourth CPU (cpu@4, i.e. the fifth core in DT) failed to come online:

    CPU3: failed to come online
    smp: Brought up 1 node, 3 CPUs

BTW, it behaves the same with CONFIG_FORCE_NR_CPUS=n.
Increasing CONFIG_NR_CPUS (before I used 8) makes the fourth
CPU core come online again.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Yury Norov]

On Tue, Oct 18, 2022 at 05:15:41PM +0200, Geert Uytterhoeven wrote:
> Hi Yuri,
> 
> On Tue, Oct 18, 2022 at 5:01 PM Yury Norov <yury.norov@gmail.com> wrote:
> > On Tue, Oct 18, 2022 at 05:44:09PM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 18, 2022 at 07:35:09AM -0700, Yury Norov wrote:
> > > > On Tue, Oct 18, 2022 at 03:50:31PM +0200, Geert Uytterhoeven wrote:
> > >
> > > ...
> > >
> > > > For those who choose FORCE_NR_CPUS, it's required to set NR_CPUS
> > > > to a value that matches to what's parsed from DT.

...

> I haven't tried the patch from your other email yet, but I did try
> CONFIG_NR_CPUS=4 and CONFIG_FORCE_NR_CPUS=y on
> Icicle earlier today.
> 
> There was no warning, as the number of CPUs did match, but the
> fourth CPU (cpu@4, i.e. the fifth core in DT) failed to come online:
> 
>     CPU3: failed to come online
>     smp: Brought up 1 node, 3 CPUs
> 
> BTW, it behaves the same with CONFIG_FORCE_NR_CPUS=n.
> Increasing CONFIG_NR_CPUS (before I used 8) makes the fourth
> CPU core come online again.

The problem is seemingly unrelated to CONFIG_FORCE_NR_CPUS...
If so, we don't need ARCH_UNFORCE_NR_CPUS. Is that right?

This all looks weird. RISCV hasn't an arch code to setup nr_cpu_ids,
and therefore should use generic setup_nr_cpu_ids(), which is:

  void __init setup_nr_cpu_ids(void)
  {
          set_nr_cpu_ids(find_last_bit(cpumask_bits(cpu_possible_mask), NR_CPUS) + 1);
  }

Where:

  static inline void set_nr_cpu_ids(unsigned int nr)
  {
  #if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
          WARN_ON(nr != nr_cpu_ids);
  #else
          nr_cpu_ids = nr;
  #endif
  }


As you can see, at this point cpu_possible_mask is initialized based
on DT, and even if arch has non-dense cpu_possible_mask, the logic
should still be correct.

Wish I could tell more, if I had an access to the hardware...

Thanks,
Yury

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Geert Uytterhoeven]

Hi Yury,

On Tue, Oct 18, 2022 at 6:19 PM Yury Norov <yury.norov@gmail.com> wrote:
> On Tue, Oct 18, 2022 at 05:15:41PM +0200, Geert Uytterhoeven wrote:
> > On Tue, Oct 18, 2022 at 5:01 PM Yury Norov <yury.norov@gmail.com> wrote:
> > > On Tue, Oct 18, 2022 at 05:44:09PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Oct 18, 2022 at 07:35:09AM -0700, Yury Norov wrote:
> > > > > On Tue, Oct 18, 2022 at 03:50:31PM +0200, Geert Uytterhoeven wrote:
> > > >
> > > > ...
> > > >
> > > > > For those who choose FORCE_NR_CPUS, it's required to set NR_CPUS
> > > > > to a value that matches to what's parsed from DT.
>
> ...
>
> > I haven't tried the patch from your other email yet, but I did try
> > CONFIG_NR_CPUS=4 and CONFIG_FORCE_NR_CPUS=y on
> > Icicle earlier today.
> >
> > There was no warning, as the number of CPUs did match, but the
> > fourth CPU (cpu@4, i.e. the fifth core in DT) failed to come online:
> >
> >     CPU3: failed to come online
> >     smp: Brought up 1 node, 3 CPUs
> >
> > BTW, it behaves the same with CONFIG_FORCE_NR_CPUS=n.
> > Increasing CONFIG_NR_CPUS (before I used 8) makes the fourth
> > CPU core come online again.
>
> The problem is seemingly unrelated to CONFIG_FORCE_NR_CPUS...
> If so, we don't need ARCH_UNFORCE_NR_CPUS. Is that right?
>
> This all looks weird. RISCV hasn't an arch code to setup nr_cpu_ids,
> and therefore should use generic setup_nr_cpu_ids(), which is:
>
>   void __init setup_nr_cpu_ids(void)
>   {
>           set_nr_cpu_ids(find_last_bit(cpumask_bits(cpu_possible_mask), NR_CPUS) + 1);
>   }
>
> Where:
>
>   static inline void set_nr_cpu_ids(unsigned int nr)
>   {
>   #if (NR_CPUS == 1) || defined(CONFIG_FORCE_NR_CPUS)
>           WARN_ON(nr != nr_cpu_ids);
>   #else
>           nr_cpu_ids = nr;
>   #endif
>   }
>
>
> As you can see, at this point cpu_possible_mask is initialized based
> on DT, and even if arch has non-dense cpu_possible_mask, the logic
> should still be correct.

Quite possible this is just an issue with the RISC-V CPU sparse hart ID
handling code.  E.g. arm64 works fine with cpu@{0,1,2,3,100,101,102,103}
and CONFIG_NR_CPUS=8.  NR_CPUS in arch/riscv/Kconfig has always
defaulted to at least 8, while all upstream DTS files describe only
4 Linux-capable CPU cores (+ 1 management core).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv