[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <26f2d843-b76f-452a-a5d0-b3a146351bb2@paulmck-laptop>
Date: Sun, 12 May 2024 07:57:50 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Oleg Nesterov <oleg@...hat.com>
Cc: "Uladzislau Rezki (Sony)" <urezki@...il.com>, RCU <rcu@...r.kernel.org>,
Neeraj upadhyay <Neeraj.Upadhyay@....com>,
Boqun Feng <boqun.feng@...il.com>, Hillf Danton <hdanton@...a.com>,
Joel Fernandes <joel@...lfernandes.org>,
LKML <linux-kernel@...r.kernel.org>,
Oleksiy Avramchenko <oleksiy.avramchenko@...y.com>,
Frederic Weisbecker <frederic@...nel.org>,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH 25/48] rcu: Mark writes to rcu_sync ->gp_count field
On Sun, May 12, 2024 at 12:53:06PM +0200, Oleg Nesterov wrote:
> On 05/10, Paul E. McKenney wrote:
> >
> > On Fri, May 10, 2024 at 01:31:49PM +0200, Oleg Nesterov wrote:
> >
> > > Why is that?
> >
> > Because I run KCSAN on RCU using Kconfig options that cause KCSAN
> > to be more strict.
>
> Yes, I see now.
>
> > > but how can KCSAN detect that all accesses to X are properly marked? I see nothing
> > > KCSAN-related in the definition of WRITE_ONCE() or READ_ONCE().
> >
> > The trick is that KCSAN sees the volatile cast that both READ_ONCE()
> > and WRITE_ONCE() use.
>
> Hmm. grep-grep-grep... I seem to understand, DEFINE_TSAN_VOLATILE_READ_WRITE.
> So __tsan_volatile_readX() will use KCSAN_ACCESS_ATOMIC.
>
> Thanks!
You got it!!!
> > > And what does the "all accesses" above actually mean? The 2nd version does
> > >
> > > WRITE_ONCE(X, X+1);
> > >
> > > but "X + 1" is the plain/unmarked access?
> >
> > ...
> >
> > In that case, the "X+1" cannot be involved in a data race, so KCSAN
> > won't complain.
>
> Yes, yes, I understand now.
>
> Paul, thanks for your explanations! and sorry for wasting your time by
> provoking the unnecessarily long discussion.
Not a problem and absolutely no need to apologize! Of course, please do
pass this information on to anyone else needing it.
> I am going to send the trivial patch which moves these WARN_ON()'s under
> spin_lock(), this looks more clean to me. But I won't argue if you prefer
> your original patch.
Actually, I like your patch quite a bit better than I do my original.
In fact, I feel quite foolish that I did not think of this to begin with.
With your way, we have strict locking for that field and can therefore
just use plain C-language accesses for all accesses to it. KCSAN will
then warn us of any buggy lockless access to that field, even if that
buggy access uses READ_ONCE(). Much much better your way!!!
So thank you for persisting on this!
Thanx, Paul
Powered by blists - more mailing lists