Messages in this thread | | | Date | Thu, 6 Oct 2016 15:30:15 -0400 | From | Waiman Long <> | Subject | Re: [RFC PATCH-tip v4 01/10] locking/osq: Make lock/unlock proper acquire/release barrier |
| |
On 10/06/2016 01:47 AM, Davidlohr Bueso wrote: > On Wed, 05 Oct 2016, Waiman Long wrote: > >> diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c >> index 05a3785..1e6823a 100644 >> --- a/kernel/locking/osq_lock.c >> +++ b/kernel/locking/osq_lock.c >> @@ -12,6 +12,23 @@ >> */ >> static DEFINE_PER_CPU_SHARED_ALIGNED(struct optimistic_spin_node, >> osq_node); >> >> +enum mbtype { >> + acquire, >> + release, >> + relaxed, >> +}; > > No, please. > >> + >> +static __always_inline int >> +_atomic_cmpxchg_(const enum mbtype barrier, atomic_t *v, int old, >> int new) >> +{ >> + if (barrier == acquire) >> + return atomic_cmpxchg_acquire(v, old, new); >> + else if (barrier == release) >> + return atomic_cmpxchg_release(v, old, new); >> + else >> + return atomic_cmpxchg_relaxed(v, old, new); >> +} > > Things like the above are icky. How about something like below? I'm not > crazy about it, but there are other similar macros, ie lockref. We still > provide the osq_lock/unlock to imply acquire/release and the new _relaxed > flavor, as I agree that should be the correct naming > > While I have not touched osq_wait_next(), the following are impacted: > > - node->locked is now completely without ordering for _relaxed() > (currently > its under smp_load_acquire, which does not match and the race is harmless > to begin with as we just iterate again. For the acquire flavor, it is > always > formed with ctr dep + smp_rmb(). > > - If osq_lock() fails we never guarantee any ordering. > > What do you think? > > Thanks, > Davidlohr
Yes, I am OK with your change. However, I need some additional changes in osq_wait_next() as well. Either it is changed to use the release variants of atomic_cmpxchg and xchg or using macro like what you did with osq_lock and osq_unlock. The release variant is needed in the osq_lock(). As osq_wait_next() is only invoked in the failure path of osq_lock(), the barrier type doesn't really matter.
Cheers, Longman
| |