[PATCH 1/2] srcu: Fix a misspelling in note

[PATCH 1/2] srcu: Fix a misspelling in note

[Pingfan Liu]

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: rcu@vger.kernel.org
---
 include/linux/srcutree.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index e3014319d1ad..f52820e88018 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -48,7 +48,7 @@ struct srcu_data {
 struct srcu_node {
 	spinlock_t __private lock;
 	unsigned long srcu_have_cbs[4];		/* GP seq for children having CBs, but only */
-						/*  if greater than ->srcu_gq_seq. */
+						/*  if greater than ->srcu_gp_seq. */
 	unsigned long srcu_data_have_cbs[4];	/* Which srcu_data structs have CBs for given GP? */
 	unsigned long srcu_gp_seq_needed_exp;	/* Furthest future exp GP. */
 	struct srcu_node *srcu_parent;		/* Next up in tree. */
-- 
2.31.1

[PATCH 2/2] srcu: Fix the comparision in srcu_invl_snp_seq()

[Pingfan Liu]

A seq contains two fields: counter and state.

SRCU_SNP_INIT_SEQ is used as invalid initial value for srcu_node GP
sequence numbers. Hence srcu_invl_snp_seq() should compare both fields
of a seq.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: rcu@vger.kernel.org
---
 kernel/rcu/srcutree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 1c304fec89c0..725c82bb0a6a 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -154,7 +154,7 @@ static void init_srcu_struct_data(struct srcu_struct *ssp)
  */
 static inline bool srcu_invl_snp_seq(unsigned long s)
 {
-	return rcu_seq_state(s) == SRCU_SNP_INIT_SEQ;
+	return s == SRCU_SNP_INIT_SEQ;
 }
 
 /*
-- 
2.31.1

Re: [PATCH 2/2] srcu: Fix the comparision in srcu_invl_snp_seq()

[Frederic Weisbecker]

On Wed, Nov 16, 2022 at 09:52:44AM +0800, Pingfan Liu wrote:
> A seq contains two fields: counter and state.
> 
> SRCU_SNP_INIT_SEQ is used as invalid initial value for srcu_node GP
> sequence numbers. Hence srcu_invl_snp_seq() should compare both fields
> of a seq.
> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Frederic Weisbecker <frederic@kernel.org>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> To: rcu@vger.kernel.org
> ---
>  kernel/rcu/srcutree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 1c304fec89c0..725c82bb0a6a 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -154,7 +154,7 @@ static void init_srcu_struct_data(struct srcu_struct *ssp)
>   */
>  static inline bool srcu_invl_snp_seq(unsigned long s)
>  {
> -	return rcu_seq_state(s) == SRCU_SNP_INIT_SEQ;
> +	return s == SRCU_SNP_INIT_SEQ;

Doesn't hurt and makes it less confusing as it doesn't suggest anymore
there _might_ be a gp number behind.

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>


>  }
>  
>  /*
> -- 
> 2.31.1
> 

Re: [PATCH 2/2] srcu: Fix the comparision in srcu_invl_snp_seq()

[Paul E. McKenney]

On Wed, Nov 16, 2022 at 02:34:37PM +0100, Frederic Weisbecker wrote:
> On Wed, Nov 16, 2022 at 09:52:44AM +0800, Pingfan Liu wrote:
> > A seq contains two fields: counter and state.
> > 
> > SRCU_SNP_INIT_SEQ is used as invalid initial value for srcu_node GP
> > sequence numbers. Hence srcu_invl_snp_seq() should compare both fields
> > of a seq.
> > 
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Frederic Weisbecker <frederic@kernel.org>
> > Cc: Josh Triplett <josh@joshtriplett.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > To: rcu@vger.kernel.org
> > ---
> >  kernel/rcu/srcutree.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 1c304fec89c0..725c82bb0a6a 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -154,7 +154,7 @@ static void init_srcu_struct_data(struct srcu_struct *ssp)
> >   */
> >  static inline bool srcu_invl_snp_seq(unsigned long s)
> >  {
> > -	return rcu_seq_state(s) == SRCU_SNP_INIT_SEQ;
> > +	return s == SRCU_SNP_INIT_SEQ;
> 
> Doesn't hurt and makes it less confusing as it doesn't suggest anymore
> there _might_ be a gp number behind.
> 
> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Good catch, thank you both!

My first reaction was that this patch was wrong, so I recorded the
results of my investigation in the commit log.  Please check below in
case I messed something up.

							Thanx, Paul

------------------------------------------------------------------------

commit 08d9c4f609afaf5c3032320d50f2bae4acdca314
Author: Pingfan Liu <kernelfans@gmail.com>
Date:   Wed Nov 16 09:52:44 2022 +0800

    srcu: Fix the comparision in srcu_invl_snp_seq()
    
    A grace-period sequence number contains two fields: counter and
    state.  SRCU_SNP_INIT_SEQ provides a guaranteed invalid value for
    grace-period sequence numbers in newly allocated srcu_node structures'
    ->srcu_have_cbs[] and ->srcu_gp_seq_needed_exp fields.  The point of the
    comparison in srcu_invl_snp_seq() is not to detect invalid grace-period
    sequence numbers in general, but rather to detect a newly allocated
    srcu_node structure whose ->srcu_have_cbs[] and ->srcu_gp_seq_needed_exp
    fields need to be brought into line with the srcu_struct structure's
    ->srcu_gp_seq field.
    
    This commit therefore causes srcu_invl_snp_seq() to compare both fields
    of the specified grace-period sequence number.
    
    Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
    Cc: Lai Jiangshan <jiangshanlai@gmail.com>
    Cc: "Paul E. McKenney" <paulmck@kernel.org>
    Cc: Josh Triplett <josh@joshtriplett.org>
    Cc: Steven Rostedt <rostedt@goodmis.org>
    Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
    Cc: <rcu@vger.kernel.org>
    Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 16953784a0bdf..6af0312005801 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -154,7 +154,7 @@ static void init_srcu_struct_data(struct srcu_struct *ssp)
  */
 static inline bool srcu_invl_snp_seq(unsigned long s)
 {
-	return rcu_seq_state(s) == SRCU_SNP_INIT_SEQ;
+	return s == SRCU_SNP_INIT_SEQ;
 }
 
 /*

Re: [PATCH 2/2] srcu: Fix the comparision in srcu_invl_snp_seq()

[Pingfan Liu]

On Thu, Nov 17, 2022 at 1:26 AM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Wed, Nov 16, 2022 at 02:34:37PM +0100, Frederic Weisbecker wrote:
> > On Wed, Nov 16, 2022 at 09:52:44AM +0800, Pingfan Liu wrote:
> > > A seq contains two fields: counter and state.
> > >
> > > SRCU_SNP_INIT_SEQ is used as invalid initial value for srcu_node GP
> > > sequence numbers. Hence srcu_invl_snp_seq() should compare both fields
> > > of a seq.
> > >
> > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > Cc: Frederic Weisbecker <frederic@kernel.org>
> > > Cc: Josh Triplett <josh@joshtriplett.org>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > To: rcu@vger.kernel.org
> > > ---
> > >  kernel/rcu/srcutree.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index 1c304fec89c0..725c82bb0a6a 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -154,7 +154,7 @@ static void init_srcu_struct_data(struct srcu_struct *ssp)
> > >   */
> > >  static inline bool srcu_invl_snp_seq(unsigned long s)
> > >  {
> > > -   return rcu_seq_state(s) == SRCU_SNP_INIT_SEQ;
> > > +   return s == SRCU_SNP_INIT_SEQ;
> >
> > Doesn't hurt and makes it less confusing as it doesn't suggest anymore
> > there _might_ be a gp number behind.
> >
> > Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
>
> Good catch, thank you both!
>
> My first reaction was that this patch was wrong, so I recorded the
> results of my investigation in the commit log.  Please check below in
> case I messed something up.
>

It looks more clear now. Thanks for improving the log.

Thanks,

    Pingfan


>                                                         Thanx, Paul
>
> ------------------------------------------------------------------------
>
> commit 08d9c4f609afaf5c3032320d50f2bae4acdca314
> Author: Pingfan Liu <kernelfans@gmail.com>
> Date:   Wed Nov 16 09:52:44 2022 +0800
>
>     srcu: Fix the comparision in srcu_invl_snp_seq()
>
>     A grace-period sequence number contains two fields: counter and
>     state.  SRCU_SNP_INIT_SEQ provides a guaranteed invalid value for
>     grace-period sequence numbers in newly allocated srcu_node structures'
>     ->srcu_have_cbs[] and ->srcu_gp_seq_needed_exp fields.  The point of the
>     comparison in srcu_invl_snp_seq() is not to detect invalid grace-period
>     sequence numbers in general, but rather to detect a newly allocated
>     srcu_node structure whose ->srcu_have_cbs[] and ->srcu_gp_seq_needed_exp
>     fields need to be brought into line with the srcu_struct structure's
>     ->srcu_gp_seq field.
>
>     This commit therefore causes srcu_invl_snp_seq() to compare both fields
>     of the specified grace-period sequence number.
>
>     Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
>     Cc: Lai Jiangshan <jiangshanlai@gmail.com>
>     Cc: "Paul E. McKenney" <paulmck@kernel.org>
>     Cc: Josh Triplett <josh@joshtriplett.org>
>     Cc: Steven Rostedt <rostedt@goodmis.org>
>     Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>     Cc: <rcu@vger.kernel.org>
>     Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
>     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 16953784a0bdf..6af0312005801 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -154,7 +154,7 @@ static void init_srcu_struct_data(struct srcu_struct *ssp)
>   */
>  static inline bool srcu_invl_snp_seq(unsigned long s)
>  {
> -       return rcu_seq_state(s) == SRCU_SNP_INIT_SEQ;
> +       return s == SRCU_SNP_INIT_SEQ;
>  }
>
>  /*

Re: [PATCH 1/2] srcu: Fix a misspelling in note

[Mukesh Ojha]

Hi,

On 11/16/2022 7:22 AM, Pingfan Liu wrote:
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Frederic Weisbecker <frederic@kernel.org>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> To: rcu@vger.kernel.org

LGTM.
Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com>

-Mukesh

> ---
>   include/linux/srcutree.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index e3014319d1ad..f52820e88018 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -48,7 +48,7 @@ struct srcu_data {
>   struct srcu_node {
>   	spinlock_t __private lock;
>   	unsigned long srcu_have_cbs[4];		/* GP seq for children having CBs, but only */
> -						/*  if greater than ->srcu_gq_seq. */
> +						/*  if greater than ->srcu_gp_seq. */
>   	unsigned long srcu_data_have_cbs[4];	/* Which srcu_data structs have CBs for given GP? */
>   	unsigned long srcu_gp_seq_needed_exp;	/* Furthest future exp GP. */
>   	struct srcu_node *srcu_parent;		/* Next up in tree. */

Re: [PATCH 1/2] srcu: Fix a misspelling in note

[Frederic Weisbecker]

On Wed, Nov 16, 2022 at 09:52:43AM +0800, Pingfan Liu wrote:
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Frederic Weisbecker <frederic@kernel.org>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> To: rcu@vger.kernel.org
> ---
>  include/linux/srcutree.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index e3014319d1ad..f52820e88018 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -48,7 +48,7 @@ struct srcu_data {
>  struct srcu_node {
>  	spinlock_t __private lock;
>  	unsigned long srcu_have_cbs[4];		/* GP seq for children having CBs, but only */
> -						/*  if greater than ->srcu_gq_seq. */
> +						/*  if greater than ->srcu_gp_seq. */

Sharp eye ;-)

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>


>  	unsigned long srcu_data_have_cbs[4];	/* Which srcu_data structs have CBs for given GP? */
>  	unsigned long srcu_gp_seq_needed_exp;	/* Furthest future exp GP. */
>  	struct srcu_node *srcu_parent;		/* Next up in tree. */
> -- 
> 2.31.1
> 

Re: [PATCH 1/2] srcu: Fix a misspelling in note

[Paul E. McKenney]

On Wed, Nov 16, 2022 at 02:36:25PM +0100, Frederic Weisbecker wrote:
> On Wed, Nov 16, 2022 at 09:52:43AM +0800, Pingfan Liu wrote:
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Frederic Weisbecker <frederic@kernel.org>
> > Cc: Josh Triplett <josh@joshtriplett.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > To: rcu@vger.kernel.org
> > ---
> >  include/linux/srcutree.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> > index e3014319d1ad..f52820e88018 100644
> > --- a/include/linux/srcutree.h
> > +++ b/include/linux/srcutree.h
> > @@ -48,7 +48,7 @@ struct srcu_data {
> >  struct srcu_node {
> >  	spinlock_t __private lock;
> >  	unsigned long srcu_have_cbs[4];		/* GP seq for children having CBs, but only */
> > -						/*  if greater than ->srcu_gq_seq. */
> > +						/*  if greater than ->srcu_gp_seq. */
> 
> Sharp eye ;-)
> 
> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Applied with both Reviewed-by tags, thank you all!

							Thanx, Paul

> >  	unsigned long srcu_data_have_cbs[4];	/* Which srcu_data structs have CBs for given GP? */
> >  	unsigned long srcu_gp_seq_needed_exp;	/* Furthest future exp GP. */
> >  	struct srcu_node *srcu_parent;		/* Next up in tree. */
> > -- 
> > 2.31.1
> >