Re: [PATCH -mm -next] ipc,sem: fix lockdep false positive

[Rik van Riel <riel@surriel.com>]

On 03/27/2013 04:42 AM, Peter Zijlstra wrote:
> On Tue, 2013-03-26 at 11:19 -0400, Rik van Riel wrote:
>>> Maybe something like:
>>>
>>> void sma_lock(struct sem_array *sma) /* global */
>>> {
>>>        int i;
>>>
>>>        sma->global_locked = 1;
>>>        smp_wmb(); /* can we merge with the LOCK ? */
>>>        spin_lock(&sma->global_lock);
>>>
>>>        /* wait for all local locks to go away */
>>>        for (i = 0; i < sma->sem_nsems; i++)
>>>                spin_unlock_wait(&sem->sem_base[i]->lock);
>>> }
>>>
>>> void sma_lock_one(struct sem_array *sma, int nr) /* local */
>>> {
>>>        smp_rmb(); /* pairs with wmb in sma_lock() */
>>>        if (unlikely(sma->global_locked)) { /* wait for global lock */
>>>                while (sma->global_locked)
>>>                        spin_unlock_wait(&sma->global_lock);
>>>        }
>>>        spin_lock(&sma->sem_base[nr]->lock);
>>> }
>
> I since realized there's an ordering problem with ->global_locked, we
> need to use spin_is_locked() or somesuch.
>
> Two competing sma_lock() operations will screw over the separate
> variable.

There may be another problem with your idea.

If there are two single locks coming in for the same semaphore,
the first one holds the lock, while the second one is spinning
on the lock.

The global lock is spinning on spin_unlock_wait, which may end
up finishing after the first single lock holder unlocks, right
before the second single lock holder grabs the lock.

At that point, you have both a process that thinks it holds the
global lock, and a process that thinks it holds a single lock.

To prevent against this, the single lock probably needs to test
whether the global lock is locked, after it acquires the local
lock.

If the global lock is locked, it needs to unlock its single lock,
and then do a spin_unlock_wait on the global lock, before trying
again from the start.

Would that work?