Re: [PATCH 1/2] kconfig: remove unneeded symbol_empty variable

[Masahiro Yamada]

On Wed, Jan 24, 2024 at 5:56 PM Yoann Congal <yoann.congal@xxxxxxxx> wrote:
>
>
>
> Le 24/01/2024 à 09:09, Masahiro Yamada a écrit :
> > On Wed, Jan 24, 2024 at 12:11 AM Yoann Congal <yoann.congal@xxxxxxxx> wrote:
> >>
> >> Le 23/01/2024 à 13:54, Geert Uytterhoeven a écrit :
> >>> Hi Yamada-san,
> >>
> >> Hello,
> >>
> >>> On Sat, Nov 25, 2023 at 5:36 PM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
> >>>> This is used only for initializing other variables.
> >>>>
> >>>> Use the empty string "".
> >>>>
> >>>> Please note newval.tri is unused for S_INT/HEX/STRING.
> >>>>
> >>>> Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> >>>
> >>> Thanks for your patch, which is now commit 4e244c10eab345a7
> >>> ("kconfig: remove unneeded symbol_empty variable") in v6.8-rc1.
> >>>
> >>> When running "make <foo>_defconfig" with <foo>_defconfig an SMP
> >>> defconfig without explicit configuration of CONFIG_LOG_CPU_MAX_BUF_SHIFT,
> >>> the aforementioned commit causes a change in the generated .config:
> >>>
> >>> -CONFIG_LOG_CPU_MAX_BUF_SHIFT=12
> >>> +CONFIG_LOG_CPU_MAX_BUF_SHIFT=0
> >>>
> >>> It looks like CONFIG_BASE_SMALL=0 is treated as a string instead of
> >>> the integer number zero?
> >>>
> >>> init/Kconfig=config LOG_CPU_MAX_BUF_SHIFT
> >>> init/Kconfig-   int "CPU kernel log buffer size contribution (13 => 8
> >>> KB, 17 => 128KB)"
> >>> init/Kconfig-   depends on SMP
> >>> init/Kconfig-   range 0 21
> >>> init/Kconfig:   default 12 if !BASE_SMALL
> >>> init/Kconfig:   default 0 if BASE_SMALL
> >>>
> >>> Note that reverting 4e244c10eab345a7 is not sufficient to fix the issue.
> >>> Also reverting commit 6262afa10ef7cc8f ("kconfig: default to zero if
> >>> int/hex symbol lacks default property") does fix it.
> >>
> >> (Since I'd really like 6262afa10ef7cc8f ("kconfig: default to zero if int/hex symbol lacks default property") to stay, allow me to try to help)
> >>
> >> The problem is quite easy to reproduce:
> >>   $ make x86_64_defconfig
> >>   $ grep 'LOG_CPU_MAX_BUF_SHIFT\|BASE_SMALL\|BASE_FULL' .config
> >>   CONFIG_LOG_CPU_MAX_BUF_SHIFT=0
> >>   CONFIG_BASE_FULL=y
> >>   CONFIG_BASE_SMALL=0
> >> Here, CONFIG_LOG_CPU_MAX_BUF_SHIFT should be 12 not 0.
> >
> >
> >
> > I could not produce it in this way.
> > I ran the same commands as yours.
> >
> > CONFIG_LOG_CPU_MAX_BUF_SHIFT=12 for me.
> >
> >
> >
> > masahiro@zoe:~/ref/linux(master)$ git describe
> > v6.8-rc1-29-g615d30064886
> > masahiro@zoe:~/ref/linux(master)$ git diff
> > masahiro@zoe:~/ref/linux(master)$ make  x86_64_defconfig
> > #
> > # No change to .config
> > #
>
> You already had a .config with the correct value of LOG_CPU_MAX_BUF_SHIFT (Maybe?)
>
> > masahiro@zoe:~/ref/linux(master)$ grep
> > 'LOG_CPU_MAX_BUF_SHIFT\|BASE_SMALL\|BASE_FULL' .config
> > CONFIG_LOG_CPU_MAX_BUF_SHIFT=12
> > CONFIG_BASE_FULL=y
> > CONFIG_BASE_SMALL=0
>
> Try to remove the existing .config:
>
>    $ git describe
>   v6.8-rc1
>    $ git diff
>    $ rm .config -f
>    $ make  x86_64_defconfig
>   #
>   # configuration written to .config
>   #
>    $ grep 'LOG_CPU_MAX_BUF_SHIFT\|BASE_SMALL\|BASE_FULL' .config
>   CONFIG_LOG_CPU_MAX_BUF_SHIFT=0
>   CONFIG_BASE_FULL=y
>   CONFIG_BASE_SMALL=0
>
> >>
> >> For what it is worth, CONFIG_BASE_SMALL is defined as an int but is only used as a bool :
> >>    $ git grep BASE_SMALL
> >>   arch/x86/include/asm/mpspec.h:#if CONFIG_BASE_SMALL == 0
> >>   drivers/tty/vt/vc_screen.c:#define CON_BUF_SIZE (CONFIG_BASE_SMALL ? 256 : PAGE_SIZE)
> >>   include/linux/threads.h:#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x8000)
> >>   include/linux/threads.h:#define PID_MAX_LIMIT (CONFIG_BASE_SMALL ? PAGE_SIZE * 8 : \
> >>   include/linux/udp.h:#define UDP_HTABLE_SIZE_MIN         (CONFIG_BASE_SMALL ? 128 : 256)
> >>   include/linux/xarray.h:#define XA_CHUNK_SHIFT           (CONFIG_BASE_SMALL ? 4 : 6)
> >>   init/Kconfig:   default 12 if !BASE_SMALL
> >>   init/Kconfig:   default 0 if BASE_SMALL
> >>   init/Kconfig:config BASE_SMALL
> >>   kernel/futex/core.c:#if CONFIG_BASE_SMALL
> >>   kernel/user.c:#define UIDHASH_BITS      (CONFIG_BASE_SMALL ? 3 : 7)
> >>
> >> Maybe we should change CONFIG_BASE_SMALL to the bool type?
>
> My first test shows that switching CONFIG_BASE_SMALL to bool type does fix the LOG_CPU_MAX_BUF_SHIFT default value.
>
> >> I'll poke around to see if I can understand why a int="0" is true for kconfig.
>
> Here's what I understood:
> To get the default value of LOG_CPU_MAX_BUF_SHIFT, kconfig calls sym_get_default_prop(LOG_CPU_MAX_BUF_SHIFT)
> -> expr_calc_value("BASE_SMALL" as an expr)
> -> sym_calc_value(BASE_SMALL as a symbol) and returns sym->curr.tri
>
> But, if I understood correctly, sym_calc_value() does not set sym->curr.tri in case of a int type config.


Right.

The following will restore the original behavior.


--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -349,12 +349,15 @@ void sym_calc_value(struct symbol *sym)
        switch (sym->type) {
        case S_INT:
                newval.val = "0";
+               newval.tri = no;
                break;
        case S_HEX:
                newval.val = "0x0";
+               newval.tri = no;
                break;
        case S_STRING:
                newval.val = "";
+               newval.tri = no;
                break;
        case S_BOOLEAN:
        case S_TRISTATE:





But, I do not think that is the right thing to do.


Presumably, turning CONFIG_BASE_SMALL is correct.





>
> Regards,
> --
> Yoann Congal
> Smile ECS - Tech Expert



-- 
Best Regards
Masahiro Yamada