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

[Libo Chen]

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.
> 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.
> --- 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?

Libo

> > Libo
> > > I'll test the Kconfig part of it later (in a few hours).
> > >
> > > > Libo
> > > > > Is there another problem here?
> > > > >
> > > > > > +    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