Re: [PATCH V11 07/17] riscv: qspinlock: Introduce qspinlock param for command line

[Leonardo Bras <leobras@redhat.com>]

On Tue, Sep 12, 2023 at 09:08:34AM +0800, Guo Ren wrote:
> On Mon, Sep 11, 2023 at 11:34 PM Waiman Long <longman@redhat.com> wrote:
> >
> > On 9/10/23 04:29, guoren@kernel.org wrote:
> > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > Allow cmdline to force the kernel to use queued_spinlock when
> > > CONFIG_RISCV_COMBO_SPINLOCKS=y.
> > >
> > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > ---
> > >   Documentation/admin-guide/kernel-parameters.txt |  2 ++
> > >   arch/riscv/kernel/setup.c                       | 16 +++++++++++++++-
> > >   2 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index 7dfb540c4f6c..61cacb8dfd0e 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -4693,6 +4693,8 @@
> > >                       [KNL] Number of legacy pty's. Overwrites compiled-in
> > >                       default number.
> > >
> > > +     qspinlock       [RISCV] Force to use qspinlock or auto-detect spinlock.
> > > +
> > >       qspinlock.numa_spinlock_threshold_ns=   [NUMA, PV_OPS]
> > >                       Set the time threshold in nanoseconds for the
> > >                       number of intra-node lock hand-offs before the
> > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > > index a447cf360a18..0f084f037651 100644
> > > --- a/arch/riscv/kernel/setup.c
> > > +++ b/arch/riscv/kernel/setup.c
> > > @@ -270,6 +270,15 @@ static void __init parse_dtb(void)
> > >   }
> > >
> > >   #ifdef CONFIG_RISCV_COMBO_SPINLOCKS
> > > +bool enable_qspinlock_key = false;
> >
> > You can use __ro_after_init qualifier for enable_qspinlock_key. BTW,
> > this is not a static key, just a simple flag. So what is the point of
> > the _key suffix?
> Okay, I would change it to:
> bool enable_qspinlock_flag __ro_after_init = false;

IIUC, this bool / flag is used in a single file, so it makes sense for it 
to be static. Being static means it does not need to be initialized to 
false, as it's standard to zero-fill this areas.

Also, since it's a bool, it does not need to be called _flag.

I would go with:

static bool enable_qspinlock __ro_after_init;


> 
> >
> > Cheers,
> > Longman
> >
> 
> 
> -- 
> Best Regards
>  Guo Ren
>