On Mon, Feb 17, 2020 at 01:42:31PM +0100, Peter Zijlstra wrote: > On Fri, Feb 14, 2020 at 04:29:29PM -0800, paulmck@xxxxxxxxxx wrote: > > From: "Paul E. McKenney" <paulmck@xxxxxxxxxx> > > > > The srcu_node structure's ->srcu_gp_seq_needed_exp field is accessed > > locklessly, so updates must use WRITE_ONCE(). This commit therefore > > adds the needed WRITE_ONCE() invocations. > > > > This data race was reported by KCSAN. Not appropriate for backporting > > due to failure being unlikely. > > This does indeed look like there can be a failure; but this Changelog > fails to describe an actual problem. Peter it sounds like you have a failure scenario in mind. Could you describe more if so? I am curious if you were thinking of invented-stores issue here. For educational purposes, I was trying to come up with an example where my compiler does something bad to code without WRITE_ONCE(). So far I only can reproduce a write-tearing example when write with an immediate value is split into 2 writes, like Will mentioned: http://lore.kernel.org/r/20190821103200.kpufwtviqhpbuv2n@willie-the-truck But that does not seem to apply to this code. thanks, - Joel > > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx> > > --- > > kernel/rcu/srcutree.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index 657e6a7..b1edac9 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -550,7 +550,7 @@ static void srcu_gp_end(struct srcu_struct *ssp) > > snp->srcu_have_cbs[idx] = gpseq; > > rcu_seq_set_state(&snp->srcu_have_cbs[idx], 1); > > if (ULONG_CMP_LT(snp->srcu_gp_seq_needed_exp, gpseq)) > > - snp->srcu_gp_seq_needed_exp = gpseq; > > + WRITE_ONCE(snp->srcu_gp_seq_needed_exp, gpseq); > > mask = snp->srcu_data_have_cbs[idx]; > > snp->srcu_data_have_cbs[idx] = 0; > > spin_unlock_irq_rcu_node(snp); > > @@ -660,7 +660,7 @@ static void srcu_funnel_gp_start(struct srcu_struct *ssp, struct srcu_data *sdp, > > if (snp == sdp->mynode) > > snp->srcu_data_have_cbs[idx] |= sdp->grpmask; > > if (!do_norm && ULONG_CMP_LT(snp->srcu_gp_seq_needed_exp, s)) > > - snp->srcu_gp_seq_needed_exp = s; > > + WRITE_ONCE(snp->srcu_gp_seq_needed_exp, s); > > spin_unlock_irqrestore_rcu_node(snp, flags); > > } > > > > -- > > 2.9.5 > >
