[tip/core/rcu,4/4] srcu: Add READ_ONCE() to srcu_struct ->srcu_gp_seq load
diff mbox series

Message ID 20200215002932.15976-4-paulmck@kernel.org
State In Next
Commit f1bf9c5daaa96bf360c48df5ff8e7a23ac4844f8
Headers show
Series
  • SRCU updates for v5.7
Related show

Commit Message

Paul E. McKenney Feb. 15, 2020, 12:29 a.m. UTC
From: "Paul E. McKenney" <paulmck@kernel.org>

The load of the srcu_struct structure's ->srcu_gp_seq field in
srcu_funnel_gp_start() is lockless, so this commit adds the requisite
READ_ONCE().

This data race was reported by KCSAN.

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

Comments

Peter Zijlstra Feb. 17, 2020, 12:45 p.m. UTC | #1
On Fri, Feb 14, 2020 at 04:29:32PM -0800, paulmck@kernel.org wrote:
> From: "Paul E. McKenney" <paulmck@kernel.org>
> 
> The load of the srcu_struct structure's ->srcu_gp_seq field in
> srcu_funnel_gp_start() is lockless, so this commit adds the requisite
> READ_ONCE().
> 
> This data race was reported by KCSAN.

But is there in actual fact a data-race? AFAICT this code was just fine.

> Signed-off-by: Paul E. McKenney <paulmck@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 119a373..90ab475 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -678,7 +678,7 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
>  
>  	/* If grace period not already done and none in progress, start it. */
>  	if (!rcu_seq_done(&ssp->srcu_gp_seq, s) &&
> -	    rcu_seq_state(ssp->srcu_gp_seq) == SRCU_STATE_IDLE) {
> +	    rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq)) == SRCU_STATE_IDLE) {
>  		WARN_ON_ONCE(ULONG_CMP_GE(ssp->srcu_gp_seq, ssp->srcu_gp_seq_needed));
>  		srcu_gp_start(ssp);
>  		if (likely(srcu_init_done))
> -- 
> 2.9.5
>
Paul E. McKenney Feb. 17, 2020, 6:32 p.m. UTC | #2
On Mon, Feb 17, 2020 at 01:45:07PM +0100, Peter Zijlstra wrote:
> On Fri, Feb 14, 2020 at 04:29:32PM -0800, paulmck@kernel.org wrote:
> > From: "Paul E. McKenney" <paulmck@kernel.org>
> > 
> > The load of the srcu_struct structure's ->srcu_gp_seq field in
> > srcu_funnel_gp_start() is lockless, so this commit adds the requisite
> > READ_ONCE().
> > 
> > This data race was reported by KCSAN.
> 
> But is there in actual fact a data-race? AFAICT this code was just fine.

Now that you mention it, the lock is held at that point, isn't it?  So if
that READ_ONCE() actually does anything, there is a bug somewhere else.

Good catch, I will drop this patch, thank you!

							Thanx, Paul

> > Signed-off-by: Paul E. McKenney <paulmck@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 119a373..90ab475 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -678,7 +678,7 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> >  
> >  	/* If grace period not already done and none in progress, start it. */
> >  	if (!rcu_seq_done(&ssp->srcu_gp_seq, s) &&
> > -	    rcu_seq_state(ssp->srcu_gp_seq) == SRCU_STATE_IDLE) {
> > +	    rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq)) == SRCU_STATE_IDLE) {
> >  		WARN_ON_ONCE(ULONG_CMP_GE(ssp->srcu_gp_seq, ssp->srcu_gp_seq_needed));
> >  		srcu_gp_start(ssp);
> >  		if (likely(srcu_init_done))
> > -- 
> > 2.9.5
> >
Paul E. McKenney Feb. 17, 2020, 11:06 p.m. UTC | #3
On Mon, Feb 17, 2020 at 10:32:20AM -0800, Paul E. McKenney wrote:
> On Mon, Feb 17, 2020 at 01:45:07PM +0100, Peter Zijlstra wrote:
> > On Fri, Feb 14, 2020 at 04:29:32PM -0800, paulmck@kernel.org wrote:
> > > From: "Paul E. McKenney" <paulmck@kernel.org>
> > > 
> > > The load of the srcu_struct structure's ->srcu_gp_seq field in
> > > srcu_funnel_gp_start() is lockless, so this commit adds the requisite
> > > READ_ONCE().
> > > 
> > > This data race was reported by KCSAN.
> > 
> > But is there in actual fact a data-race? AFAICT this code was just fine.
> 
> Now that you mention it, the lock is held at that point, isn't it?  So if
> that READ_ONCE() actually does anything, there is a bug somewhere else.
> 
> Good catch, I will drop this patch, thank you!

But looking more closely, I saw a lockless update to that field.  Which
can be argued to be sort of OK, but it definitely was not the intent.
So please see below for the updated patch.

							Thanx, Paul

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

commit 52324a7b8a025f47a1a1a9fbd23ffe59fa764764
Author: Paul E. McKenney <paulmck@kernel.org>
Date:   Fri Jan 3 11:42:05 2020 -0800

    srcu: Hold srcu_struct ->lock when updating ->srcu_gp_seq
    
    A read of the srcu_struct structure's ->srcu_gp_seq field should not
    need READ_ONCE() when that structure's ->lock is held.  Except that this
    lock is not always held when updating this field.  This commit therefore
    acquires the lock around updates and removes a now-unneeded READ_ONCE().
    
    This data race was reported by KCSAN.
    
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
    [ paulmck: Switch from READ_ONCE() to lock per Peter Zilstra question. ]

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 119a373..c19c1df 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -450,7 +450,7 @@ static void srcu_gp_start(struct srcu_struct *ssp)
 	spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
 	smp_mb(); /* Order prior store to ->srcu_gp_seq_needed vs. GP start. */
 	rcu_seq_start(&ssp->srcu_gp_seq);
-	state = rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq));
+	state = rcu_seq_state(ssp->srcu_gp_seq);
 	WARN_ON_ONCE(state != SRCU_STATE_SCAN1);
 }
 
