[tip/core/rcu,06/30] rcu: Add WRITE_ONCE to rcu_node ->exp_seq_rq store
diff mbox series

Message ID 20200214235607.13749-6-paulmck@kernel.org
State In Next
Commit ccf2ded1f07417f40e49ec66ccc57f860f7144c1
Headers show
Series
  • Miscellaneous fixes for v5.7
Related show

Commit Message

Paul E. McKenney Feb. 14, 2020, 11:55 p.m. UTC
From: "Paul E. McKenney" <paulmck@kernel.org>

The rcu_node structure's ->exp_seq_rq field is read locklessly, so
this commit adds the WRITE_ONCE() to a load in order to provide proper
documentation and READ_ONCE()/WRITE_ONCE() pairing.

This data race was reported by KCSAN.  Not appropriate for backporting
due to failure being unlikely.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/tree_exp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Steven Rostedt Feb. 15, 2020, 3:47 a.m. UTC | #1
On Fri, 14 Feb 2020 15:55:43 -0800
paulmck@kernel.org wrote:

> From: "Paul E. McKenney" <paulmck@kernel.org>
> 
> The rcu_node structure's ->exp_seq_rq field is read locklessly, so
> this commit adds the WRITE_ONCE() to a load in order to provide proper
> documentation and READ_ONCE()/WRITE_ONCE() pairing.
> 
> This data race was reported by KCSAN.  Not appropriate for backporting
> due to failure being unlikely.
> 
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> ---
>  kernel/rcu/tree_exp.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index d7e0484..85b009e 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -314,7 +314,7 @@ static bool exp_funnel_lock(unsigned long s)
>  				   sync_exp_work_done(s));
>  			return true;
>  		}
> -		rnp->exp_seq_rq = s; /* Followers can wait on us. */
> +		WRITE_ONCE(rnp->exp_seq_rq, s); /* Followers can wait on us. */

Didn't Linus say this is basically bogus?

Perhaps just using it as documenting that it's read locklessly, but is
it really needed?

-- Steve



>  		spin_unlock(&rnp->exp_lock);
>  		trace_rcu_exp_funnel_lock(rcu_state.name, rnp->level,
>  					  rnp->grplo, rnp->grphi, TPS("nxtlvl"));
Paul E. McKenney Feb. 15, 2020, 10:58 a.m. UTC | #2
On Fri, Feb 14, 2020 at 10:47:43PM -0500, Steven Rostedt wrote:
> On Fri, 14 Feb 2020 15:55:43 -0800
> paulmck@kernel.org wrote:
> 
> > From: "Paul E. McKenney" <paulmck@kernel.org>
> > 
> > The rcu_node structure's ->exp_seq_rq field is read locklessly, so
> > this commit adds the WRITE_ONCE() to a load in order to provide proper
> > documentation and READ_ONCE()/WRITE_ONCE() pairing.
> > 
> > This data race was reported by KCSAN.  Not appropriate for backporting
> > due to failure being unlikely.
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > ---
> >  kernel/rcu/tree_exp.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > index d7e0484..85b009e 100644
> > --- a/kernel/rcu/tree_exp.h
> > +++ b/kernel/rcu/tree_exp.h
> > @@ -314,7 +314,7 @@ static bool exp_funnel_lock(unsigned long s)
> >  				   sync_exp_work_done(s));
> >  			return true;
> >  		}
> > -		rnp->exp_seq_rq = s; /* Followers can wait on us. */
> > +		WRITE_ONCE(rnp->exp_seq_rq, s); /* Followers can wait on us. */
> 
> Didn't Linus say this is basically bogus?
> 
> Perhaps just using it as documenting that it's read locklessly, but is
> it really needed?

Yes, Linus explicitly stated that WRITE_ONCE() is not required in
this case, but he also said that he was OK with it being there for
documentation purposes.

