Received-SPF: pass (google.com: domain of linux-kernel+bounces-175851-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 10 May 2024 07:04:39 -0700
From: "Paul E. McKenney" <paulmck@kernel.org>
To: Oleg Nesterov <oleg@redhat.com>
Cc: "Uladzislau Rezki (Sony)" <urezki@gmail.com>, RCU <rcu@vger.kernel.org>,
	Neeraj upadhyay <Neeraj.Upadhyay@amd.com>,
	Boqun Feng <boqun.feng@gmail.com>, Hillf Danton <hdanton@sina.com>,
	Joel Fernandes <joel@joelfernandes.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Oleksiy Avramchenko <oleksiy.avramchenko@sony.com>,
	Frederic Weisbecker <frederic@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH 25/48] rcu: Mark writes to rcu_sync ->gp_count field
Message-ID: <8ca02df3-5034-4483-8e64-3fc22eb14431@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <20240507093530.3043-1-urezki@gmail.com>
 <20240507093530.3043-26-urezki@gmail.com>
 <ZjpAsYJIfzYSKgdA@redhat.com>
 <4c9e89b5-c981-4809-8bc2-247563ce04e9@paulmck-laptop>
 <20240509151312.GA22612@redhat.com>
 <f0a02cfe-7fc2-494c-8734-e5583f42a8f7@paulmck-laptop>
 <20240510113149.GA24764@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240510113149.GA24764@redhat.com>

On Fri, May 10, 2024 at 01:31:49PM +0200, Oleg Nesterov wrote:
> On 05/09, Paul E. McKenney wrote:
> >
> > On Thu, May 09, 2024 at 05:13:12PM +0200, Oleg Nesterov wrote:
> > >
> > > We can move these WARN_ON()'s into the ->rss_lock protected section.
> > >
> > > Or perhaps we can use data_race(rsp->gp_count) ? To be honest I thought
> > > that READ_ONCE() should be enough...
> > >
> > > Or we can simply kill these WARN_ON_ONCE()'s.
> >
> > Or we could move those WARN_ON_ONCE() under the lock.
> 
> Sure, see above.
> 
> But could you help me to understand this magic? I naively thought
> that READ_ONCE() is always "safe"...
> 
> So, unless I am totally confused it turns out that, say,
> 
> 	CPU 0			CPU 1
> 	-----			-----
> 
> 	spin_lock(LOCK);
> 	++X;			READ_ONCE(X); // data race
> 	spin_unlock(LOCK);
> 
> is data-racy accoring to KCSAN, while
> 
> 	CPU 0			CPU 1
> 	-----			-----
> 
> 	spin_lock(LOCK);
> 	WRITE_ONCE(X, X+1);	READ_ONCE(X); // no data race
> 	spin_unlock(LOCK);
> 
> is not.

Agreed, in RCU code.

> Why is that?

Because I run KCSAN on RCU using Kconfig options that cause KCSAN
to be more strict.

> Trying to read Documentation/dev-tools/kcsan.rst... it says
> 
> 	KCSAN is aware of *marked atomic operations* (``READ_ONCE``, WRITE_ONCE``,
> 
> 	...
> 
> 	if all accesses to a variable that is accessed concurrently are properly
> 	marked, KCSAN will never trigger a watchpoint
> 
> 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.

> 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?

That would be the correct usage in RCU code if there were lockless
accesses to X, which would use READ_ONCE(), but a lock was held across
that WRITE_ONCE() such that there would be no concurrent updates of X.
In that case, the "X+1" cannot be involved in a data race, so KCSAN
won't complain.

But if all accesses to X were protected by an exclusive lock, then there
would be no data races involving X, and thus no marking of any accesses
to X.  Which would allow KCSAN to detect buggy lockless accesses to X.

							Thanx, Paul