From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753786AbcKOMds (ORCPT ); Tue, 15 Nov 2016 07:33:48 -0500 Received: from mail-pf0-f193.google.com ([209.85.192.193]:34015 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751271AbcKOMdp (ORCPT ); Tue, 15 Nov 2016 07:33:45 -0500 Date: Tue, 15 Nov 2016 20:33:37 +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: <20161115123337.GD12110@tardis.cn.ibm.com> References: <20161114173946.501528675@infradead.org> <20161114174446.832175072@infradead.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="pQhZXvAqiZgbeUkD" Content-Disposition: inline In-Reply-To: <20161114174446.832175072@infradead.org> 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 --pQhZXvAqiZgbeUkD Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Peter, 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 fail to > + * decrement when saturated at UINT_MAX. > + * > + * Provides release memory ordering, such that prior loads and stores ar= e done > + * before a subsequent free. 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: 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() could be reordered as, I think: atomic_cmpxchg_release(&a, 1, 2); r1 =3D ll(&a) if (r1 =3D=3D 1) { free() WRITE_ONCE(x, 1) sc_release(&a, 2); } 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_*(). That said, I think the really ordering guarantee we need here is that two cmpxchg()s in refcount_dec_and_*() could be paired with each other to ensure nobody observes a freed object inside a refcount critical section. In that case, we need atomic_cmpxchg() here for ordering on both sides. Or maybe replace atomic_read() with smp_load_acquire(). Regards, Boqun > + */ > +static inline __must_check > +bool refcount_dec_and_test(refcount_t *r) > +{ > + unsigned int old, new, val =3D atomic_read(&r->refs); > + > + for (;;) { > + if (val =3D=3D UINT_MAX) > + return false; > + > + new =3D val - 1; > + if (new > val) > + BUG(); /* underflow */ > + > + old =3D atomic_cmpxchg_release(&r->refs, val, new); > + if (old =3D=3D val) > + break; > + > + val =3D old; > + } > + > + return !new; > +} > + > +/* > + * Similar to atomic_dec_and_mutex_lock(), it will BUG on underflow and = fail > + * to decrement when saturated at UINT_MAX. > + * > + * Provides release memory ordering, such that prior loads and stores ar= e done > + * before a subsequent free. This allows free() while holding the mutex. > + */ > +static inline __must_check > +bool refcount_dec_and_mutex_lock(refcount_t *r, struct mutex *lock) > +{ > + unsigned int old, new, val =3D atomic_read(&r->refs); > + bool locked =3D false; > + > + for (;;) { > + if (val =3D=3D UINT_MAX) > + return false; > + > + if (val =3D=3D 1 && !locked) { > + locked =3D true; > + mutex_lock(lock); > + } > + > + new =3D val - 1; > + if (new > val) { > + if (locked) > + mutex_unlock(lock); > + BUG(); /* underflow */ > + } > + > + old =3D atomic_cmpxchg_release(&r->refs, val, new); > + if (old =3D=3D val) > + break; > + > + val =3D old; > + } > + > + if (new && locked) > + mutex_unlock(lock); > + > + return !new; > +} > + > +/* > + * Similar to atomic_dec_and_lock(), it will BUG on underflow and fail > + * to decrement when saturated at UINT_MAX. > + * > + * Provides release memory ordering, such that prior loads and stores ar= e done > + * before a subsequent free. This allows free() while holding the lock. > + */ > +static inline __must_check > +bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock) > +{ > + unsigned int old, new, val =3D atomic_read(&r->refs); > + bool locked =3D false; > + > + for (;;) { > + if (val =3D=3D UINT_MAX) > + return false; > + > + if (val =3D=3D 1 && !locked) { > + locked =3D true; > + spin_lock(lock); > + } > + > + new =3D val - 1; > + if (new > val) { > + if (locked) > + spin_unlock(lock); > + BUG(); /* underflow */ > + } > + > + old =3D atomic_cmpxchg_release(&r->refs, val, new); > + if (old =3D=3D val) > + break; > + > + val =3D old; > + } > + > + if (new && locked) > + spin_unlock(lock); > + > + return !new; > +} > + > +#endif /* _LINUX_REFCOUNT_H */ >=20 >=20 --pQhZXvAqiZgbeUkD Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAABCAAGBQJYKwCbAAoJEEl56MO1B/q4dd4IAK1MECPxkTyiS1dv98ZO9JGc BEKzYLYoJyGAmJ31F44L0C2a64KsLBVBSQwFpFYzVSzVmxYRwmFuSuGZVU+/Ezxa JRHwQLq8o6kcW/lXNP2nol0IqZ7pP01w4wiPyLamblwoebOUq5+mnUpR6D6PPYBb fazvQ32dLu035FOwFbRPpfJSNvsC/1BEbDZufVUnl7+jsprCOVHvUKvV8ZsEMr22 h4rv9tzMbFqk6HritAukfg21bRwrpNA55I3K9/2st8womimGo273UbIKhZJVHvnl z3nJjYl/8VmAAn1CItfcnys4j/trx7/TXUQGurQJu/ejAXVlTV1Pny9ZuNw2D8I= =g3vG -----END PGP SIGNATURE----- --pQhZXvAqiZgbeUkD--