@@ -1130,7 +1130,9 @@ static void srcu_advance_state(struct srcu_struct *ssp)
 			return; /* readers present, retry later. */
 		}
 		srcu_flip(ssp);
+		spin_lock_irq_rcu_node(ssp);
 		rcu_seq_set_state(&ssp->srcu_gp_seq, SRCU_STATE_SCAN2);
+		spin_unlock_irq_rcu_node(ssp);
 	}
 
 	if (rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq)) == SRCU_STATE_SCAN2) {
Peter Zijlstra Feb. 18, 2020, 11:43 a.m. UTC | #4
On Mon, Feb 17, 2020 at 10:32:20AM -0800, Paul E. McKenney wrote:
> On Mon, Feb 17, 2020 at 01:45:07PM +0100, Peter Zijlstra wrote:
> > On Fri, Feb 14, 2020 at 04:29:32PM -0800, paulmck@kernel.org wrote:
> > > From: "Paul E. McKenney" <paulmck@kernel.org>
> > > 
> > > The load of the srcu_struct structure's ->srcu_gp_seq field in
> > > srcu_funnel_gp_start() is lockless, so this commit adds the requisite
> > > READ_ONCE().
> > > 
> > > This data race was reported by KCSAN.
> > 
> > But is there in actual fact a data-race? AFAICT this code was just fine.
> 
> Now that you mention it, the lock is held at that point, isn't it?  So if
> that READ_ONCE() actually does anything, there is a bug somewhere else.
> 
> Good catch, I will drop this patch, thank you!

Well, I didn't get further than the Changelog fails to describe an
actual problem and it looks like compare-against-a-constant.

(worse, it masks off everything but the 2 lowest bits, so even if there
was a problem with load-tearing, it still wouldn't matter)

I'm not going to argue with you if you want to use READ_ONCE() vs
data_race() and a comment to denote false-positive KCSAN warnings, but I
do feel somewhat strongly that the Changelog should describe the actual
problem -- if there is one -- or just flat out state that this is to
make KCSAN shut up but the code is fine.

That is; every KCSAN report should be analysed, right? All I'm asking is
for that analysis to end up in the Changelog.

> > > Signed-off-by: Paul E. McKenney <paulmck@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 119a373..90ab475 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -678,7 +678,7 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> > >  
> > >  	/* If grace period not already done and none in progress, start it. */
> > >  	if (!rcu_seq_done(&ssp->srcu_gp_seq, s) &&
> > > -	    rcu_seq_state(ssp->srcu_gp_seq) == SRCU_STATE_IDLE) {
> > > +	    rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq)) == SRCU_STATE_IDLE) {
> > >  		WARN_ON_ONCE(ULONG_CMP_GE(ssp->srcu_gp_seq, ssp->srcu_gp_seq_needed));
> > >  		srcu_gp_start(ssp);
> > >  		if (likely(srcu_init_done))
> > > -- 
> > > 2.9.5
> > >
Peter Zijlstra Feb. 18, 2020, 11:46 a.m. UTC | #5
On Mon, Feb 17, 2020 at 03:06:57PM -0800, Paul E. McKenney wrote:
> commit 52324a7b8a025f47a1a1a9fbd23ffe59fa764764
> Author: Paul E. McKenney <paulmck@kernel.org>
> Date:   Fri Jan 3 11:42:05 2020 -0800
> 
>     srcu: Hold srcu_struct ->lock when updating ->srcu_gp_seq
>     
>     A read of the srcu_struct structure's ->srcu_gp_seq field should not
>     need READ_ONCE() when that structure's ->lock is held.  Except that this
>     lock is not always held when updating this field.  This commit therefore
>     acquires the lock around updates and removes a now-unneeded READ_ONCE().
>     
>     This data race was reported by KCSAN.
>     
>     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 119a373..c19c1df 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -450,7 +450,7 @@ static void srcu_gp_start(struct srcu_struct *ssp)
>  	spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
>  	smp_mb(); /* Order prior store to ->srcu_gp_seq_needed vs. GP start. */
>  	rcu_seq_start(&ssp->srcu_gp_seq);
> -	state = rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq));
> +	state = rcu_seq_state(ssp->srcu_gp_seq);
>  	WARN_ON_ONCE(state != SRCU_STATE_SCAN1);
>  }
>  
> @@ -1130,7 +1130,9 @@ static void srcu_advance_state(struct srcu_struct *ssp)
>  			return; /* readers present, retry later. */
>  		}
>  		srcu_flip(ssp);
> +		spin_lock_irq_rcu_node(ssp);
>  		rcu_seq_set_state(&ssp->srcu_gp_seq, SRCU_STATE_SCAN2);
> +		spin_unlock_irq_rcu_node(ssp);
>  	}
>  
>  	if (rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq)) == SRCU_STATE_SCAN2) {
Paul E. McKenney Feb. 18, 2020, 4:34 p.m. UTC | #6
On Tue, Feb 18, 2020 at 12:43:34PM +0100, Peter Zijlstra wrote:
> On Mon, Feb 17, 2020 at 10:32:20AM -0800, Paul E. McKenney wrote:
> > On Mon, Feb 17, 2020 at 01:45:07PM +0100, Peter Zijlstra wrote:
> > > On Fri, Feb 14, 2020 at 04:29:32PM -0800, paulmck@kernel.org wrote:
> > > > From: "Paul E. McKenney" <paulmck@kernel.org>
> > > > 
> > > > The load of the srcu_struct structure's ->srcu_gp_seq field in
> > > > srcu_funnel_gp_start() is lockless, so this commit adds the requisite
> > > > READ_ONCE().
> > > > 
> > > > This data race was reported by KCSAN.
> > > 
> > > But is there in actual fact a data-race? AFAICT this code was just fine.
> > 
> > Now that you mention it, the lock is held at that point, isn't it?  So if
> > that READ_ONCE() actually does anything, there is a bug somewhere else.
> > 
> > Good catch, I will drop this patch, thank you!
> 
> Well, I didn't get further than the Changelog fails to describe an
> actual problem and it looks like compare-against-a-constant.
> 
> (worse, it masks off everything but the 2 lowest bits, so even if there
> was a problem with load-tearing, it still wouldn't matter)

There is still the possibility of load fusing.  And the possibility
of defending against possible future changes as well as the current
snapshot of the code base.

> I'm not going to argue with you if you want to use READ_ONCE() vs
> data_race() and a comment to denote false-positive KCSAN warnings, but I
> do feel somewhat strongly that the Changelog should describe the actual
> problem -- if there is one -- or just flat out state that this is to
> make KCSAN shut up but the code is fine.

The problem is that "the code is fine" is highly subjective and varies
over time.  :-/

But in this case there was a real problem, just that I got confused
when analyzing.

> That is; every KCSAN report should be analysed, right? All I'm asking is
> for that analysis to end up in the Changelog.

Before responding further, I have to ask...

Are you intending your "every KCSAN report should be analyzed" to apply
globally or just when someone creates a patch based on such a report?

In any case, you have acked this patch's successor (thank you very
much!), so on this specific patch (or more accurately, its successor)
I presume that we are all good.

							Thanx, Paul

> > > > Signed-off-by: Paul E. McKenney <paulmck@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 119a373..90ab475 100644
> > > > --- a/kernel/rcu/srcutree.c
> > > > +++ b/kernel/rcu/srcutree.c
> > > > @@ -678,7 +678,7 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
> > > >  
> > > >  	/* If grace period not already done and none in progress, start it. */
> > > >  	if (!rcu_seq_done(&ssp->srcu_gp_seq, s) &&
> > > > -	    rcu_seq_state(ssp->srcu_gp_seq) == SRCU_STATE_IDLE) {
> > > > +	    rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq)) == SRCU_STATE_IDLE) {
> > > >  		WARN_ON_ONCE(ULONG_CMP_GE(ssp->srcu_gp_seq, ssp->srcu_gp_seq_needed));
> > > >  		srcu_gp_start(ssp);
> > > >  		if (likely(srcu_init_done))
> > > > -- 
> > > > 2.9.5
> > > >
Peter Zijlstra Feb. 18, 2020, 8:26 p.m. UTC | #7
On Tue, Feb 18, 2020 at 08:34:05AM -0800, Paul E. McKenney wrote:
> On Tue, Feb 18, 2020 at 12:43:34PM +0100, Peter Zijlstra wrote:

> > Well, I didn't get further than the Changelog fails to describe an
> > actual problem and it looks like compare-against-a-constant.
> > 
> > (worse, it masks off everything but the 2 lowest bits, so even if there
> > was a problem with load-tearing, it still wouldn't matter)
> 
> There is still the possibility of load fusing. 

Agreed; that can be an issue. But if so, that then needs to be stated.

> And the possibility
> of defending against possible future changes as well as the current
> snapshot of the code base.

Sure; and like I said, if you want to use READ_ONCE() I'm not going to
argue.

> > I'm not going to argue with you if you want to use READ_ONCE() vs
> > data_race() and a comment to denote false-positive KCSAN warnings, but I
> > do feel somewhat strongly that the Changelog should describe the actual
> > problem -- if there is one -- or just flat out state that this is to
> > make KCSAN shut up but the code is fine.
> 
> The problem is that "the code is fine" is highly subjective and varies
> over time.  :-/
> 
> But in this case there was a real problem, just that I got confused
> when analyzing.

Shit happens :-)

> > That is; every KCSAN report should be analysed, right? All I'm asking is
> > for that analysis to end up in the Changelog.
> 
> Before responding further, I have to ask...
> 
> Are you intending your "every KCSAN report should be analyzed" to apply
> globally or just when someone creates a patch based on such a report?

Ideally every KCSAN report, but that is a longer term effort. But
specifically, when someone has written a patch, I expect that same
someone to have analysed the code. Then it also makes sense to put that
in the Changelog.

> In any case, you have acked this patch's successor (thank you very
> much!), so on this specific patch (or more accurately, its successor)
> I presume that we are all good.

