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 <[email protected]>
Reported-by: Geert Uytterhoeven <[email protected]>
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/[email protected]/
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 <[email protected]>
CC: Thomas Gleixner <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Borislav Petkov <[email protected]>
CC: Dave Hansen <[email protected]>
CC: "H. Peter Anvin" <[email protected]>
CC: Greg Kroah-Hartman <[email protected]>
CC: Jiri Slaby <[email protected]>
CC: Willem de Bruijn <[email protected]>
CC: Matthew Wilcox <[email protected]>
CC: Peter Zijlstra <[email protected]>
CC: Darren Hart <[email protected]>
CC: Davidlohr Bueso <[email protected]>
CC: "André Almeida" <[email protected]>
CC: Masahiro Yamada <[email protected]>
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
---
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(-)
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)
--
2.39.2
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 <[email protected]>
> Reported-by: Geert Uytterhoeven <[email protected]>
> 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/[email protected]/
>
> 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,
--
js
suse labs
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 <[email protected]>
>> Reported-by: Geert Uytterhoeven <[email protected]>
>> 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/[email protected]/
>>
>> 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,
--
Yoann Congal
Smile ECS - Tech Expert
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 <[email protected]> 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 <[email protected]>
--
Best Regards
Masahiro Yamada
On 2024-01-29, Luis Chamberlain <[email protected]> wrote:
> 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.
For printk this will mean that BASE_SMALL systems were probably
previously allocating/using the dynamic ringbuffer and now they will
just continue to use the static ringbuffer. Which is fine and saves
memory (as it should).
Reviewed-by: John Ogness <[email protected]>