And within RCU, I -do- need it because I absolutely need to see if a
given patch introduced new KCSAN reports.  So I need it for the same
reason that I need the build to proceed without warnings.

Others who are working with less concurrency-intensive code might quite
reasonably make other choices, of course.  And my setting certain KCSAN
config options in my own builds doesn't inconvenience them, so we should
all be happy, right?  :-)

							Thanx, Paul

> -- Steve
> 
> 
> 
> >  		spin_unlock(&rnp->exp_lock);
> >  		trace_rcu_exp_funnel_lock(rcu_state.name, rnp->level,
> >  					  rnp->grplo, rnp->grphi, TPS("nxtlvl"));
>
Joel Fernandes Feb. 17, 2020, 9:11 p.m. UTC | #3
On Sat, Feb 15, 2020 at 02:58:03AM -0800, Paul E. McKenney wrote:
> On Fri, Feb 14, 2020 at 10:47:43PM -0500, Steven Rostedt wrote:
> > On Fri, 14 Feb 2020 15:55:43 -0800
> > paulmck@kernel.org wrote:
> > 
> > > From: "Paul E. McKenney" <paulmck@kernel.org>
> > > 
> > > The rcu_node structure's ->exp_seq_rq field is read locklessly, so
> > > this commit adds the WRITE_ONCE() to a load in order to provide proper
> > > documentation and READ_ONCE()/WRITE_ONCE() pairing.
> > > 
> > > This data race was reported by KCSAN.  Not appropriate for backporting
> > > due to failure being unlikely.
> > > 
> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > ---
> > >  kernel/rcu/tree_exp.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > > index d7e0484..85b009e 100644
> > > --- a/kernel/rcu/tree_exp.h
> > > +++ b/kernel/rcu/tree_exp.h
> > > @@ -314,7 +314,7 @@ static bool exp_funnel_lock(unsigned long s)
> > >  				   sync_exp_work_done(s));
> > >  			return true;
> > >  		}
> > > -		rnp->exp_seq_rq = s; /* Followers can wait on us. */
> > > +		WRITE_ONCE(rnp->exp_seq_rq, s); /* Followers can wait on us. */
> > 
> > Didn't Linus say this is basically bogus?
> > 
> > Perhaps just using it as documenting that it's read locklessly, but is
> > it really needed?
> 
> Yes, Linus explicitly stated that WRITE_ONCE() is not required in
> this case, but he also said that he was OK with it being there for
> documentation purposes.

Just to add, PeterZ does approve of WRITE_ONCE() to prevent store-tearing
where applicable.

And I have reproduced Will's example [1] with the arm64 Clang compiler
shipping with the latest Android NDK just now. It does break up stores when
writing zeroes to 64-bit valyes, this is a real problem which WRITE_ONCE()
resolves. And I've verified GCC on arm64 does break up 64-bit immediate value
writes without WRITE_ONCE().

thanks,

 - Joel

[1] https://lore.kernel.org/lkml/20190821103200.kpufwtviqhpbuv2n@willie-the-truck/


