Messages in this thread |  | | Date | Wed, 18 Feb 2015 20:14:01 +0100 | From | Manfred Spraul <> | Subject | Re: [PATCH 2/2] [PATCH] sched: Add smp_rmb() in task rq locking cycles |
| |
Hi Oleg,
On 02/18/2015 04:59 PM, Oleg Nesterov wrote: > Let's look at sem_lock(). I never looked at this code before, I can be > easily wrong. Manfred will correct me. But at first glance we can write > the oversimplified pseudo-code: > > spinlock_t local, global; > > bool my_lock(bool try_local) > { > if (try_local) { > spin_lock(&local); > if (!spin_is_locked(&global)) > return true; > spin_unlock(&local); > } > > spin_lock(&global); > spin_unlock_wait(&local); > return false; > } > > void my_unlock(bool drop_local) > { > if (drop_local) > spin_unlock(&local); > else > spin_unlock(&global); > } > > it assumes that the "local" lock is cheaper than "global", the usage is > > bool xxx = my_lock(condition); > /* CRITICAL SECTION */ > my_unlock(xxx); > > Now. Unless I missed something, my_lock() does NOT need a barrier BEFORE > spin_unlock_wait() (or spin_is_locked()). Either my_lock(true) should see > spin_is_locked(global) == T, or my_lock(false)->spin_unlock_wait() should > see that "local" is locked and wait. I would agree: There is no need for a barrier. spin_unlock_read() is just a read, the barriers are from spin_lock() and spin_unlock().
The barrier exist to protect something like a "force_global" flag (complex_count)
> spinlock_t local, global; > bool force_global; > bool my_lock(bool try_local) > { > if (try_local) { > spin_lock(&local); > if (!spin_is_locked(&global)) { > if (!force_global) { > return true; > } > } > spin_unlock(&local); > > > spin_lock(&global); > spin_unlock_wait(&local); > return false; > } > > void my_unlock(bool drop_local) > { > if (drop_local) > spin_unlock(&local); > else > spin_unlock(&global); > } > }
force_global can only be set by the owner of &global.
> Another question is do we need a barrier AFTER spin_unlock_wait(). I do not > know what ipc/sem.c actually needs, but in general (I think) this does need > mb(). Otherwise my_lock / my_unlock itself does not have the proper acq/rel > semantics. For example, my_lock(false) can miss the changes which were done > under my_lock(true). How could that happen? I thought that thread A: protected_var = 1234; spin_unlock(&lock_a)
thread B: spin_lock(&lock_b) if (protected_var) is safe. i.e, there is no need that acquire and releases is done on the same pointer.
-- Manfred
|  |