Messages in this thread | | | Date | Tue, 15 Nov 2016 14:01:54 +0100 | From | Peter Zijlstra <> | Subject | Re: [RFC][PATCH 7/7] kref: Implement using refcount_t |
| |
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().
So I don't think we can get free() (which very much includes stores) to happen before the store-release.
| |