We are. That new patch describes a clear problem and fixes it.

Anyway, the reaoson I'm being difficuly is partly because on the one
hand I'm just an annoying pendant at times, but also because I've seen
a bunch of, let's say, hasty, KCSAN patches.
Paul E. McKenney Feb. 18, 2020, 9:38 p.m. UTC | #8
On Tue, Feb 18, 2020 at 09:26:44PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 18, 2020 at 08:34:05AM -0800, Paul E. McKenney wrote:
> > On Tue, Feb 18, 2020 at 12:43:34PM +0100, Peter Zijlstra wrote:
> 
> > > Well, I didn't get further than the Changelog fails to describe an
> > > actual problem and it looks like compare-against-a-constant.
> > > 
> > > (worse, it masks off everything but the 2 lowest bits, so even if there
> > > was a problem with load-tearing, it still wouldn't matter)
> > 
> > There is still the possibility of load fusing. 
> 
> Agreed; that can be an issue. But if so, that then needs to be stated.
> 
> > And the possibility
> > of defending against possible future changes as well as the current
> > snapshot of the code base.
> 
> Sure; and like I said, if you want to use READ_ONCE() I'm not going to
> argue.
> 
> > > I'm not going to argue with you if you want to use READ_ONCE() vs
> > > data_race() and a comment to denote false-positive KCSAN warnings, but I
> > > do feel somewhat strongly that the Changelog should describe the actual
> > > problem -- if there is one -- or just flat out state that this is to
> > > make KCSAN shut up but the code is fine.
> > 
> > The problem is that "the code is fine" is highly subjective and varies
> > over time.  :-/
> > 
> > But in this case there was a real problem, just that I got confused
> > when analyzing.
> 
> Shit happens :-)
> 
> > > That is; every KCSAN report should be analysed, right? All I'm asking is
> > > for that analysis to end up in the Changelog.
> > 
> > Before responding further, I have to ask...
> > 
> > Are you intending your "every KCSAN report should be analyzed" to apply
> > globally or just when someone creates a patch based on such a report?
> 
> Ideally every KCSAN report, but that is a longer term effort. But
> specifically, when someone has written a patch, I expect that same
> someone to have analysed the code. Then it also makes sense to put that
> in the Changelog.
> 
> > In any case, you have acked this patch's successor (thank you very
> > much!), so on this specific patch (or more accurately, its successor)
> > I presume that we are all good.
> 
> We are. That new patch describes a clear problem and fixes it.
> 
> Anyway, the reaoson I'm being difficuly is partly because on the one
> hand I'm just an annoying pendant at times, but also because I've seen
> a bunch of, let's say, hasty, KCSAN patches.

