diff mbox series

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

Message ID 20221116015244.10539-2-kernelfans@gmail.com (mailing list archive)
State Accepted
Commit 08d9c4f609afaf5c3032320d50f2bae4acdca314
Headers show
Series [1/2] srcu: Fix a misspelling in note | expand

Commit Message

Pingfan Liu Nov. 16, 2022, 1:52 a.m. UTC
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(-)

Comments

Frederic Weisbecker Nov. 16, 2022, 1:34 p.m. UTC | #1
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
>
Paul E. McKenney Nov. 16, 2022, 5:26 p.m. UTC | #2
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;
 }
 
 /*
Pingfan Liu Nov. 17, 2022, 2:34 a.m. UTC | #3
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;
>  }
>
>  /*
diff mbox series

Patch

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;
 }
 
 /*