Skip to content

Commit

Permalink
MDEV-24167 fixup: Avoid hangs in SRW_LOCK_DUMMY
Browse files Browse the repository at this point in the history
In commit 1fdc161 we introduced
a mutex-and-condition-variable based fallback implementation
for platforms that lack a futex system call. That implementation
is prone to hangs.

Let us use separate condition variables for shared and exclusive requests.
  • Loading branch information
dr-m committed Dec 3, 2020
1 parent a13fac9 commit 260161f
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 25 deletions.
12 changes: 8 additions & 4 deletions storage/innobase/include/srw_lock.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ class srw_lock_low final : private rw_lock
#endif
#ifdef SRW_LOCK_DUMMY
pthread_mutex_t mutex;
pthread_cond_t cond;
pthread_cond_t cond_shared;
pthread_cond_t cond_exclusive;
#endif
/** @return pointer to the lock word */
rw_lock *word() { return static_cast<rw_lock*>(this); }
Expand All @@ -46,11 +47,14 @@ class srw_lock_low final : private rw_lock
void write_lock();
/** Wait for signal
@param l lock word from a failed acquisition */
inline void wait(uint32_t l);
inline void writer_wait(uint32_t l);
/** Wait for signal
@param l lock word from a failed acquisition */
inline void readers_wait(uint32_t l);
/** Send signal to one waiter */
inline void wake_one();
inline void writer_wake();
/** Send signal to all waiters */
inline void wake_all();
inline void readers_wake();
public:
#ifdef SRW_LOCK_DUMMY
void init();
Expand Down
58 changes: 37 additions & 21 deletions storage/innobase/sync/srw_lock.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,48 +24,60 @@ void srw_lock_low::init()
{
DBUG_ASSERT(!is_locked_or_waiting());
pthread_mutex_init(&mutex, nullptr);
pthread_cond_init(&cond, nullptr);
pthread_cond_init(&cond_shared, nullptr);
pthread_cond_init(&cond_exclusive, nullptr);
}

void srw_lock_low::destroy()
{
DBUG_ASSERT(!is_locked_or_waiting());
pthread_mutex_destroy(&mutex);
pthread_cond_destroy(&cond);
pthread_cond_destroy(&cond_shared);
pthread_cond_destroy(&cond_exclusive);
}

inline void srw_lock_low::wait(uint32_t l)
inline void srw_lock_low::writer_wait(uint32_t l)
{
pthread_mutex_lock(&mutex);
if (value() == l)
pthread_cond_wait(&cond, &mutex);
pthread_cond_wait(&cond_exclusive, &mutex);
pthread_mutex_unlock(&mutex);
}

inline void srw_lock_low::wake_one()
inline void srw_lock_low::readers_wait(uint32_t l)
{
pthread_mutex_lock(&mutex);
pthread_cond_signal(&cond);
if (value() == l)
pthread_cond_wait(&cond_shared, &mutex);
pthread_mutex_unlock(&mutex);
}

inline void srw_lock_low::wake_all()
inline void srw_lock_low::writer_wake()
{
pthread_mutex_lock(&mutex);
pthread_cond_broadcast(&cond);
uint32_t l= value();
if (l & WRITER)
DBUG_ASSERT(!(l & ~WRITER_PENDING));
else
{
pthread_cond_broadcast(&cond_exclusive);
if (!(l & WRITER_PENDING))
pthread_cond_broadcast(&cond_shared);
}
pthread_mutex_unlock(&mutex);
}
# define readers_wake writer_wake
#else
static_assert(4 == sizeof(rw_lock), "ABI");
# ifdef _WIN32
# include <synchapi.h>

inline void srw_lock_low::wait(uint32_t l)
inline void srw_lock_low::writer_wait(uint32_t l)
{
WaitOnAddress(word(), &l, 4, INFINITE);
}
inline void srw_lock_low::wake_one() { WakeByAddressSingle(word()); }
inline void srw_lock_low::wake_all() { WakeByAddressAll(word()); }
inline void srw_lock_low::writer_wake() { WakeByAddressSingle(word()); }
inline void srw_lock_low::readers_wake() { WakeByAddressAll(word()); }
# else
# ifdef __linux__
# include <linux/futex.h>
Expand All @@ -81,10 +93,14 @@ inline void srw_lock_low::wake_all() { WakeByAddressAll(word()); }
# error "no futex support"
# endif

inline void srw_lock_low::wait(uint32_t l) { SRW_FUTEX(word(), WAIT, l); }
inline void srw_lock_low::wake_one() { SRW_FUTEX(word(), WAKE, 1); }
inline void srw_lock_low::wake_all() { SRW_FUTEX(word(), WAKE, INT_MAX); }
inline void srw_lock_low::writer_wait(uint32_t l)
{
SRW_FUTEX(word(), WAIT, l);
}
inline void srw_lock_low::writer_wake() { SRW_FUTEX(word(), WAKE, 1); }
inline void srw_lock_low::readers_wake() { SRW_FUTEX(word(), WAKE, INT_MAX); }
# endif
# define readers_wait writer_wait
#endif

/** Wait for a read lock.
Expand All @@ -99,15 +115,15 @@ void srw_lock_low::read_lock(uint32_t l)
#ifdef SRW_LOCK_DUMMY
pthread_mutex_lock(&mutex);
{
pthread_cond_signal(&cond);
pthread_cond_wait(&cond, &mutex);
pthread_cond_signal(&cond_exclusive);
pthread_cond_wait(&cond_shared, &mutex);
l= value();
}
while (l == WRITER_WAITING);
pthread_mutex_unlock(&mutex);
continue;
#else
wake_one();
writer_wake();
#endif
}
else
Expand All @@ -120,7 +136,7 @@ void srw_lock_low::read_lock(uint32_t l)
goto wake_writer;
}

wait(l);
readers_wait(l);
}
while (!read_trylock(l));
}
Expand Down Expand Up @@ -153,10 +169,10 @@ void srw_lock_low::write_lock()
else
DBUG_ASSERT(~WRITER_WAITING & l);

wait(l);
writer_wait(l);
}
}

void srw_lock_low::rd_unlock() { if (read_unlock()) wake_one(); }
void srw_lock_low::rd_unlock() { if (read_unlock()) writer_wake(); }

void srw_lock_low::wr_unlock() { write_unlock(); wake_all(); }
void srw_lock_low::wr_unlock() { write_unlock(); readers_wake(); }

0 comments on commit 260161f

Please sign in to comment.