Date: Tue, 1 Aug 2017 10:14:48 +0800
From: Boqun Feng <boqun.feng@gmail.com>
To: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
        Will Deacon <will.deacon@arm.com>, linux-kernel@vger.kernel.org,
        Ingo Molnar <mingo@kernel.org>, Thomas Gleixner <tglx@linutronix.de>,
        Randy Dunlap <rdunlap@infradead.org>
Subject: Re: [RFC][PATCH v3]: documentation,atomic: Add new documents
Message-ID: <20170801021448.3ana7ad2xha4vbca@tardis>
References: <20170609092450.jwmldgtli57ozxgq@hirez.programming.kicks-ass.net>
 <20170609154442.GB9236@arm.com>
 <20170609193604.ncw3hhgvewzc3h5u@hirez.programming.kicks-ass.net>
 <20170611135632.sl72klbeklelupej@tardis>
 <20170612144929.3wiwtbqopsfpm3qk@hirez.programming.kicks-ass.net>
 <20170726115328.2sxiitivlnlq64dk@hirez.programming.kicks-ass.net>
 <20170726124750.vktrn5zi2gmpzfru@tardis>
 <20170731090535.rjgnoewqg7mhzr55@hirez.programming.kicks-ass.net>
 <20170731110403.ou3zqsp3uviqorkz@tardis>
 <20170731174345.GL3730@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="62bqypyakm26i5ty"
Content-Disposition: inline
In-Reply-To: <20170731174345.GL3730@linux.vnet.ibm.com>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4225
Lines: 128


--62bqypyakm26i5ty
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Jul 31, 2017 at 10:43:45AM -0700, Paul E. McKenney wrote:
> On Mon, Jul 31, 2017 at 07:04:03PM +0800, Boqun Feng wrote:
> > On Mon, Jul 31, 2017 at 11:05:35AM +0200, Peter Zijlstra wrote:
> > > On Wed, Jul 26, 2017 at 08:47:50PM +0800, Boqun Feng wrote:
> > >=20
> > > > > +
> > > > > +Further, while something like:
> > > > > +
> > > > > +  smp_mb__before_atomic();
> > > > > +  atomic_dec(&X);
> > > > > +
> > > > > +is a 'typical' RELEASE pattern, the barrier is strictly stronger=
 than
> > > > > +a RELEASE. Similarly for something like:
> > > > > +
> > > >=20
> > > > .. at here. Maybe you planned to put stronger ACQUIRE pattern?
> > >=20
> > > Yes, although I struggled to find a sensible one. The problem is that
> > > ACQUIRE is on loads and value returning atomics have an ACQUIRE varia=
nt,
> > > so why would you ever want to use smp_mb__after_atomic() for this.
> > >=20
> > >=20
> > > That is, the best I could come up with is something like:
> > >=20
> > > 	val =3D atomic_fetch_or_relaxed(1, &var);
> > > 	smp_mb__after_atomic();
> > >=20
> > > But in that case we should've just written:
> > >=20
> > > 	val =3D atomic_fetch_or_acquire(1, &var);
> > >=20
> >=20
> > Agreed.
> >=20
> > And besides, in memory-barriers.txt, the wording is:
> >=20
> >  (*) smp_mb__before_atomic();
> >  (*) smp_mb__after_atomic();
> >=20
> >      These are for use with atomic (such as add, subtract, increment and
> >      decrement) functions that don't return a value, especially when us=
ed for
> >      reference counting.=20
> >=20
> > So actually, using smp_mb__after_atomic() for ACQUIRE is a misuse.
>=20
> You lost me on this one.
>=20
> Why wouldn't the following have ACQUIRE semantics?
>=20
> 	atomic_inc(&var);
> 	smp_mb__after_atomic();
>=20
> Is the issue that there is no actual value returned or some such?
>=20

That "misuse" is a wrong word there ;-(=20

I actually meant "the usage is correct but we don't encourage using
smp_mb__after_atomic() *only* for an ACQUIRE, because _acquire() could
always be used in that case, as Peter said".

In fact in your case, the ordering is stronger, both the load and store
part of the atomic op are ordered with the memory ops following it.

In short, I suggested we tell users to use
smp_mb__{before,after}_atomic() only when _{release,acquire} ops don't
suffice, i.e. for an RCsc RELEASE or a smp_mb() to order ops other than
the atomic op itself(like the one you use in __call_rcu_nocb_enqueue()).

But maybe this is too strict, and Peter said he would write something
about it in IRC, so I'm not that stick to this suggestion ;-)

Regards,
Boqun

> > > Suggestions?
> >=20
> > As a result, I think it's better we say smp_mb__{before,after}_atomic()
> > are only for 1) non-value-returning RmW atomic ops, 2)
> > {set,clear,change}_bit and 3) internal use of atomic primitives(e.g. the
> > generic version of fully ordered atomics).
> >=20
> > 1) prevents people to use it for an ACQUIRE, but allows for a RELEASE.
> > 1) & 2) makes atomic_t.txt consistent with memory-barriers.txt
> > 3) explains our usage of those barriers internally.
> >=20
> > Thoughts?
>=20
> So if I have something like this, the assertion really can trigger?
>=20
> 	WRITE_ONCE(x, 1);		atomic_inc(&y);
> 	r0 =3D xchg_release(&y, 5);	smp_mb__after_atomic();
> 					r1 =3D READ_ONCE(x);
>=20
>=20
> 	WARN_ON(r0 =3D=3D 0 && r1 =3D=3D 0);
>=20
> I must confess that I am not seeing why we would want to allow this
> outcome.
>=20
> 							Thanx, Paul
>=20

--62bqypyakm26i5ty
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAll/5BQACgkQSXnow7UH
+rjDzAf/ZjIEf1IzcnhcmEVuHA3U8oM6fJttDZbyrc4z8TtwGWs0XEj+xE1DFyLk
/88K8abiTyIQUdWKgTnweZ7QOOlgF6fHvF4gIaMpCQ+wI5vYFih59omQ3msLroBb
bQ9yj+8+PZ6PLlmfrYYIbW2f0vZ5D1mHb+qW3g7embEVSo1+u8QugsVY8ZnA+qkO
f3MZP252CCxBkvDlP6LYnoE+709bpnpGoa93pxfeT7xoatFOCWPGjh3cG0rh56Up
AiS6qJa+eDQVle2VM1sTafOPSZF1tus/Zn6O44QKixAD3f/VDJyw0utUlTkRFumO
PdCnDOwhzTVMX9LyTkdHA/1QoagrDw==
=hThw
-----END PGP SIGNATURE-----

--62bqypyakm26i5ty--