From b60354d82e3dc0e219cf08e956c94aa2585c104b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Bri=C3=A8re?= Date: Wed, 2 Oct 2019 22:05:01 -0400 Subject: Move t_rwmutex_reader/writer definitions into mutex.cpp These classes are about to get more complex, so let's move them ahead of time into mutex.cpp. --- src/threads/mutex.cpp | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) (limited to 'src/threads/mutex.cpp') diff --git a/src/threads/mutex.cpp b/src/threads/mutex.cpp index 744cd7b..9c6a762 100644 --- a/src/threads/mutex.cpp +++ b/src/threads/mutex.cpp @@ -124,3 +124,31 @@ void t_rwmutex::unlock() { pthread_rwlock_unlock(&_lock); } + +/////////////////////////// +// t_rwmutex_guard +/////////////////////////// + +t_rwmutex_reader::t_rwmutex_reader(t_rwmutex& mutex) : _mutex(mutex) +{ + // std::cout << "mtx rd lock " << (void*)&_mutex << std::endl; + _mutex.lockRead(); +} + +t_rwmutex_reader::~t_rwmutex_reader() +{ + // std::cout << "mtx rd unlock " << (void*)&_mutex << std::endl; + _mutex.unlock(); +} + +t_rwmutex_writer::t_rwmutex_writer(t_rwmutex& mutex) : _mutex(mutex) +{ + // std::cout << "mtx wr lock " << (void*)&_mutex << std::endl; + _mutex.lockWrite(); +} + +t_rwmutex_writer::~t_rwmutex_writer() +{ + // std::cout << "mtx wr unlock " << (void*)&_mutex << std::endl; + _mutex.unlock(); +} -- cgit v1.2.3 From 3d126cd9a7f9029e199e9220e2ffa08ac4e23227 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Bri=C3=A8re?= Date: Wed, 2 Oct 2019 22:13:56 -0400 Subject: Create t_rwmutex_guard base class (Doing this ahead of time to simplify the next commit a bit.) --- src/threads/mutex.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) (limited to 'src/threads/mutex.cpp') diff --git a/src/threads/mutex.cpp b/src/threads/mutex.cpp index 9c6a762..7eaa329 100644 --- a/src/threads/mutex.cpp +++ b/src/threads/mutex.cpp @@ -129,7 +129,13 @@ void t_rwmutex::unlock() // t_rwmutex_guard /////////////////////////// -t_rwmutex_reader::t_rwmutex_reader(t_rwmutex& mutex) : _mutex(mutex) +t_rwmutex_guard::t_rwmutex_guard(t_rwmutex& mutex) : + _mutex(mutex) +{ +} + +t_rwmutex_reader::t_rwmutex_reader(t_rwmutex& mutex) : + t_rwmutex_guard(mutex) { // std::cout << "mtx rd lock " << (void*)&_mutex << std::endl; _mutex.lockRead(); @@ -141,7 +147,8 @@ t_rwmutex_reader::~t_rwmutex_reader() _mutex.unlock(); } -t_rwmutex_writer::t_rwmutex_writer(t_rwmutex& mutex) : _mutex(mutex) +t_rwmutex_writer::t_rwmutex_writer(t_rwmutex& mutex) : + t_rwmutex_guard(mutex) { // std::cout << "mtx wr lock " << (void*)&_mutex << std::endl; _mutex.lockWrite(); -- cgit v1.2.3 From 91de36717a119f9501698af97550bfcdffd2875a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Bri=C3=A8re?= Date: Fri, 27 Dec 2019 02:26:38 -0500 Subject: Introduce read-write-update locks and guards to prevent deadlocks This converts t_rwmutex and t_rwmutex_guard into a read-write-update[*] lock and guard, in an attempt to circumvent the various deadlocks that were introduced with the addition of lines_mtx in 38bb6b7. [*] For more details, see https://stackoverflow.com/a/18785300 and http://lkml.iu.edu/hypermail/linux/kernel/0004.3/0117.html. Note that this is not a real fix; this would require analyzing and refactoring phone.cpp, which is well beyond my abilities. This is at best a workaround that appears to conveniently dodge all the deadlocks I've encountered so far. (It would have been more proper to introduce a separate class for this purpose, but this would have required modifying over 80 lines just to change one type for another. As phone_users_mtx is the only other instance of this class, the impact of subverting t_rwmutex directly is minimal.) --- src/threads/mutex.cpp | 143 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 130 insertions(+), 13 deletions(-) (limited to 'src/threads/mutex.cpp') diff --git a/src/threads/mutex.cpp b/src/threads/mutex.cpp index 7eaa329..4c8c078 100644 --- a/src/threads/mutex.cpp +++ b/src/threads/mutex.cpp @@ -94,7 +94,13 @@ t_mutex_guard::~t_mutex_guard() { // t_rwmutex /////////////////////////// -t_rwmutex::t_rwmutex() +// Equivalent of an invalid thread ID, to be used when initializing t_rwmutex +// or when releasing upgrade ownership; the use of pthread_self() is only to +// provide a dummy value of the appropriate type. +static const optional_pthread_t invalid_thread_id = { false, pthread_self() }; + +t_rwmutex::t_rwmutex() : + _up_mutex_thread( invalid_thread_id ) { int ret = pthread_rwlock_init(&_lock, nullptr); if (ret != 0) throw string( @@ -106,56 +112,167 @@ t_rwmutex::~t_rwmutex() pthread_rwlock_destroy(&_lock); } -void t_rwmutex::lockRead() +void t_rwmutex::getUpgradeOwnership() +{ + _up_mutex.lock(); + _up_mutex_thread = { true, pthread_self() }; +} + +void t_rwmutex::releaseUpgradeOwnership() +{ + _up_mutex_thread = invalid_thread_id; + _up_mutex.unlock(); +} + +bool t_rwmutex::isUpgradeOwnershipOurs() const +{ + // Note that we don't need a mutex over _up_mutex_thread, being atomic + // is enough for our purposes. (We don't care about *who* owns + // _up_mutex, only about whether or not *we* own it, a fact which only + // our own thread can modify.) + optional_pthread_t lockOwner = _up_mutex_thread; + return lockOwner.has_value && pthread_equal(lockOwner.value, pthread_self()); +} + +void t_rwmutex::_lockRead() { int err = pthread_rwlock_rdlock(&_lock); if (err != 0) throw std::logic_error("Mutex lock failed"); } -void t_rwmutex::lockWrite() +void t_rwmutex::_lockWrite() { int err = pthread_rwlock_wrlock(&_lock); if (err != 0) throw std::logic_error("Mutex lock failed"); } -void t_rwmutex::unlock() +void t_rwmutex::_unlock() { pthread_rwlock_unlock(&_lock); } +void t_rwmutex::lockRead() +{ + if (isUpgradeOwnershipOurs()) { + throw std::logic_error("Acquiring read lock while holding update/write lock is not supported"); + } + + _lockRead(); +} + +void t_rwmutex::lockUpdate() +{ + if (isUpgradeOwnershipOurs()) { + throw std::logic_error("Acquiring update lock while holding update/write lock is not supported"); + } + + getUpgradeOwnership(); + _lockRead(); +} + +void t_rwmutex::lockWrite() +{ + if (isUpgradeOwnershipOurs()) { + throw std::logic_error("Acquiring write lock while holding update/write lock is not supported"); + } + + getUpgradeOwnership(); + _lockWrite(); +} + +void t_rwmutex::unlock() +{ + _unlock(); + + if (isUpgradeOwnershipOurs()) { + releaseUpgradeOwnership(); + } +} + +void t_rwmutex::upgradeLock() +{ + if (!isUpgradeOwnershipOurs()) { + throw std::logic_error("Attempting to upgrade a lock without upgrade ownership"); + } + + _unlock(); + _lockWrite(); +} + +void t_rwmutex::downgradeLock() +{ + if (!isUpgradeOwnershipOurs()) { + throw std::logic_error("Attempting to downgrade a lock without upgrade ownership"); + } + + _unlock(); + _lockRead(); +} + /////////////////////////// // t_rwmutex_guard /////////////////////////// t_rwmutex_guard::t_rwmutex_guard(t_rwmutex& mutex) : - _mutex(mutex) + _mutex(mutex), + _previously_owned_upgrade(_mutex.isUpgradeOwnershipOurs()) { } t_rwmutex_reader::t_rwmutex_reader(t_rwmutex& mutex) : t_rwmutex_guard(mutex) { - // std::cout << "mtx rd lock " << (void*)&_mutex << std::endl; - _mutex.lockRead(); + // No-op if we are nested within a writer/future_writer guard + if (!_previously_owned_upgrade) { + _mutex.lockRead(); + } } t_rwmutex_reader::~t_rwmutex_reader() { - // std::cout << "mtx rd unlock " << (void*)&_mutex << std::endl; - _mutex.unlock(); + // No-op if we are nested within a writer/future_writer guard + if (!_previously_owned_upgrade) { + _mutex.unlock(); + } +} + +t_rwmutex_future_writer::t_rwmutex_future_writer(t_rwmutex& mutex) : + t_rwmutex_guard(mutex) +{ + // No-op if we are nested within a future_writer guard + if (!_previously_owned_upgrade) { + _mutex.lockUpdate(); + } +} + +t_rwmutex_future_writer::~t_rwmutex_future_writer() { + // No-op if we are nested within a future_writer guard + if (!_previously_owned_upgrade) { + _mutex.unlock(); + } } t_rwmutex_writer::t_rwmutex_writer(t_rwmutex& mutex) : t_rwmutex_guard(mutex) { - // std::cout << "mtx wr lock " << (void*)&_mutex << std::endl; - _mutex.lockWrite(); + if (_previously_owned_upgrade) { + // Writer nested inside a future_writer: upgrade lock + _mutex.upgradeLock(); + } else { + // Stand-alone writer guard + _mutex.lockWrite(); + } } t_rwmutex_writer::~t_rwmutex_writer() { - // std::cout << "mtx wr unlock " << (void*)&_mutex << std::endl; - _mutex.unlock(); + if (_previously_owned_upgrade) { + // We were nested within a future_writer guard, so return + // the mutex to its previous state + _mutex.downgradeLock(); + } else { + _mutex.unlock(); + } } -- cgit v1.2.3