Re: [Qemu-block] [Qemu-devel] [PATCH 13/17] coroutine-lock: introduce qemu_co_mutex_lock_unlock

[Paolo Bonzini]

On 04/05/2017 11:47, Paolo Bonzini wrote:
> 
> 
> On 04/05/2017 09:39, Fam Zheng wrote:
>> On Thu, 04/20 14:00, Paolo Bonzini wrote:
>>> +    if (atomic_cmpxchg(&mutex->locked, waiters, waiters + 1) != waiters) {
>>
>> Is it still useful to try the fast path again if there are now even more
>> waiters, i.e.  "atomic_cmpxchg(...) > waiters"?
> 
> Probably not.

... but when this happens, we don't enter the fast path loop:

 retry_fast_path:
    waiters = atomic_read(&mutex->locked);
    if (waiters == 0) {
        /* Provide same memory ordering semantics as mutex lock/unlock.  */
        smp_mb_acquire();
        smp_mb_release();
        return;
    }

    i = 0;
    while (waiters == 1 && ++i < 1000) {
        if (atomic_read(&mutex->ctx) == ctx) {
            break;
        }
        waiters = atomic_read(&mutex->locked);
        if (waiters == 0) {
            smp_mb_acquire();
            smp_mb_release();
            return;
        }
        cpu_relax();
    }

    if (atomic_cmpxchg(&mutex->locked, waiters, waiters + 1) != waiters) {
        goto retry_fast_path;
    }

    qemu_co_mutex_lock_slowpath(ctx, mutex);
    qemu_co_mutex_unlock(mutex);

The "if (waiters == 0)" fails, and the "while (waiters == 1 && ...)" won't
happen either if atomic_cmpxchg returns > 1.  So really what you get is
a retry of the atomic_cmpxchg.

We should introduce atomic_try_cmpxchg.  Linux added it recently, too,
and it simplifies the code because you don't need to redo the atomic_read.
Like this:

    waiters = atomic_read(&mutex->locked);
 retry_fast_path:
    if (waiters == 0) {
        ...
    }
    i = 0;
    while (waiters == 1 && ++i < 1000) {
        ...
    }
    if (!atomic_cmpxchg(&mutex->locked, &waiters, waiters + 1)) {
        goto retry_fast_path;
    }

    qemu_co_mutex_lock_slowpath(ctx, mutex);
    qemu_co_mutex_unlock(mutex);


Paolo


> Paolo
> 
>>> +        goto retry_fast_path;
>>> +    }
>>> +
>>> +    qemu_co_mutex_lock_slowpath(ctx, mutex);
>>> +    qemu_co_mutex_unlock(mutex);
>>> +}
>>> +
>>>  void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex)
>>>  {
>>>      Coroutine *self = qemu_coroutine_self();
>>> -- 
>>> 2.9.3
>>>
>>>
>>>
> 
>