From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932559AbcKQJ2K (ORCPT ); Thu, 17 Nov 2016 04:28:10 -0500 Received: from merlin.infradead.org ([205.233.59.134]:42530 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932328AbcKQJ2E (ORCPT ); Thu, 17 Nov 2016 04:28:04 -0500 Date: Thu, 17 Nov 2016 10:28:00 +0100 From: Peter Zijlstra To: Boqun Feng 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: <20161117092800.GD3142@twins.programming.kicks-ass.net> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161115141909.GJ27541@tardis.cn.ibm.com> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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, > > > > > > 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 are 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) > > > > > > atomic_cmpxchg_release(&a, 1, 2); > > > r1 = ll(&a) > > > if (r1 == 1) { > > > sc_release(&a, 2); > > > } > > > > > > free() > > > > > > could be reordered as, I think: > > > > > > atomic_cmpxchg_release(&a, 1, 2); > > > r1 = ll(&a) > > > if (r1 == 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_*(). > > > > Can't happen I think because of the control dependency between > > dec_and_test() and free(). > > > > 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(). > > > > 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(). 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. This is because all RmW on a specific variable, irrespective of their memory ordering on other loads/stores, are totally ordered against one another. > But as I said, we actually only need the pairing of orderings: > > 1) load part of cmpxchg -> free() > 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? I would want to further quality 1), it must be no earlier than the load of the last / successful ll/sc round. 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. If we agree on this, I'll update the comment :-) Will, do you too agree?