| Message ID | 20240127220026.1722399-1-yoann.congal@smile.fr |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] printk: Remove redundant CONFIG_BASE_SMALL | expand |
On 27. 01. 24, 23:00, Yoann Congal wrote: > CONFIG_BASE_SMALL is currently a type int but is only used as a boolean > equivalent to !CONFIG_BASE_FULL. > > So, remove it entirely and move every usage to !CONFIG_BASE_FULL. > > In addition, recent kconfig changes (see the discussion in Closes: tag) > revealed that using: > config SOMETHING > default "some value" if X > does not work as expected if X is not of type bool. > > CONFIG_BASE_SMALL was used that way in init/Kconfig: > config LOG_CPU_MAX_BUF_SHIFT > default 12 if !BASE_SMALL > default 0 if BASE_SMALL > > Note: This changes CONFIG_LOG_CPU_MAX_BUF_SHIFT=12 to > CONFIG_LOG_CPU_MAX_BUF_SHIFT=0 for some defconfigs, but that will not be > a big impact due to this code in kernel/printk/printk.c: > /* by default this will only continue through for large > 64 CPUs */ > if (cpu_extra <= __LOG_BUF_LEN / 2) > return; > > Signed-off-by: Yoann Congal <yoann.congal@smile.fr> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Closes: https://lore.kernel.org/all/CAMuHMdWm6u1wX7efZQf=2XUAHascps76YQac6rdnQGhc8nop_Q@mail.gmail.com/ > Fixes: 4e244c10eab3 ("kconfig: remove unneeded symbol_empty variable") > --- > v1 patch was named "treewide: Change CONFIG_BASE_SMALL to bool type" > https://lore.kernel.org/all/20240126163032.1613731-1-yoann.congal@smile.fr/ > > v1 -> v2: Applied Masahiro Yamada's comments (Thanks!): > * Changed from "Change CONFIG_BASE_SMALL to type bool" to > "Remove it and switch usage to !CONFIG_BASE_FULL" > * Fixed "Fixes:" tag and reference to the mailing list thread. > * Added a note about CONFIG_LOG_CPU_MAX_BUF_SHIFT changing. ... > --- a/drivers/tty/vt/vc_screen.c > +++ b/drivers/tty/vt/vc_screen.c > @@ -51,7 +51,7 @@ > #include <asm/unaligned.h> > > #define HEADER_SIZE 4u > -#define CON_BUF_SIZE (CONFIG_BASE_SMALL ? 256 : PAGE_SIZE) > +#define CON_BUF_SIZE (IS_ENABLED(CONFIG_BASE_FULL) ? PAGE_SIZE : 256) Why is the IS_ENABLED() addition needed? You don't say anything about that in the commit log. thanks,
Le 29/01/2024 à 12:28, Jiri Slaby a écrit : > On 27. 01. 24, 23:00, Yoann Congal wrote: >> CONFIG_BASE_SMALL is currently a type int but is only used as a boolean >> equivalent to !CONFIG_BASE_FULL. >> >> So, remove it entirely and move every usage to !CONFIG_BASE_FULL. >> >> In addition, recent kconfig changes (see the discussion in Closes: tag) >> revealed that using: >> config SOMETHING >> default "some value" if X >> does not work as expected if X is not of type bool. >> >> CONFIG_BASE_SMALL was used that way in init/Kconfig: >> config LOG_CPU_MAX_BUF_SHIFT >> default 12 if !BASE_SMALL >> default 0 if BASE_SMALL >> >> Note: This changes CONFIG_LOG_CPU_MAX_BUF_SHIFT=12 to >> CONFIG_LOG_CPU_MAX_BUF_SHIFT=0 for some defconfigs, but that will not be >> a big impact due to this code in kernel/printk/printk.c: >> /* by default this will only continue through for large > 64 CPUs */ >> if (cpu_extra <= __LOG_BUF_LEN / 2) >> return; >> >> Signed-off-by: Yoann Congal <yoann.congal@smile.fr> >> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> >> Closes: https://lore.kernel.org/all/CAMuHMdWm6u1wX7efZQf=2XUAHascps76YQac6rdnQGhc8nop_Q@mail.gmail.com/ >> Fixes: 4e244c10eab3 ("kconfig: remove unneeded symbol_empty variable") >> --- >> v1 patch was named "treewide: Change CONFIG_BASE_SMALL to bool type" >> https://lore.kernel.org/all/20240126163032.1613731-1-yoann.congal@smile.fr/ >> >> v1 -> v2: Applied Masahiro Yamada's comments (Thanks!): >> * Changed from "Change CONFIG_BASE_SMALL to type bool" to >> "Remove it and switch usage to !CONFIG_BASE_FULL" >> * Fixed "Fixes:" tag and reference to the mailing list thread. >> * Added a note about CONFIG_LOG_CPU_MAX_BUF_SHIFT changing. > ... >> --- a/drivers/tty/vt/vc_screen.c >> +++ b/drivers/tty/vt/vc_screen.c >> @@ -51,7 +51,7 @@ >> #include <asm/unaligned.h> >> #define HEADER_SIZE 4u >> -#define CON_BUF_SIZE (CONFIG_BASE_SMALL ? 256 : PAGE_SIZE) >> +#define CON_BUF_SIZE (IS_ENABLED(CONFIG_BASE_FULL) ? PAGE_SIZE : 256) > > Why is the IS_ENABLED() addition needed? You don't say anything about that in the commit log. > > thanks, It is needed because we go from using CONFIG_BASE_*SMALL* which is of type _int_ (so either defined to 0 or some other non-zero value) to CONFIG_BASE_*FULL* which is of type _bool_ (so, it is either enabled or not). If I understood correctly, the proper way to check a config of type bool inside of a C function is with IS_ENABLED(). Another way to say this is : CONFIG_BASE_SMALL != 0 is equivalent to !IS_ENABLED(CONFIG_BASE_FULL) Finally, CONFIG_XXX is not defined if CONFIG_XXX is a type bool and disabled so : CONFIG_XXX? "yes":"no"; ... does not compile. I will try to explain it better in the v3 commit log. Thanks! Regards,
You wanna address the printk maintainers, which I've added now. And Josh as he's interested in tiny linux. On Sat, Jan 27, 2024 at 11:00:26PM +0100, Yoann Congal wrote: > CONFIG_BASE_SMALL is currently a type int but is only used as a boolean > equivalent to !CONFIG_BASE_FULL. > > So, remove it entirely and move every usage to !CONFIG_BASE_FULL. Thanks for doing this. > In addition, recent kconfig changes (see the discussion in Closes: tag) > revealed that using: > config SOMETHING > default "some value" if X > does not work as expected if X is not of type bool. We should see if we can get kconfig to warn on this type of use. Also note that this was reported long ago by Vegard Nossum but he never really sent a fix [0] as I suggested, so thanks for doing this work. [0] https://lkml.iu.edu/hypermail/linux/kernel/2110.2/02402.html You should mention the one case which this patch fixes is: > CONFIG_BASE_SMALL was used that way in init/Kconfig: > config LOG_CPU_MAX_BUF_SHIFT > default 12 if !BASE_SMALL > default 0 if BASE_SMALL You should then mention this has been using 12 for a long time now for BASE_SMALL, and so this patch is a functional fix for those who used BASE_SMALL and wanted a smaller printk buffer contribtion per cpu. The contribution was only per CPU, and since BASE_SMALL systems likely don't have many CPUs the impact of this was relatively small, 4 KiB per CPU. This patch fixes that back down to 0 KiB per CPU. So in practice I'd imagine this fix is not critical to stable. However if folks do want it backported I'll note BAS_FULL has been around since we started with git on Linux so it should backport just fine. > diff --git a/init/Kconfig b/init/Kconfig > index 8d4e836e1b6b1..877b3f6f0e605 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -734,8 +734,8 @@ config LOG_CPU_MAX_BUF_SHIFT > int "CPU kernel log buffer size contribution (13 => 8 KB, 17 => 128KB)" > depends on SMP > range 0 21 > - default 12 if !BASE_SMALL > - default 0 if BASE_SMALL > + default 12 if BASE_FULL > + default 0 > depends on PRINTK > help > This option allows to increase the default ring buffer size This is the only functional change, it is a fix, so please address this in a separate small patch where you can go into all the above details about its issue and implications of fixing this as per my note above. Then you can address a separate patch which addresses the move of BASE_SMALL users to BASE_FULL so to remove BASE_SMALL, that is because that commit would have no functional changes and it makes it easier to review. Luis
On Tue, Jan 30, 2024 at 1:00 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > You wanna address the printk maintainers, which I've added now. > And Josh as he's interested in tiny linux. > > On Sat, Jan 27, 2024 at 11:00:26PM +0100, Yoann Congal wrote: > > CONFIG_BASE_SMALL is currently a type int but is only used as a boolean > > equivalent to !CONFIG_BASE_FULL. > > > > So, remove it entirely and move every usage to !CONFIG_BASE_FULL. > > Thanks for doing this. > > > In addition, recent kconfig changes (see the discussion in Closes: tag) > > revealed that using: > > config SOMETHING > > default "some value" if X > > does not work as expected if X is not of type bool. > > We should see if we can get kconfig to warn on this type of use. > Also note that this was reported long ago by Vegard Nossum but he > never really sent a fix [0] as I suggested, so thanks for doing this > work. > > [0] https://lkml.iu.edu/hypermail/linux/kernel/2110.2/02402.html It is good to know that this issue was already pointed out in the past. > You should mention the one case which this patch fixes is: > > > CONFIG_BASE_SMALL was used that way in init/Kconfig: > > config LOG_CPU_MAX_BUF_SHIFT > > default 12 if !BASE_SMALL > > default 0 if BASE_SMALL > > You should then mention this has been using 12 for a long time now > for BASE_SMALL, and so this patch is a functional fix for those > who used BASE_SMALL and wanted a smaller printk buffer contribtion per > cpu. The contribution was only per CPU, and since BASE_SMALL systems > likely don't have many CPUs the impact of this was relatively small, > 4 KiB per CPU. This patch fixes that back down to 0 KiB per CPU. > > So in practice I'd imagine this fix is not critical to stable. However > if folks do want it backported I'll note BAS_FULL has been around since > we started with git on Linux so it should backport just fine. > > > diff --git a/init/Kconfig b/init/Kconfig > > index 8d4e836e1b6b1..877b3f6f0e605 100644 > > --- a/init/Kconfig > > +++ b/init/Kconfig > > @@ -734,8 +734,8 @@ config LOG_CPU_MAX_BUF_SHIFT > > int "CPU kernel log buffer size contribution (13 => 8 KB, 17 => 128KB)" > > depends on SMP > > range 0 21 > > - default 12 if !BASE_SMALL > > - default 0 if BASE_SMALL > > + default 12 if BASE_FULL > > + default 0 > > depends on PRINTK > > help > > This option allows to increase the default ring buffer size > > This is the only functional change, it is a fix, so please address > this in a separate small patch where you can go into all the above > details about its issue and implications of fixing this as per my > note above. > > Then you can address a separate patch which addresses the move of > BASE_SMALL users to BASE_FULL so to remove BASE_SMALL, that is > because that commit would have no functional changes and it makes > it easier to review. > > Luis Splitting this into two patches sounds fine to me. Either is fine. Up to the printk maintainer. Anyway, this patch looks good: Reviewed-by: Masahiro Yamada <masahiroy@kernel.org>
diff --git a/arch/x86/include/asm/mpspec.h b/arch/x86/include/asm/mpspec.h index 4b0f98a8d338d..44307fb37fa25 100644 --- a/arch/x86/include/asm/mpspec.h +++ b/arch/x86/include/asm/mpspec.h @@ -15,7 +15,7 @@ extern int pic_mode; * Summit or generic (i.e. installer) kernels need lots of bus entries. * Maximum 256 PCI busses, plus 1 ISA bus in each of 4 cabinets. */ -#if CONFIG_BASE_SMALL == 0 +#ifdef CONFIG_BASE_FULL # define MAX_MP_BUSSES 260 #else # define MAX_MP_BUSSES 32 diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c index 67e2cb7c96eec..d0e4fcd1bd8b5 100644 --- a/drivers/tty/vt/vc_screen.c +++ b/drivers/tty/vt/vc_screen.c @@ -51,7 +51,7 @@ #include <asm/unaligned.h> #define HEADER_SIZE 4u -#define CON_BUF_SIZE (CONFIG_BASE_SMALL ? 256 : PAGE_SIZE) +#define CON_BUF_SIZE (IS_ENABLED(CONFIG_BASE_FULL) ? PAGE_SIZE : 256) /* * Our minor space: diff --git a/include/linux/threads.h b/include/linux/threads.h index c34173e6c5f18..f0f7a8aaba77d 100644 --- a/include/linux/threads.h +++ b/include/linux/threads.h @@ -25,14 +25,14 @@ /* * This controls the default maximum pid allocated to a process */ -#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x8000) +#define PID_MAX_DEFAULT (IS_ENABLED(CONFIG_BASE_FULL) ? 0x8000 : 0x1000) /* * A maximum of 4 million PIDs should be enough for a while. * [NOTE: PID/TIDs are limited to 2^30 ~= 1 billion, see FUTEX_TID_MASK.] */ -#define PID_MAX_LIMIT (CONFIG_BASE_SMALL ? PAGE_SIZE * 8 : \ - (sizeof(long) > 4 ? 4 * 1024 * 1024 : PID_MAX_DEFAULT)) +#define PID_MAX_LIMIT (IS_ENABLED(CONFIG_BASE_FULL) ? \ + (sizeof(long) > 4 ? 4 * 1024 * 1024 : PID_MAX_DEFAULT) : PAGE_SIZE * 8) /* * Define a minimum number of pids per cpu. Heuristically based diff --git a/include/linux/udp.h b/include/linux/udp.h index d04188714dca1..ca8a172169019 100644 --- a/include/linux/udp.h +++ b/include/linux/udp.h @@ -24,7 +24,7 @@ static inline struct udphdr *udp_hdr(const struct sk_buff *skb) } #define UDP_HTABLE_SIZE_MIN_PERNET 128 -#define UDP_HTABLE_SIZE_MIN (CONFIG_BASE_SMALL ? 128 : 256) +#define UDP_HTABLE_SIZE_MIN (IS_ENABLED(CONFIG_BASE_FULL) ? 256 : 128) #define UDP_HTABLE_SIZE_MAX 65536 static inline u32 udp_hashfn(const struct net *net, u32 num, u32 mask) diff --git a/include/linux/xarray.h b/include/linux/xarray.h index cb571dfcf4b16..7e00e71c2d266 100644 --- a/include/linux/xarray.h +++ b/include/linux/xarray.h @@ -1141,7 +1141,7 @@ static inline void xa_release(struct xarray *xa, unsigned long index) * doubled the number of slots per node, we'd get only 3 nodes per 4kB page. */ #ifndef XA_CHUNK_SHIFT -#define XA_CHUNK_SHIFT (CONFIG_BASE_SMALL ? 4 : 6) +#define XA_CHUNK_SHIFT (IS_ENABLED(CONFIG_BASE_FULL) ? 6 : 4) #endif #define XA_CHUNK_SIZE (1UL << XA_CHUNK_SHIFT) #define XA_CHUNK_MASK (XA_CHUNK_SIZE - 1) diff --git a/init/Kconfig b/init/Kconfig index 8d4e836e1b6b1..877b3f6f0e605 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -734,8 +734,8 @@ config LOG_CPU_MAX_BUF_SHIFT int "CPU kernel log buffer size contribution (13 => 8 KB, 17 => 128KB)" depends on SMP range 0 21 - default 12 if !BASE_SMALL - default 0 if BASE_SMALL + default 12 if BASE_FULL + default 0 depends on PRINTK help This option allows to increase the default ring buffer size @@ -1940,11 +1940,6 @@ config RT_MUTEXES bool default y if PREEMPT_RT -config BASE_SMALL - int - default 0 if BASE_FULL - default 1 if !BASE_FULL - config MODULE_SIG_FORMAT def_bool n select SYSTEM_DATA_VERIFICATION diff --git a/kernel/futex/core.c b/kernel/futex/core.c index e0e853412c158..8f85afef9d061 100644 --- a/kernel/futex/core.c +++ b/kernel/futex/core.c @@ -1141,10 +1141,10 @@ static int __init futex_init(void) unsigned int futex_shift; unsigned long i; -#if CONFIG_BASE_SMALL - futex_hashsize = 16; -#else +#ifdef CONFIG_BASE_FULL futex_hashsize = roundup_pow_of_two(256 * num_possible_cpus()); +#else + futex_hashsize = 16; #endif futex_queues = alloc_large_system_hash("futex", sizeof(*futex_queues), diff --git a/kernel/user.c b/kernel/user.c index 03cedc366dc9e..8f39fd0236fa0 100644 --- a/kernel/user.c +++ b/kernel/user.c @@ -88,7 +88,7 @@ EXPORT_SYMBOL_GPL(init_user_ns); * when changing user ID's (ie setuid() and friends). */ -#define UIDHASH_BITS (CONFIG_BASE_SMALL ? 3 : 7) +#define UIDHASH_BITS (IS_ENABLED(CONFIG_BASE_FULL) ? 7 : 3) #define UIDHASH_SZ (1 << UIDHASH_BITS) #define UIDHASH_MASK (UIDHASH_SZ - 1) #define __uidhashfn(uid) (((uid >> UIDHASH_BITS) + uid) & UIDHASH_MASK)
CONFIG_BASE_SMALL is currently a type int but is only used as a boolean equivalent to !CONFIG_BASE_FULL. So, remove it entirely and move every usage to !CONFIG_BASE_FULL. In addition, recent kconfig changes (see the discussion in Closes: tag) revealed that using: config SOMETHING default "some value" if X does not work as expected if X is not of type bool. CONFIG_BASE_SMALL was used that way in init/Kconfig: config LOG_CPU_MAX_BUF_SHIFT default 12 if !BASE_SMALL default 0 if BASE_SMALL Note: This changes CONFIG_LOG_CPU_MAX_BUF_SHIFT=12 to CONFIG_LOG_CPU_MAX_BUF_SHIFT=0 for some defconfigs, but that will not be a big impact due to this code in kernel/printk/printk.c: /* by default this will only continue through for large > 64 CPUs */ if (cpu_extra <= __LOG_BUF_LEN / 2) return; Signed-off-by: Yoann Congal <yoann.congal@smile.fr> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Closes: https://lore.kernel.org/all/CAMuHMdWm6u1wX7efZQf=2XUAHascps76YQac6rdnQGhc8nop_Q@mail.gmail.com/ Fixes: 4e244c10eab3 ("kconfig: remove unneeded symbol_empty variable") --- v1 patch was named "treewide: Change CONFIG_BASE_SMALL to bool type" https://lore.kernel.org/all/20240126163032.1613731-1-yoann.congal@smile.fr/ v1 -> v2: Applied Masahiro Yamada's comments (Thanks!): * Changed from "Change CONFIG_BASE_SMALL to type bool" to "Remove it and switch usage to !CONFIG_BASE_FULL" * Fixed "Fixes:" tag and reference to the mailing list thread. * Added a note about CONFIG_LOG_CPU_MAX_BUF_SHIFT changing. CC: Luis R. Rodriguez <mcgrof@kernel.org> CC: Thomas Gleixner <tglx@linutronix.de> CC: Ingo Molnar <mingo@redhat.com> CC: Borislav Petkov <bp@alien8.de> CC: Dave Hansen <dave.hansen@linux.intel.com> CC: "H. Peter Anvin" <hpa@zytor.com> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> CC: Jiri Slaby <jirislaby@kernel.org> CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com> CC: Matthew Wilcox <willy@infradead.org> CC: Peter Zijlstra <peterz@infradead.org> CC: Darren Hart <dvhart@infradead.org> CC: Davidlohr Bueso <dave@stgolabs.net> CC: "André Almeida" <andrealmeid@igalia.com> CC: Masahiro Yamada <masahiroy@kernel.org> CC: x86@kernel.org CC: linux-kernel@vger.kernel.org CC: linux-serial@vger.kernel.org CC: linux-fsdevel@vger.kernel.org CC: linux-kbuild@vger.kernel.org --- arch/x86/include/asm/mpspec.h | 2 +- drivers/tty/vt/vc_screen.c | 2 +- include/linux/threads.h | 6 +++--- include/linux/udp.h | 2 +- include/linux/xarray.h | 2 +- init/Kconfig | 9 ++------- kernel/futex/core.c | 6 +++--- kernel/user.c | 2 +- 8 files changed, 13 insertions(+), 18 deletions(-)