[dm-devel] [PATCH 2/6] multipathd: track waiters for mutex_lock
Martin Wilck
martin.wilck at suse.com
Tue Aug 16 20:34:33 UTC 2022
On Tue, 2022-08-16 at 13:34 -0500, Benjamin Marzinski wrote:
> On Fri, Aug 12, 2022 at 03:11:54PM +0000, Martin Wilck wrote:
> > On Sat, 2022-07-30 at 00:12 -0500, Benjamin Marzinski wrote:
> > > use the uatomic operations to track how many threads are waiting
> > > in
> > > lock() for mutex_locks. This will be used by a later patch.
> > >
> > > Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> > > ---
> > > libmultipath/lock.h | 16 ++++++++++++++++
> > > multipathd/main.c | 2 +-
> > > 2 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libmultipath/lock.h b/libmultipath/lock.h
> > > index d7b779e7..20ca77e6 100644
> > > --- a/libmultipath/lock.h
> > > +++ b/libmultipath/lock.h
> > > @@ -2,17 +2,28 @@
> > > #define _LOCK_H
> > >
> > > #include <pthread.h>
> > > +#include <urcu/uatomic.h>
> > > +#include <stdbool.h>
> > >
> > > typedef void (wakeup_fn)(void);
> > >
> > > struct mutex_lock {
> > > pthread_mutex_t mutex;
> > > wakeup_fn *wakeup;
> > > + int waiters; /* uatomic access only */
> > > };
> > >
> > > +static inline void init_lock(struct mutex_lock *a)
> > > +{
> > > + pthread_mutex_init(&a->mutex, NULL);
> > > + uatomic_set(&a->waiters, 0);
> > > +}
> > > +
> > > static inline void lock(struct mutex_lock *a)
> > > {
> > > + uatomic_inc(&a->waiters);
> > > pthread_mutex_lock(&a->mutex);
> > > + uatomic_dec(&a->waiters);
> > > }
> > >
> >
> > According to
> > https://github.com/urcu/userspace-rcu/blob/master/doc/uatomic-api.md
> > ,
> > uatomic_inc() and uatomic_dec() require additional memory barriers.
> > The pthread_mutex_lock() implies a barrier, but I'm unsure if it's
> > a
> > full barrier or has ACQUIRE semantics only (which would mean that
> > an
> > uatomic_inc() on another CPU that happens before our uatomic_inc()
> > here
> > might be observed after our uatomic_dec()). In any case I think we
> > need
> > barriers before the uatomic_inc() and after the uatomic_dec().
> >
> > If you want to be on the safe side, you could use
> > uatomic_add_return().
>
> You're gonna have to walk me through this one, because I'm missing
> the
> problem.
I won't claim that I'm an expert. I was just looking at the docs.
https://github.com/urcu/userspace-rcu/blob/master/doc/uatomic-api.md
I hope I won't make a fool out of myself with what I say below.
> First I can't believe that pthread_mutex_lock would allow any
> reads or write to be reordered to either side of it. The compiler
> doesn't know what it's protecting, and so reordering anything through
> it could cause values to be set or read outside of the lock. right?
I haven't found a conclusive answer to this wrt pthread_mutex_lock().
https://pubs.opengroup.org/onlinepubs/9699919799/ says that
pthread_mutex lock is one of the functions that "synchronize memory
with respect to other threads", but doesn't elaborate what this exactly
means. Both memory_barriers.txt and the C++ standard basically just say
that lock() synchronizes with a preceding unlock() (iow, a thread that
completes lock() sees all memory operations that happened before the
previous unlock(). IIUC, if this was the case, it could happen that the
effect of the uatomic_inc() would be observed by other threads after
the lock was already taken. I assume that this is also the semantics of
pthread_mutex_lock(), because additional synchronization guarantees
would harm performance of the mutex operatoins on some architectures,
while not providing additional safety for correct lock()/unlock()
pairs.
> Also, while I'm totally willing to believe that the atomic_inc/dec
> will
> not guarantee that memory accesses don't get reordered around them, I
> can't believe that two seperate racing threads could each believe
> that
> a->waiters was 0 and they incremented it to 1. That race is exactly
> the
> thing that atomic_inc is supposed to protect against.
I'm unsure. "atomic" by itself means only that "no concurrent operation
that reads from addr will see partial effects". I believe the following
would not be impossible:
Thread A Thread B
inc() 0 -> 1
inc() 0 -> 1
lock()
dec() 1 -> 0
unlock()
lock()
dec() -> 0 -> -1
>
> So, since (I believe) a thread's increment must be globally visible
> by
> the time the thread has finished acquiring the mutex,
This is what I'm uncertain about.
> and the dec must
> be globally visible by the time the thread has finished dropping the
> mutex, the question is, what bad thing can happen? Well, a thread
> that
> is checking the state of a->waiters while holding the mutex might not
> see that another thread is about to attempt to acquire the mutex.
> Conversely, it might think that another thread is about to attempt to
> acquire the mutex, but that thread still has some memory accesses to
> complete before it will actually try. The result of this would be
> that
> the checker thread didn't yield the lock when it should (but had the
> timing changed by less than a millisecond it would have been correct
> to
> not yield the lock). Conversely, the checker thread may yield the
> lock
> when there isn't yet any other thread about to grab it (but had the
> timing change by less than a millisecond, it would have been correct
> to
> yield the lock).
>
> These seem like pretty insignificant problems to me (if they even
> reach
> the level of problems), and not really worth adding two new full
> memory
> barriers to deal with.
You don't have to. IIUC, liburcu will do the right thing if you use
cmm_smp_mb__after_uatomic_inc() etc., that is, emit a noop if the
architecture guarantees a barrier anyway.
> Again, I'm not asserting that I'm definitely
> right here.
Neither am I. If someone with deeper knowledge can assert that your
code is safe (I suppose it'd be sufficient to assert that the counter
can never become negative on any architecture), I'm fine.
> Dealing with compiler/hardware reorders makes my head hurt,
> and I'm completely willing to be convinced that I'm overlooking
> something important.
Same here. Just trying to be on the safe side.
Martin
>
> -Ben
> >
> > Regards
> > Martin
> >
> >
> > >
> > > static inline int trylock(struct mutex_lock *a)
> > > @@ -30,6 +41,11 @@ static inline void __unlock(struct mutex_lock
> > > *a)
> > > pthread_mutex_unlock(&a->mutex);
> > > }d
> > >
> > > +static inline bool lock_has_waiters(struct mutex_lock *a)
> > > +{
> > > + return (uatomic_read(&a->waiters) > 0);
> > > +}
> > > +
> > > #define lock_cleanup_pop(a) pthread_cleanup_pop(1)
> > >
> > > void cleanup_lock (void * data);
> > > diff --git a/multipathd/main.c b/multipathd/main.c
> > > index 2f2b9d4c..71079113 100644
> > > --- a/multipathd/main.c
> > > +++ b/multipathd/main.c
> > > @@ -2940,7 +2940,7 @@ init_vecs (void)
> > > if (!vecs)
> > > return NULL;
> > >
> > > - pthread_mutex_init(&vecs->lock.mutex, NULL);
> > > + init_lock(&vecs->lock);
> > >
> > > return vecs;
> > > }
>
More information about the dm-devel
mailing list