From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933504AbdCHCvq (ORCPT ); Tue, 7 Mar 2017 21:51:46 -0500 Received: from mail-pf0-f194.google.com ([209.85.192.194]:32848 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933152AbdCHCvp (ORCPT ); Tue, 7 Mar 2017 21:51:45 -0500 Date: Wed, 8 Mar 2017 10:44:17 +0800 From: Boqun Feng To: "Paul E. McKenney" Cc: Dmitry Vyukov , josh@joshtriplett.org, Steven Rostedt , Mathieu Desnoyers , jiangshanlai@gmail.com, LKML , syzkaller Subject: Re: rcu: WARNING in rcu_seq_end Message-ID: <20170308024417.sptzlehrtfl6bnc2@tardis> References: <20170306230800.GK30506@linux.vnet.ibm.com> <20170307142740.uh2nnaw44albn3t2@tardis> <20170307152715.GM30506@linux.vnet.ibm.com> <20170307230513.7mwnnhuqqbgsecm6@tardis> <20170307233154.GV30506@linux.vnet.ibm.com> <20170308013913.4si2ldnomfktzedq@tardis> <20170308022602.GA30506@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="iyfzdit6ijt55a74" Content-Disposition: inline In-Reply-To: <20170308022602.GA30506@linux.vnet.ibm.com> User-Agent: NeoMutt/20170225 (1.8.0) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --iyfzdit6ijt55a74 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 07, 2017 at 06:26:03PM -0800, Paul E. McKenney wrote: > On Wed, Mar 08, 2017 at 09:39:13AM +0800, Boqun Feng wrote: > > On Tue, Mar 07, 2017 at 03:31:54PM -0800, Paul E. McKenney wrote: > > > On Wed, Mar 08, 2017 at 07:05:13AM +0800, Boqun Feng wrote: > > > > On Tue, Mar 07, 2017 at 07:27:15AM -0800, Paul E. McKenney wrote: > > > > > On Tue, Mar 07, 2017 at 03:43:42PM +0100, Dmitry Vyukov wrote: > > > > > > On Tue, Mar 7, 2017 at 3:27 PM, Boqun Feng wrote: > > > > > > > On Tue, Mar 07, 2017 at 08:05:19AM +0100, Dmitry Vyukov wrote: > > > > > > > [...] > > > > > > >> >> > > > > > > >> >> What is that mutex? And what locks/unlocks provide synchr= onization? I > > > > > > >> >> see that one uses exp_mutex and another -- exp_wake_mutex. > > > > > > >> > > > > > > > >> > Both of them. > > > > > > >> > > > > > > > >> > ->exp_mutex is acquired by the task requesting the grace p= eriod, and > > > > > > >> > the counter's first increment is done by that task under t= hat mutex. > > > > > > >> > This task then schedules a workqueue, which drives forward= the grace > > > > > > >> > period. Upon grace-period completion, the workqueue handl= er does the > > > > > > >> > second increment (the one that your patch addressed). The= workqueue > > > > > > >> > handler then acquires ->exp_wake_mutex and wakes the task = that holds > > > > > > >> > ->exp_mutex (along with all other tasks waiting for this g= race period), > > > > > > >> > and that task releases ->exp_mutex, which allows the next = grace period to > > > > > > >> > start (and the first increment for that next grace period = to be carried > > > > > > >> > out under that lock). The workqueue handler releases ->ex= p_wake_mutex > > > > > > >> > after finishing its wakeups. > > > > > > >> > > > > > > >> Then we need the following for the case when task requesting= the grace > > > > > > >> period does not block, right? > > > > > > > > > > > > > > Won't be necessary I think, as the smp_mb() in rcu_seq_end() = and the > > > > > > > smp_mb__before_atomic() in sync_exp_work_done() already provi= de the > > > > > > > required ordering, no? > > > > > >=20 > > > > > > smp_mb() is probably fine, but smp_mb__before_atomic() is relea= se not > > > > > > acquire. If we want to play that game, then I guess we also need > > > >=20 > > > > The point is that smp_mb__before_atomic() + atomic_long_inc() will > > > > guarantee a smp_mb() before or right along with the atomic operatio= n, > > > > and that's enough because rcu_seq_done() followed by a smp_mb() will > > > > give it a acquire-like behavior. > > >=20 > > > Given current architectures, true enough, from what I can see. > > >=20 > > > However, let's take a look at atomic_ops.rst: > > >=20 > > >=20 > > > If a caller requires memory barrier semantics around an atomic_t > > > operation which does not return a value, a set of interfaces are > > > defined which accomplish this:: > > >=20 > > > void smp_mb__before_atomic(void); > > > void smp_mb__after_atomic(void); > > >=20 > > > For example, smp_mb__before_atomic() can be used like so:: > > >=20 > > > obj->dead =3D 1; > > > smp_mb__before_atomic(); > > > atomic_dec(&obj->ref_count); > > >=20 > > > It makes sure that all memory operations preceding the atomic_dec() > > > call are strongly ordered with respect to the atomic counter > > > operation. In the above example, it guarantees that the assignment = of > > > "1" to obj->dead will be globally visible to other cpus before the > > > atomic counter decrement. > > >=20 > > > Without the explicit smp_mb__before_atomic() call, the > > > implementation could legally allow the atomic counter update visible > > > to other cpus before the "obj->dead =3D 1;" assignment. > > >=20 > > > So the ordering is guaranteed against the atomic operation, not > > > necessarily the stuff after it. But again, the implementations I know > > > of do make the guarantee, hence my calling it a theoretical bug in the > > > commit log. > >=20 > > Fair enough ;-) It's me who misunderstood this part of document. > >=20 > > However, the names of the barriers are smp_mb__{before,after}_atomic(), > > so if they, semantically, only provide ordering for the corresponding > > atomic ops rather than a full barrier, I would their names are > > misleading ;-) >=20 > Well, if you have both ordering before and after, then you have full > ordering. >=20 I mean the names of the barriers are *smp_mb*__before_atomic() and *smp_mb*__after_atomic(), so it's natural to think they provide a smp_mb() in some situations ;-) > > > > > > smp_mb__after_atomic() there. But it would be way easier to und= erstand > > > >=20 > > > > Adding smp_mb__after_atomic() would be pointless as it's the load o= f=20 > > > > ->expedited_sequence that we want to ensure having acquire behavior > > > > rather than the atomic increment of @stat. > > >=20 > > > Again, agreed given current code, but atomic_ops.rst doesn't guarantee > > > ordering past the actual atomic operation itself. > >=20 > > Neither does atomic_ops.rst guarantee the ordering between a load before > > the atomic op and memory accesses after the atomic op, right? I.e. > > atomic_ops.rst doesn't say no for reordering like this: > >=20 > > r1 =3D READ_ONCE(a); ---------+ > > atomic_long_inc(b); | =20 > > smp_mb__after_atomic(); | > > WRITE_ONCE(c); | > > {r1 =3D READ_ONCE(a)} <-------+ > > =09 > > So it's still not an acquire for READ_ONCE(a), in our case "a" is > > ->expedited_sequence. > >=20 > > To me, we can either fix the atomic_ops.rst or, as I proposed, just > > change smp_mb__before_atomic() to smp_mb(). >=20 > Or have both an smp_mb__before_atomic() and an smp_mb__after_atomic(), > as is the usual approach when you need full ordering. ;-) >=20 Yes ;-) It's just that "adding a barrier after one operation to provide acquire semantic for another operation" looks weird to me. Regards, Boqun > Thanx, Paul >=20 > > Thoughts? > >=20 > > Regards, > > Boqun > >=20 > > > Thanx, Paul > > >=20 > > > > > > what's happens there and prove that it's correct, if we use > > > > > > store_release/load_acquire. > > > > >=20 > > > > > Fair point, how about the following? > > > > >=20 > > > > > Thanx, Paul > > > > >=20 > > > > > -----------------------------------------------------------------= ------- > > > > >=20 > > > > > commit 6fd8074f1976596898e39f5b7ea1755652533906 > > > > > Author: Paul E. McKenney > > > > > Date: Tue Mar 7 07:21:23 2017 -0800 > > > > >=20 > > > > > rcu: Add smp_mb__after_atomic() to sync_exp_work_done() > > > > > =20 > > > > > The sync_exp_work_done() function needs to fully order the co= unter-check > > > > > operation against anything happening after the corresponding = grace period. > > > > > This is a theoretical bug, as all current architectures eithe= r provide > > > > > full ordering for atomic operation on the one hand or impleme= nt, > > > > > however, a little future-proofing is a good thing. This comm= it > > > > > therefore adds smp_mb__after_atomic() after the atomic_long_i= nc() > > > > > in sync_exp_work_done(). > > > > > =20 > > > > > Reported-by: Dmitry Vyukov > > > > > Signed-off-by: Paul E. McKenney > > > > >=20 > > > > > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h > > > > > index 027e123d93c7..652071abd9b4 100644 > > > > > --- a/kernel/rcu/tree_exp.h > > > > > +++ b/kernel/rcu/tree_exp.h > > > > > @@ -247,6 +247,7 @@ static bool sync_exp_work_done(struct rcu_sta= te *rsp, atomic_long_t *stat, > > > > > /* Ensure test happens before caller kfree(). */ > > > > > smp_mb__before_atomic(); /* ^^^ */ > > > > > atomic_long_inc(stat); > > > > > + smp_mb__after_atomic(); /* ^^^ */ > > > >=20 > > > > If we really care about future-proofing, I think it's more safe to > > > > change smp_mb__before_atomic() to smp_mb() rather than adding > > > > __after_atomic() barrier. Though I think both would be unnecessary = ;-) > > > >=20 > > > > Regards, > > > > Boqun > > > >=20 > > > > > return true; > > > > > } > > > > > return false; > > > > >=20 > > >=20 > > >=20 >=20 >=20 --iyfzdit6ijt55a74 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAli/b/4ACgkQSXnow7UH +rid3wf+Jia2JPtiKf36i4ERI0Zkm4i7nht00tNlqGUZCHotva5fa93K5eW6eGEL EBBHGb/3WVsaojg7SIERGqZewkdMj4OSsai5MRxBIdhAVIVnlkve79LGJPwYFQ/x FjuZ/4hTwVilCTiIF5EjeOw61CnS2fG/Rp61Ybq/tQk+in/PXLfU7xIYennEOeKy xxcilap8DW64bRASyGc27wsv8tLdNK9WZdsqJhMbZeS9SlTADuNK5Dui5KF4fUsx 4PKWdy9Xrihn9E/abGvdN4JkEJvHMxQDZxRE2HDVA8GX9Vpl3mfuxIhnLsuzv5VR UB2PrEvfehZ5eZV+kjGmMyoVVwNTwg== =23mY -----END PGP SIGNATURE----- --iyfzdit6ijt55a74--