From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934402AbcKQJtg (ORCPT ); Thu, 17 Nov 2016 04:49:36 -0500 Received: from mail-pg0-f68.google.com ([74.125.83.68]:36261 "EHLO mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932816AbcKQJsy (ORCPT ); Thu, 17 Nov 2016 04:48:54 -0500 Date: Thu, 17 Nov 2016 17:48:51 +0800 From: Boqun Feng To: Peter Zijlstra Cc: gregkh@linuxfoundation.org, keescook@chromium.org, will.deacon@arm.com, elena.reshetova@intel.com, arnd@arndb.de, tglx@linutronix.de, mingo@kernel.org, hpa@zytor.com, dave@progbits.org, linux-kernel@vger.kernel.org Subject: Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Message-ID: <20161117094851.GA5227@tardis.cn.ibm.com> References: <20161114173946.501528675@infradead.org> <20161114174446.832175072@infradead.org> <20161115123337.GD12110@tardis.cn.ibm.com> <20161115130154.GX3117@twins.programming.kicks-ass.net> <20161115141909.GJ27541@tardis.cn.ibm.com> <20161117092800.GD3142@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="HcAYCG3uE/tztfnV" Content-Disposition: inline In-Reply-To: <20161117092800.GD3142@twins.programming.kicks-ass.net> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --HcAYCG3uE/tztfnV Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Nov 17, 2016 at 10:28:00AM +0100, Peter Zijlstra wrote: > On Tue, Nov 15, 2016 at 10:19:09PM +0800, Boqun Feng wrote: > > On Tue, Nov 15, 2016 at 02:01:54PM +0100, Peter Zijlstra wrote: > > > On Tue, Nov 15, 2016 at 08:33:37PM +0800, Boqun Feng wrote: > > > > Hi Peter, > > > >=20 > > > > On Mon, Nov 14, 2016 at 06:39:53PM +0100, Peter Zijlstra wrote: > > > > [...] > > > > > +/* > > > > > + * Similar to atomic_dec_and_test(), it will BUG on underflow an= d fail to > > > > > + * decrement when saturated at UINT_MAX. > > > > > + * > > > > > + * Provides release memory ordering, such that prior loads and s= tores are done > > > > > + * before a subsequent free. > > > >=20 > > > > I'm not sure this is correct, the RELEASE semantics is for the STORE > > > > part of cmpxchg, and semantically it will guarantee that memory > > > > operations after cmpxchg won't be reordered upwards, for example, on > > > > ARM64, the following code: > > > >=20 > > > > WRITE_ONCE(x, 1) > > > > =09 > > > > atomic_cmpxchg_release(&a, 1, 2); > > > > r1 =3D ll(&a) > > > > if (r1 =3D=3D 1) { > > > > sc_release(&a, 2); > > > > } > > > > =09 > > > > free() > > > >=20 > > > > could be reordered as, I think: > > > >=20 > > > > atomic_cmpxchg_release(&a, 1, 2); > > > > r1 =3D ll(&a) > > > > if (r1 =3D=3D 1) { > > > > free() > > > > WRITE_ONCE(x, 1) > > > > sc_release(&a, 2); > > > > } > > > >=20 > > > > Of course, we need to wait for Will to confirm about this. But if t= his > > > > could happen, we'd better to use a smp_mb()+atomic_cmpxchg_relaxed() > > > > here and for other refcount_dec_and_*(). > > >=20 > > > Can't happen I think because of the control dependency between > > > dec_and_test() and free(). > > >=20 > > > That is, the cmpxchg_release() must complete to determine if it was > > > successful or it needs a retry. The success, combined with the state = of > > > the variable will then determine if we call free(). > > >=20 > >=20 > > The thing is that determination of the variable's state(i.e. > > store_release() succeeds) and the actual writeback to memory are two > > separate events. So yes, free() won't execute before store_release() > > commits successfully, but there is no barrier here to order the memory > > effects of store_release() and free(). >=20 > Doesn't matter. If we dropped the refcount to 0, nobody else will be > observing this memory anymore (unless ill-formed program). The only > thing we need is that the free() will not be speculated. >=20 > This is because all RmW on a specific variable, irrespective of their > memory ordering on other loads/stores, are totally ordered against one > another. >=20 Fair enough ;-) > > But as I said, we actually only need the pairing of orderings: > >=20 > > 1) load part of cmpxchg -> free()=20 > > 2) object accesses -> store part of cmpxchg > >=20 > > Ordering #1 can be achieved via control dependency as you pointed out > > that free()s very much includes stores. And ordering #2 can be achieved > > with RELEASE. > >=20 > > So the code is right, I just thought the comment may be misleading. The > > reason we use cmpxchg_release() is just for achieving ordering #2, and > > not to order "prior loads and stores" with "a subsequent free". > >=20 > > Am I missing some subtle orderings here? >=20 > I would want to further quality 1), it must be no earlier than the load > of the last / successful ll/sc round. >=20 Great, that's more accurate! > At that point we're guaranteed a reference count of 1 that _will_ drop > to 0, and thus nobody else (should) reference that memory anymore. >=20 > If we agree on this, I'll update the comment :-) Will, do you too agree? Agreed ;-) Control dependencies and RELEASE are totally enough for the internal correctness of refcount_t along with its interactivity with free(). People better not reply order guarantees other than this ;-) Regards, Boqun --HcAYCG3uE/tztfnV Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAABCAAGBQJYLX0AAAoJEEl56MO1B/q4oawH/juAiGqRMqIxvlvoEZWr7Q7o AgFfAfTua+rWAoQ7D13V/2bgUfnZsitSZ/WeAa50oQScsk7sybb5/flYsxTuX+EK YPy+kz6RWFL4YdM8tgx2i8miU1AljqMIUWuQVA9lQfYdJzDCjpRzVD6Cf1SkAMO6 677RwZJFuUGqzRHb80g7Rcgj0ejhl7pZN1apr9bLQbg7wGJNDzl2OFp6E1cR8CQs E6s3QCQlZajB/YC+SeuaY0pS1hHroxXgDWf4G4m7IHQMJx8ijHzsmU7rmKsyDefZ l0ZDpIBZqUg960MNtRrix35nH8Lz6XYmCe0Ls1Jt0Zh0AZwRinou0yiC2mCpL0k= =Oi4x -----END PGP SIGNATURE----- --HcAYCG3uE/tztfnV--