Messages in this thread | | | Date | Wed, 13 Apr 2022 12:28:38 -0700 | Subject | Re: [PATCH RESEND 1/1] lib/Kconfig: remove DEBUG_PER_CPU_MAPS dependency for CPUMASK_OFFSTACK | From | Libo Chen <> |
| |
On 4/13/22 08:41, Randy Dunlap wrote: > > 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@oracle.com> >>>>>>>> --- >>>>>>>> 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... Masahiro explained that CPUMASK_OFFSTACK can only be configured by options not users if DEBUG_PER_CPU_MASK is not enabled. This doesn't seem to be what we want.
Libo
| |