| Message ID | 1446044465-19312-2-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Oct 28, 2015 at 06:01:02PM +0300, Denis V. Lunev wrote: > +int rfifolock_is_locked(RFifoLock *r); Please use bool instead of int. > diff --git a/util/rfifolock.c b/util/rfifolock.c > index afbf748..8ac58cb 100644 > --- a/util/rfifolock.c > +++ b/util/rfifolock.c > @@ -48,7 +48,7 @@ void rfifolock_lock(RFifoLock *r) > /* Take a ticket */ > unsigned int ticket = r->tail++; > > - if (r->nesting > 0 && qemu_thread_is_self(&r->owner_thread)) { > + if (r->nesting > 0 && rfifolock_is_locked(r)) { > r->tail--; /* put ticket back, we're nesting */ > } else { > while (ticket != r->head) { > @@ -69,10 +69,15 @@ void rfifolock_unlock(RFifoLock *r) > { > qemu_mutex_lock(&r->lock); > assert(r->nesting > 0); > - assert(qemu_thread_is_self(&r->owner_thread)); > + assert(rfifolock_is_locked(r)); > if (--r->nesting == 0) { > r->head++; > qemu_cond_broadcast(&r->cond); > } > qemu_mutex_unlock(&r->lock); > } > + > +int rfifolock_is_locked(RFifoLock *r) > +{ > + return qemu_thread_is_self(&r->owner_thread); > +} The function name confused me since "does the current thread hold the lock?" != "does anyone currently hold the lock?". I suggest: bool rfifolock_held_by_current_thread(RFifoLock *r) { return r->nesting > 0 && qemu_thread_is_self(&r->owner_thread); } Then the r->nesting > 0 testing can also be dropped by callers, which is good since rfifolock_is_locked() does not return a meaningful result when r->nesting == 0.
On 10/30/2015 06:41 PM, Stefan Hajnoczi wrote: > On Wed, Oct 28, 2015 at 06:01:02PM +0300, Denis V. Lunev wrote: >> +int rfifolock_is_locked(RFifoLock *r); > Please use bool instead of int. > >> diff --git a/util/rfifolock.c b/util/rfifolock.c >> index afbf748..8ac58cb 100644 >> --- a/util/rfifolock.c >> +++ b/util/rfifolock.c >> @@ -48,7 +48,7 @@ void rfifolock_lock(RFifoLock *r) >> /* Take a ticket */ >> unsigned int ticket = r->tail++; >> >> - if (r->nesting > 0 && qemu_thread_is_self(&r->owner_thread)) { >> + if (r->nesting > 0 && rfifolock_is_locked(r)) { >> r->tail--; /* put ticket back, we're nesting */ >> } else { >> while (ticket != r->head) { >> @@ -69,10 +69,15 @@ void rfifolock_unlock(RFifoLock *r) >> { >> qemu_mutex_lock(&r->lock); >> assert(r->nesting > 0); >> - assert(qemu_thread_is_self(&r->owner_thread)); >> + assert(rfifolock_is_locked(r)); >> if (--r->nesting == 0) { >> r->head++; >> qemu_cond_broadcast(&r->cond); >> } >> qemu_mutex_unlock(&r->lock); >> } >> + >> +int rfifolock_is_locked(RFifoLock *r) >> +{ >> + return qemu_thread_is_self(&r->owner_thread); >> +} > The function name confused me since "does the current thread hold the > lock?" != "does anyone currently hold the lock?". > > I suggest: > > bool rfifolock_held_by_current_thread(RFifoLock *r) { > return r->nesting > 0 && qemu_thread_is_self(&r->owner_thread); > } > > Then the r->nesting > 0 testing can also be dropped by callers, which is > good since rfifolock_is_locked() does not return a meaningful result > when r->nesting == 0. what about rfifolock_is_owner() ? Den
On 10/30/2015 06:41 PM, Stefan Hajnoczi wrote: > On Wed, Oct 28, 2015 at 06:01:02PM +0300, Denis V. Lunev wrote: >> +int rfifolock_is_locked(RFifoLock *r); > Please use bool instead of int. > >> diff --git a/util/rfifolock.c b/util/rfifolock.c >> index afbf748..8ac58cb 100644 >> --- a/util/rfifolock.c >> +++ b/util/rfifolock.c >> @@ -48,7 +48,7 @@ void rfifolock_lock(RFifoLock *r) >> /* Take a ticket */ >> unsigned int ticket = r->tail++; >> >> - if (r->nesting > 0 && qemu_thread_is_self(&r->owner_thread)) { >> + if (r->nesting > 0 && rfifolock_is_locked(r)) { >> r->tail--; /* put ticket back, we're nesting */ >> } else { >> while (ticket != r->head) { >> @@ -69,10 +69,15 @@ void rfifolock_unlock(RFifoLock *r) >> { >> qemu_mutex_lock(&r->lock); >> assert(r->nesting > 0); >> - assert(qemu_thread_is_self(&r->owner_thread)); >> + assert(rfifolock_is_locked(r)); >> if (--r->nesting == 0) { >> r->head++; >> qemu_cond_broadcast(&r->cond); >> } >> qemu_mutex_unlock(&r->lock); >> } >> + >> +int rfifolock_is_locked(RFifoLock *r) >> +{ >> + return qemu_thread_is_self(&r->owner_thread); >> +} > The function name confused me since "does the current thread hold the > lock?" != "does anyone currently hold the lock?". > > I suggest: > > bool rfifolock_held_by_current_thread(RFifoLock *r) { > return r->nesting > 0 && qemu_thread_is_self(&r->owner_thread); > } > > Then the r->nesting > 0 testing can also be dropped by callers, which is > good since rfifolock_is_locked() does not return a meaningful result > when r->nesting == 0. actually the function is broken in the current state: aio_context_acquire() aio_context_release() aio_context_is_locked() will return true. the problem is that owner thread is not reset on lock release. with your proposal the function become racy if called from outer thread. I need to think a bit here. May be we'll need 2 versions - locked and unlocked or something other should be added. I am in progress of invention :) Den
On Fri, Oct 30, 2015 at 11:30:04PM +0300, Denis V. Lunev wrote: > On 10/30/2015 06:41 PM, Stefan Hajnoczi wrote: > >On Wed, Oct 28, 2015 at 06:01:02PM +0300, Denis V. Lunev wrote: > >>+int rfifolock_is_locked(RFifoLock *r); > >Please use bool instead of int. > > > >>diff --git a/util/rfifolock.c b/util/rfifolock.c > >>index afbf748..8ac58cb 100644 > >>--- a/util/rfifolock.c > >>+++ b/util/rfifolock.c > >>@@ -48,7 +48,7 @@ void rfifolock_lock(RFifoLock *r) > >> /* Take a ticket */ > >> unsigned int ticket = r->tail++; > >>- if (r->nesting > 0 && qemu_thread_is_self(&r->owner_thread)) { > >>+ if (r->nesting > 0 && rfifolock_is_locked(r)) { > >> r->tail--; /* put ticket back, we're nesting */ > >> } else { > >> while (ticket != r->head) { > >>@@ -69,10 +69,15 @@ void rfifolock_unlock(RFifoLock *r) > >> { > >> qemu_mutex_lock(&r->lock); > >> assert(r->nesting > 0); > >>- assert(qemu_thread_is_self(&r->owner_thread)); > >>+ assert(rfifolock_is_locked(r)); > >> if (--r->nesting == 0) { > >> r->head++; > >> qemu_cond_broadcast(&r->cond); > >> } > >> qemu_mutex_unlock(&r->lock); > >> } > >>+ > >>+int rfifolock_is_locked(RFifoLock *r) > >>+{ > >>+ return qemu_thread_is_self(&r->owner_thread); > >>+} > >The function name confused me since "does the current thread hold the > >lock?" != "does anyone currently hold the lock?". > > > >I suggest: > > > >bool rfifolock_held_by_current_thread(RFifoLock *r) { > > return r->nesting > 0 && qemu_thread_is_self(&r->owner_thread); > >} > > > >Then the r->nesting > 0 testing can also be dropped by callers, which is > >good since rfifolock_is_locked() does not return a meaningful result > >when r->nesting == 0. > what about > > rfifolock_is_owner() ? Great! Stefan
On Sun, Nov 01, 2015 at 04:55:51PM +0300, Denis V. Lunev wrote: > On 10/30/2015 06:41 PM, Stefan Hajnoczi wrote: > >On Wed, Oct 28, 2015 at 06:01:02PM +0300, Denis V. Lunev wrote: > >>+int rfifolock_is_locked(RFifoLock *r); > >Please use bool instead of int. > > > >>diff --git a/util/rfifolock.c b/util/rfifolock.c > >>index afbf748..8ac58cb 100644 > >>--- a/util/rfifolock.c > >>+++ b/util/rfifolock.c > >>@@ -48,7 +48,7 @@ void rfifolock_lock(RFifoLock *r) > >> /* Take a ticket */ > >> unsigned int ticket = r->tail++; > >>- if (r->nesting > 0 && qemu_thread_is_self(&r->owner_thread)) { > >>+ if (r->nesting > 0 && rfifolock_is_locked(r)) { > >> r->tail--; /* put ticket back, we're nesting */ > >> } else { > >> while (ticket != r->head) { > >>@@ -69,10 +69,15 @@ void rfifolock_unlock(RFifoLock *r) > >> { > >> qemu_mutex_lock(&r->lock); > >> assert(r->nesting > 0); > >>- assert(qemu_thread_is_self(&r->owner_thread)); > >>+ assert(rfifolock_is_locked(r)); > >> if (--r->nesting == 0) { > >> r->head++; > >> qemu_cond_broadcast(&r->cond); > >> } > >> qemu_mutex_unlock(&r->lock); > >> } > >>+ > >>+int rfifolock_is_locked(RFifoLock *r) > >>+{ > >>+ return qemu_thread_is_self(&r->owner_thread); > >>+} > >The function name confused me since "does the current thread hold the > >lock?" != "does anyone currently hold the lock?". > > > >I suggest: > > > >bool rfifolock_held_by_current_thread(RFifoLock *r) { > > return r->nesting > 0 && qemu_thread_is_self(&r->owner_thread); > >} > > > >Then the r->nesting > 0 testing can also be dropped by callers, which is > >good since rfifolock_is_locked() does not return a meaningful result > >when r->nesting == 0. > actually the function is broken in the current state: > aio_context_acquire() > aio_context_release() > aio_context_is_locked() will return true. > the problem is that owner thread is not reset on lock release. The owner thread field is only valid when nesting > 0. > with your proposal the function become racy if called from outer > thread. I need to think a bit here. This is thread-safe: bool owner; qemu_mutex_lock(&r->lock); owner = r->nesting > 0 && qemu_thread_is_self(&r->owner_thread); qemu_mutex_unlock(&r->lock); return owner;
On 11/02/2015 04:12 PM, Stefan Hajnoczi wrote: > On Sun, Nov 01, 2015 at 04:55:51PM +0300, Denis V. Lunev wrote: >> On 10/30/2015 06:41 PM, Stefan Hajnoczi wrote: >>> On Wed, Oct 28, 2015 at 06:01:02PM +0300, Denis V. Lunev wrote: >>>> +int rfifolock_is_locked(RFifoLock *r); >>> Please use bool instead of int. >>> >>>> diff --git a/util/rfifolock.c b/util/rfifolock.c >>>> index afbf748..8ac58cb 100644 >>>> --- a/util/rfifolock.c >>>> +++ b/util/rfifolock.c >>>> @@ -48,7 +48,7 @@ void rfifolock_lock(RFifoLock *r) >>>> /* Take a ticket */ >>>> unsigned int ticket = r->tail++; >>>> - if (r->nesting > 0 && qemu_thread_is_self(&r->owner_thread)) { >>>> + if (r->nesting > 0 && rfifolock_is_locked(r)) { >>>> r->tail--; /* put ticket back, we're nesting */ >>>> } else { >>>> while (ticket != r->head) { >>>> @@ -69,10 +69,15 @@ void rfifolock_unlock(RFifoLock *r) >>>> { >>>> qemu_mutex_lock(&r->lock); >>>> assert(r->nesting > 0); >>>> - assert(qemu_thread_is_self(&r->owner_thread)); >>>> + assert(rfifolock_is_locked(r)); >>>> if (--r->nesting == 0) { >>>> r->head++; >>>> qemu_cond_broadcast(&r->cond); >>>> } >>>> qemu_mutex_unlock(&r->lock); >>>> } >>>> + >>>> +int rfifolock_is_locked(RFifoLock *r) >>>> +{ >>>> + return qemu_thread_is_self(&r->owner_thread); >>>> +} >>> The function name confused me since "does the current thread hold the >>> lock?" != "does anyone currently hold the lock?". >>> >>> I suggest: >>> >>> bool rfifolock_held_by_current_thread(RFifoLock *r) { >>> return r->nesting > 0 && qemu_thread_is_self(&r->owner_thread); >>> } >>> >>> Then the r->nesting > 0 testing can also be dropped by callers, which is >>> good since rfifolock_is_locked() does not return a meaningful result >>> when r->nesting == 0. >> actually the function is broken in the current state: >> aio_context_acquire() >> aio_context_release() >> aio_context_is_locked() will return true. >> the problem is that owner thread is not reset on lock release. > The owner thread field is only valid when nesting > 0. > >> with your proposal the function become racy if called from outer >> thread. I need to think a bit here. > This is thread-safe: > > bool owner; > > qemu_mutex_lock(&r->lock); > owner = r->nesting > 0 && qemu_thread_is_self(&r->owner_thread); > qemu_mutex_unlock(&r->lock); > > return owner; yep, I know. But I do not want to take the lock for check. IMHO it would be better to @@ -68,11 +68,16 @@ void rfifolock_lock(RFifoLock *r) void rfifolock_unlock(RFifoLock *r) { qemu_mutex_lock(&r->lock); - assert(r->nesting > 0); - assert(qemu_thread_is_self(&r->owner_thread)); + assert(rfifolock_is_owner(r)); if (--r->nesting == 0) { + qemu_thread_clear(&r->owner_thread); r->head++; qemu_cond_broadcast(&r->cond); } qemu_mutex_unlock(&r->lock); } + +bool rfifolock_is_owner(RFifoLock *r) +{ + return r->nesting > 0 && qemu_thread_is_self(&r->owner_thread); +} which does not require lock and thread safe. Den
On 02/11/2015 14:39, Denis V. Lunev wrote: >> This is thread-safe: >> >> bool owner; >> >> qemu_mutex_lock(&r->lock); >> owner = r->nesting > 0 && qemu_thread_is_self(&r->owner_thread); >> qemu_mutex_unlock(&r->lock); >> >> return owner; > yep, I know. > > But I do not want to take the lock for check. You can use a trylock. If it fails, it is definitely safe to return false. > IMHO it would be better to > > @@ -68,11 +68,16 @@ void rfifolock_lock(RFifoLock *r) > void rfifolock_unlock(RFifoLock *r) > { > qemu_mutex_lock(&r->lock); > - assert(r->nesting > 0); > - assert(qemu_thread_is_self(&r->owner_thread)); > + assert(rfifolock_is_owner(r)); > if (--r->nesting == 0) { > + qemu_thread_clear(&r->owner_thread); > r->head++; > qemu_cond_broadcast(&r->cond); > } > qemu_mutex_unlock(&r->lock); > } > + > +bool rfifolock_is_owner(RFifoLock *r) > +{ > + return r->nesting > 0 && qemu_thread_is_self(&r->owner_thread); > +} > > which does not require lock and thread safe. I think it requires memory barriers though. But it can be simplified: if you clear owner_thread, you do not need to check r->nesting in rfifolock_is_owner. Clearing owner_thread can be done with a simple memset. Paolo
On 11/02/2015 04:55 PM, Paolo Bonzini wrote: > > On 02/11/2015 14:39, Denis V. Lunev wrote: >>> This is thread-safe: >>> >>> bool owner; >>> >>> qemu_mutex_lock(&r->lock); >>> owner = r->nesting > 0 && qemu_thread_is_self(&r->owner_thread); >>> qemu_mutex_unlock(&r->lock); >>> >>> return owner; >> yep, I know. >> >> But I do not want to take the lock for check. > You can use a trylock. If it fails, it is definitely safe to return false. > >> IMHO it would be better to >> >> @@ -68,11 +68,16 @@ void rfifolock_lock(RFifoLock *r) >> void rfifolock_unlock(RFifoLock *r) >> { >> qemu_mutex_lock(&r->lock); >> - assert(r->nesting > 0); >> - assert(qemu_thread_is_self(&r->owner_thread)); >> + assert(rfifolock_is_owner(r)); >> if (--r->nesting == 0) { >> + qemu_thread_clear(&r->owner_thread); >> r->head++; >> qemu_cond_broadcast(&r->cond); >> } >> qemu_mutex_unlock(&r->lock); >> } >> + >> +bool rfifolock_is_owner(RFifoLock *r) >> +{ >> + return r->nesting > 0 && qemu_thread_is_self(&r->owner_thread); >> +} >> >> which does not require lock and thread safe. > I think it requires memory barriers though. But it can be simplified: > if you clear owner_thread, you do not need to check r->nesting in > rfifolock_is_owner. > > Clearing owner_thread can be done with a simple memset. this does not require memory barrier as call to qemu_call_broadcast will do the job just fine. The check for ownership is actually enough as: - current thread could be set in the current thread only and cleared in the same current thread only. This is not racy at all :) - count check is moved here for convenience only by request of Stefan, it is not required at all to make a decision with after clearing the thread. OK, I can use memset for sure if it will be accepted :) This was my first opinion. Den
diff --git a/include/qemu/rfifolock.h b/include/qemu/rfifolock.h index b23ab53..2c00c4b 100644 --- a/include/qemu/rfifolock.h +++ b/include/qemu/rfifolock.h @@ -50,5 +50,6 @@ void rfifolock_init(RFifoLock *r, void (*cb)(void *), void *opaque); void rfifolock_destroy(RFifoLock *r); void rfifolock_lock(RFifoLock *r); void rfifolock_unlock(RFifoLock *r); +int rfifolock_is_locked(RFifoLock *r); #endif /* QEMU_RFIFOLOCK_H */ diff --git a/util/rfifolock.c b/util/rfifolock.c index afbf748..8ac58cb 100644 --- a/util/rfifolock.c +++ b/util/rfifolock.c @@ -48,7 +48,7 @@ void rfifolock_lock(RFifoLock *r) /* Take a ticket */ unsigned int ticket = r->tail++; - if (r->nesting > 0 && qemu_thread_is_self(&r->owner_thread)) { + if (r->nesting > 0 && rfifolock_is_locked(r)) { r->tail--; /* put ticket back, we're nesting */ } else { while (ticket != r->head) { @@ -69,10 +69,15 @@ void rfifolock_unlock(RFifoLock *r) { qemu_mutex_lock(&r->lock); assert(r->nesting > 0); - assert(qemu_thread_is_self(&r->owner_thread)); + assert(rfifolock_is_locked(r)); if (--r->nesting == 0) { r->head++; qemu_cond_broadcast(&r->cond); } qemu_mutex_unlock(&r->lock); } + +int rfifolock_is_locked(RFifoLock *r) +{ + return qemu_thread_is_self(&r->owner_thread); +}
This helper is necessary to ensure locking constraints. Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Stefan Hajnoczi <stefanha@redhat.com> CC: Paolo Bonzini <pbonzini@redhat.com> --- include/qemu/rfifolock.h | 1 + util/rfifolock.c | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-)