Agreed.  I briefly considered putting KCSAN for RCU on the newbie list,
but looking at a few of them put paid to that idea.  Responding to them
properly does require a reasonably deep understanding of the code.

							Thanx, Paul
Paul E. McKenney Feb. 18, 2020, 9:48 p.m. UTC | #9
On Tue, Feb 18, 2020 at 12:46:31PM +0100, Peter Zijlstra wrote:
> On Mon, Feb 17, 2020 at 03:06:57PM -0800, Paul E. McKenney wrote:
> > commit 52324a7b8a025f47a1a1a9fbd23ffe59fa764764
> > Author: Paul E. McKenney <paulmck@kernel.org>
> > Date:   Fri Jan 3 11:42:05 2020 -0800
> > 
> >     srcu: Hold srcu_struct ->lock when updating ->srcu_gp_seq
> >     
> >     A read of the srcu_struct structure's ->srcu_gp_seq field should not
> >     need READ_ONCE() when that structure's ->lock is held.  Except that this
> >     lock is not always held when updating this field.  This commit therefore
> >     acquires the lock around updates and removes a now-unneeded READ_ONCE().
> >     
> >     This data race was reported by KCSAN.
> >     
> >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> 
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Applied, thank you!

							Thanx, Paul

> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 119a373..c19c1df 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -450,7 +450,7 @@ static void srcu_gp_start(struct srcu_struct *ssp)
> >  	spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
> >  	smp_mb(); /* Order prior store to ->srcu_gp_seq_needed vs. GP start. */
> >  	rcu_seq_start(&ssp->srcu_gp_seq);
> > -	state = rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq));
> > +	state = rcu_seq_state(ssp->srcu_gp_seq);
> >  	WARN_ON_ONCE(state != SRCU_STATE_SCAN1);
> >  }
> >  
> > @@ -1130,7 +1130,9 @@ static void srcu_advance_state(struct srcu_struct *ssp)
> >  			return; /* readers present, retry later. */
> >  		}
> >  		srcu_flip(ssp);
> > +		spin_lock_irq_rcu_node(ssp);
> >  		rcu_seq_set_state(&ssp->srcu_gp_seq, SRCU_STATE_SCAN2);
> > +		spin_unlock_irq_rcu_node(ssp);
> >  	}
> >  
> >  	if (rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq)) == SRCU_STATE_SCAN2) {

Patch
diff mbox series

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 119a373..90ab475 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -678,7 +678,7 @@  static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp,
 
 	/* If grace period not already done and none in progress, start it. */
 	if (!rcu_seq_done(&ssp->srcu_gp_seq, s) &&
-	    rcu_seq_state(ssp->srcu_gp_seq) == SRCU_STATE_IDLE) {
+	    rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq)) == SRCU_STATE_IDLE) {
 		WARN_ON_ONCE(ULONG_CMP_GE(ssp->srcu_gp_seq, ssp->srcu_gp_seq_needed));
 		srcu_gp_start(ssp);
 		if (likely(srcu_init_done))