From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754264AbcKOOTT (ORCPT ); Tue, 15 Nov 2016 09:19:19 -0500 Received: from mail-pg0-f68.google.com ([74.125.83.68]:35813 "EHLO mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752488AbcKOOTR (ORCPT ); Tue, 15 Nov 2016 09:19:17 -0500 Date: Tue, 15 Nov 2016 22:19:09 +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: <20161115141909.GJ27541@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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="apbmkPN6Hu/1dI3g" Content-Disposition: inline In-Reply-To: <20161115130154.GX3117@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 --apbmkPN6Hu/1dI3g Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 and fa= il to > > > + * decrement when saturated at UINT_MAX. > > > + * > > > + * Provides release memory ordering, such that prior loads and store= s 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 this > > 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 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(). See a similar example: https://marc.info/?l=3Dlinux-s390&m=3D146604339321723&w=3D2 But as I said, we actually only need the pairing of orderings: 1) load part of cmpxchg -> free()=20 2) object accesses -> store part of cmpxchg 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. 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". Am I missing some subtle orderings here? Regards, Boqun > So I don't think we can get free() (which very much includes stores) to > happen before the store-release. --apbmkPN6Hu/1dI3g Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAABCAAGBQJYKxlZAAoJEEl56MO1B/q4buMH/RMJAuV8NFF3Mtjh1sPBer8R TRSaEOs8MPaO3q+jDguRlmyJr/z3KZiArYREmDwyLshLbuaGe3VIDvWw+FAYovvH wZwHMQyw/7vWG0LGFKEevCzNjKcUjkUkKlm9YNzw9D6+j5mLmOh5zH2M3pELa083 CLX+coDpoU3oZnZYZ2OHOD2kXV026NEo1kU22mszzD4aILF+IcHFHk0XkZKT32HX 3UXUCjiylfdJhwe1kkSi0RplYv/bnbR5aV6jjl0dEnzD9vYBwGfzJUZRE/lHeI8u Ug44/anr6LwIl1J3AdrX9M+L98YODwI9mreW1kAGy95SWGL7NBXUjXa/UUE/9wY= =pKq9 -----END PGP SIGNATURE----- --apbmkPN6Hu/1dI3g--