Re: [PATCH RESEND 1/1] lib/Kconfig: remove DEBUG_PER_CPU_MAPS dependency for CPUMASK_OFFSTACK

[Randy Dunlap]

On 4/12/22 23:56, Libo Chen wrote:
> Hi Randy
> 
> On 4/12/22 22:54, Randy Dunlap wrote:
>> Hi Libo,
>>
>> On 4/12/22 19:34, Libo Chen wrote:
>>>
>>> On 4/12/22 19:13, Randy Dunlap wrote:
>>>> Hi,
>>>>
>>>> On 4/12/22 18:35, Libo Chen wrote:
>>>>> Hi Randy,
>>>>>
>>>>> On 4/12/22 17:18, Randy Dunlap wrote:
>>>>>> Hi--
>>>>>>
>>>>>> On 4/12/22 16:15, Libo Chen wrote:
>>>>>>> Forcing CPUMASK_OFFSTACK to be conditoned on DEBUG_PER_CPU_MAPS doesn't
>>>>>>> make a lot of sense nowaday. Even the original patch dating back to 2008,
>>>>>>> aab46da0520a ("cpumask: Add CONFIG_CPUMASK_OFFSTACK") didn't give any
>>>>>>> rationale for such dependency.
>>>>>>>
>>>>>>> Nowhere in the code supports the presumption that DEBUG_PER_CPU_MAPS is
>>>>>>> necessary for CONFIG_CPUMASK_OFFSTACK. Make no mistake, it's good to
>>>>>>> have DEBUG_PER_CPU_MAPS for debugging purpose or precaution, but it's
>>>>>>> simply not a hard requirement for CPUMASK_OFFSTACK. Moreover, x86 Kconfig
>>>>>>> already can set CPUMASK_OFFSTACK=y without DEBUG_PER_CPU_MAPS=y.
>>>>>>> There is no reason other architectures cannot given the fact that they
>>>>>>> have even fewer, if any, arch-specific CONFIG_DEBUG_PER_CPU_MAPS code than
>>>>>>> x86.
>>>>>>>
>>>>>>> Signed-off-by: Libo Chen <libo.chen@xxxxxxxxxx>
>>>>>>> ---
>>>>>>>     lib/Kconfig | 2 +-
>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/lib/Kconfig b/lib/Kconfig
>>>>>>> index 087e06b4cdfd..7209039dfb59 100644
>>>>>>> --- a/lib/Kconfig
>>>>>>> +++ b/lib/Kconfig
>>>>>>> @@ -511,7 +511,7 @@ config CHECK_SIGNATURE
>>>>>>>         bool
>>>>>>>       config CPUMASK_OFFSTACK
>>>>>>> -    bool "Force CPU masks off stack" if DEBUG_PER_CPU_MAPS
>>>>>> This "if" dependency only controls whether the Kconfig symbol's prompt is
>>>>>> displayed (presented) in kconfig tools. Removing it makes the prompt always
>>>>>> be displayed.
>>>>>>
>>>>>> Any architecture could select (should be able to) CPUMASK_OFFSTACK independently
>>>>>> of DEBUG_PER_CPU_MAPS.
>>>>> Do you mean changing arch/xxxx/Kconfig to select CPUMASK_OFFSTACK under some config xxx? That will work but it requires code changes for each architecture.
>>>>> But if you are talking about setting CONFIG_CPUMASK_OFFSTACK=y without CONFIG_DEBUG_PER_CPU_MAPS directly in config file, I have tried, it doesn't work.
>>>> I'm just talking about the Kconfig change below.  Not talking about whatever else
>>>> it might require per architecture.
>>>>
>>>> But you say you have tried that and it doesn't work. What part of it doesn't work?
>>>> The Kconfig part or some code execution?
>>> oh the Kconfig part. For example, make olddefconfig on a config file with CPUMASK_OFFSTACK=y only turns off CPUMASK_OFFSTACK unless I explicitly set DEBUG_PER_CPU_MAPS=y
>> I can enable CPUMASK_OFFSTACK for arm64 without having DEBUG_PER_CPU_MAPS enabled.
>> (with a patch, of course.)
>> It builds OK. I don't know if it will run OK.
> 
> I am a little confused, did you succeed with your patch (replacing "if" with "depends on") or my patch (removing "if")? Because I definitely cannot enable CPUMASK_OFFSTACK for arm64 without DEBUG_PER_CPUMAPS enabled using your change.

This patch builds cleanly for me:

---
 arch/arm64/Kconfig |    1 +
 lib/Kconfig        |    2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -511,7 +511,7 @@ config CHECK_SIGNATURE
 	bool
 
 config CPUMASK_OFFSTACK
-	bool "Force CPU masks off stack" if DEBUG_PER_CPU_MAPS
+	bool "Force CPU masks off stack"
 	help
 	  Use dynamic allocation for cpumask_var_t, instead of putting
 	  them on the stack.  This is a bit more expensive, but avoids
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -316,6 +316,7 @@ config ARCH_MHP_MEMMAP_ON_MEMORY_ENABLE
 
 config SMP
 	def_bool y
+	select CPUMASK_OFFSTACK
 
 config KERNEL_MODE_NEON
 	def_bool y

along with:
# CONFIG_DEBUG_PER_CPU_MAPS is not set


>> I think that you are arguing for a patch like this:
> 
> I am actually arguing for the opposite, I don't think CPUMASK_OFFSTACK should require DEBUG_PER_CPU_MAPS. They should be separate and independent to each other. So removing "if ..." should be enough in my opinion.

I agree.

>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -511,7 +511,8 @@ config CHECK_SIGNATURE
>>       bool
>>     config CPUMASK_OFFSTACK
>> -    bool "Force CPU masks off stack" if DEBUG_PER_CPU_MAPS
>> +    bool "Force CPU masks off stack"
>> +    depends on DEBUG_PER_CPU_MAPS
> 
> This forces every arch to enable DEBUG_PER_CPU_MAPS if they want to enable CPUMASK_OFFSTACK, it's even stronger than "if". My whole argument is CPUMASK_OFFSTACK should be enable/disabled independent of DEBUG_PER_CPU_MASK
>>       help
>>         Use dynamic allocation for cpumask_var_t, instead of putting
>>         them on the stack.  This is a bit more expensive, but avoids
>>
>>
>> As I said earlier, the "if" on the "bool" line just controls the prompt message.
>> This patch make CPUMASK_OFFSTACK require DEBUG_PER_CPU_MAPS -- which might be overkill.
>>
> 
> Okay I understand now "if" on the "boot" is not a dependency and it only controls the prompt message, then the question is why we cannot enable CPUMASK_OFFSTACK without DEBUG_PER_CPU_MAPS if it only controls prompt message? Is it not the behavior we expect?

Yes, it is. I don't know that the problem is...

-- 
~Randy