diff mbox series

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

Message ID 20230910082911.3378782-8-guoren@kernel.org (mailing list archive)
State New, archived
Headers show
Series riscv: Add Native/Paravirt qspinlock support | expand

Commit Message

Guo Ren Sept. 10, 2023, 8:29 a.m. UTC 
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(-)

Comments

Waiman Long Sept. 11, 2023, 3:22 p.m. UTC | #1 
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

Your patch series is still based on top of numa-aware qspinlock patchset 
which isn't upstream yet. Please rebase it without that as that will 
cause merge conflict during upstream merge.

Cheers,
Longman
Waiman Long Sept. 11, 2023, 3:34 p.m. UTC | #2 
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?

Cheers,
Longman
Guo Ren Sept. 12, 2023, 1:06 a.m. UTC | #3 
On Mon, Sep 11, 2023 at 11:22 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
>
> Your patch series is still based on top of numa-aware qspinlock patchset
> which isn't upstream yet. Please rebase it without that as that will
> cause merge conflict during upstream merge.
Okay, thx for pointing it out.

>
> Cheers,
> Longman
>
Guo Ren Sept. 12, 2023, 1:08 a.m. UTC | #4 
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;

>
> Cheers,
> Longman
>
Leonardo Bras Sept. 14, 2023, 7:32 a.m. UTC | #5 
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
>
Waiman Long Sept. 14, 2023, 5:23 p.m. UTC | #6 
On 9/14/23 03:32, Leonardo Bras wrote:
> 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;

I actually was thinking about the same suggestion to add static. Then I 
realized that the flag was also used in another file in a later patch. 
Of course, if it turns out that this flag is no longer needed outside of 
this file, it should be static.

Cheers,
Longman
diff mbox series

Patch

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;
+static int __init queued_spinlock_setup(char *p)
+{
+	enable_qspinlock_key = true;
+
+	return 0;
+}
+early_param("qspinlock", queued_spinlock_setup);
+
 DEFINE_STATIC_KEY_TRUE(combo_qspinlock_key);
 EXPORT_SYMBOL(combo_qspinlock_key);
 #endif
@@ -277,7 +286,12 @@  EXPORT_SYMBOL(combo_qspinlock_key);
 static void __init riscv_spinlock_init(void)
 {
 #ifdef CONFIG_RISCV_COMBO_SPINLOCKS
-	static_branch_disable(&combo_qspinlock_key);
+	if (!enable_qspinlock_key) {
+		static_branch_disable(&combo_qspinlock_key);
+		pr_info("Ticket spinlock: enabled\n");
+	} else {
+		pr_info("Queued spinlock: enabled\n");
+	}
 #endif
 }