> And within RCU, I -do- need it because I absolutely need to see if a
> given patch introduced new KCSAN reports.  So I need it for the same
> reason that I need the build to proceed without warnings.
> 
> Others who are working with less concurrency-intensive code might quite
> reasonably make other choices, of course.  And my setting certain KCSAN
> config options in my own builds doesn't inconvenience them, so we should
> all be happy, right?  :-)
> 
> 							Thanx, Paul
> 
> > -- Steve
> > 
> > 
> > 
> > >  		spin_unlock(&rnp->exp_lock);
> > >  		trace_rcu_exp_funnel_lock(rcu_state.name, rnp->level,
> > >  					  rnp->grplo, rnp->grphi, TPS("nxtlvl"));
> >
Paul E. McKenney Feb. 17, 2020, 9:36 p.m. UTC | #4
On Mon, Feb 17, 2020 at 04:11:35PM -0500, Joel Fernandes wrote:
> On Sat, Feb 15, 2020 at 02:58:03AM -0800, Paul E. McKenney wrote:
> > On Fri, Feb 14, 2020 at 10:47:43PM -0500, Steven Rostedt wrote:
> > > On Fri, 14 Feb 2020 15:55:43 -0800
> > > paulmck@kernel.org wrote:
> > > 
> > > > From: "Paul E. McKenney" <paulmck@kernel.org>
> > > > 
> > > > The rcu_node structure's ->exp_seq_rq field is read locklessly, so
> > > > this commit adds the WRITE_ONCE() to a load in order to provide proper
> > > > documentation and READ_ONCE()/WRITE_ONCE() pairing.
> > > > 
> > > > This data race was reported by KCSAN.  Not appropriate for backporting
> > > > due to failure being unlikely.
> > > > 
> > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > ---
> > > >  kernel/rcu/tree_exp.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > > > index d7e0484..85b009e 100644
> > > > --- a/kernel/rcu/tree_exp.h
> > > > +++ b/kernel/rcu/tree_exp.h
> > > > @@ -314,7 +314,7 @@ static bool exp_funnel_lock(unsigned long s)
> > > >  				   sync_exp_work_done(s));
> > > >  			return true;
> > > >  		}
> > > > -		rnp->exp_seq_rq = s; /* Followers can wait on us. */
> > > > +		WRITE_ONCE(rnp->exp_seq_rq, s); /* Followers can wait on us. */
> > > 
> > > Didn't Linus say this is basically bogus?
> > > 
> > > Perhaps just using it as documenting that it's read locklessly, but is
> > > it really needed?
> > 
> > Yes, Linus explicitly stated that WRITE_ONCE() is not required in
> > this case, but he also said that he was OK with it being there for
> > documentation purposes.
> 
> Just to add, PeterZ does approve of WRITE_ONCE() to prevent store-tearing
> where applicable.
> 
> And I have reproduced Will's example [1] with the arm64 Clang compiler
> shipping with the latest Android NDK just now. It does break up stores when
> writing zeroes to 64-bit valyes, this is a real problem which WRITE_ONCE()
> resolves. And I've verified GCC on arm64 does break up 64-bit immediate value
> writes without WRITE_ONCE().

That does sounds a bit on the required side!  ;-)

							Thanx, Paul

> thanks,
> 
>  - Joel
> 
> [1] https://lore.kernel.org/lkml/20190821103200.kpufwtviqhpbuv2n@willie-the-truck/
> 
> 
> > And within RCU, I -do- need it because I absolutely need to see if a
> > given patch introduced new KCSAN reports.  So I need it for the same
> > reason that I need the build to proceed without warnings.
> > 
> > Others who are working with less concurrency-intensive code might quite
> > reasonably make other choices, of course.  And my setting certain KCSAN
> > config options in my own builds doesn't inconvenience them, so we should
> > all be happy, right?  :-)
> > 
> > 							Thanx, Paul
> > 
> > > -- Steve
> > > 
> > > 
> > > 
> > > >  		spin_unlock(&rnp->exp_lock);
> > > >  		trace_rcu_exp_funnel_lock(rcu_state.name, rnp->level,
> > > >  					  rnp->grplo, rnp->grphi, TPS("nxtlvl"));
> > >

Patch
diff mbox series

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index d7e0484..85b009e 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -314,7 +314,7 @@  static bool exp_funnel_lock(unsigned long s)
 				   sync_exp_work_done(s));
 			return true;
 		}
-		rnp->exp_seq_rq = s; /* Followers can wait on us. */
+		WRITE_ONCE(rnp->exp_seq_rq, s); /* Followers can wait on us. */
 		spin_unlock(&rnp->exp_lock);
 		trace_rcu_exp_funnel_lock(rcu_state.name, rnp->level,
 					  rnp->grplo, rnp->grphi, TPS("nxtlvl"));