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

[Yoann Congal <yoann.congal@smile.fr>]

Le 24/01/2024 à 09:09, Masahiro Yamada a écrit :
> On Wed, Jan 24, 2024 at 12:11 AM Yoann Congal <yoann.congal@smile.fr> 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@kernel.org> 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@kernel.org>
>>>
>>> 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.

Regards,
-- 
Yoann Congal
Smile ECS - Tech Expert