| Message ID | 20170420120058.28404-14-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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"? > + 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 > > >
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. 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 >> >> >>
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 >>> >>> >>> > >
On Thu, Apr 20, 2017 at 02:00:54PM +0200, Paolo Bonzini wrote: > This primitive lets you lock/unlock a CoMutex, guaranteeing neither > blocking nor cacheline bouncing if there is no qemu_co_mutex_lock > critical section. "Guaranteeing neither X nor Y" means "does not guarantee X or Y". Did you mean "Guarantees not X and not Y"? > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > include/qemu/coroutine.h | 6 ++++++ > util/qemu-coroutine-lock.c | 36 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 42 insertions(+) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 04/05/2017 16:12, Stefan Hajnoczi wrote: >> This primitive lets you lock/unlock a CoMutex, guaranteeing neither >> blocking nor cacheline bouncing if there is no qemu_co_mutex_lock >> critical section. > "Guaranteeing neither X nor Y" means "does not guarantee X or Y". Did > you mean "Guarantees not X and not Y"? Oops, thanks---I didn't know this! "Guaranteeing no blocking and no cacheline bouncing if there is no concurrent critical section". Paolo
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index a4509bd..8d4416c 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -157,6 +157,12 @@ void qemu_co_mutex_init(CoMutex *mutex); void coroutine_fn qemu_co_mutex_lock(CoMutex *mutex); /** + * Locks the mutex and immediately unlock it. This is faster than back-to-back + * lock/unlock if the mutex is not taken by anyone. + */ +void coroutine_fn qemu_co_mutex_lock_unlock(CoMutex *mutex); + +/** * Unlocks the mutex and schedules the next coroutine that was waiting for this * lock to be run. */ diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c index 6328eed..86f56cd 100644 --- a/util/qemu-coroutine-lock.c +++ b/util/qemu-coroutine-lock.c @@ -287,6 +287,42 @@ retry_fast_path: self->locks_held++; } +void coroutine_fn qemu_co_mutex_lock_unlock(CoMutex *mutex) +{ + AioContext *ctx = qemu_get_current_aio_context(); + int waiters, i; + +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); +} + void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex) { Coroutine *self = qemu_coroutine_self();
This primitive lets you lock/unlock a CoMutex, guaranteeing neither blocking nor cacheline bouncing if there is no qemu_co_mutex_lock critical section. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- include/qemu/coroutine.h | 6 ++++++ util/qemu-coroutine-lock.c | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+)