Re: [PATCH 2/2] [PATCH] sched: Add smp_rmb() in task rq locking cycles

[Manfred Spraul <manfred@colorfullife.com>]

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