The C++ std provides unique_lock which supports all ScopeLock features. Use the standard lock instead of rewriting it. --- src/log/agent/lga_agent.cc | 59 +++++++++++++------------------------ src/log/agent/lga_agent.h | 11 +++---- src/log/agent/lga_client.cc | 32 +++++--------------- src/log/agent/lga_client.h | 3 +- src/log/agent/lga_common.h | 19 ------------ src/log/agent/lga_state.cc | 9 +++--- src/log/agent/lga_util.cc | 8 ++--- 7 files changed, 43 insertions(+), 98 deletions(-)
diff --git a/src/log/agent/lga_agent.cc b/src/log/agent/lga_agent.cc index 6c8d927a5..0f9aad91f 100644 --- a/src/log/agent/lga_agent.cc +++ b/src/log/agent/lga_agent.cc @@ -136,25 +136,6 @@ ScopeData::~ScopeData() { //------------------------------------------------------------------------------ LogAgent::LogAgent() { client_list_.clear(); - // There is high risk of calling one @LogClient method - // in the body of other @LogClient methods, such case would cause deadlock - // even they are in the same thread context. - // To avoid such risk, use RECURSIVE MUTEX for @LogClient - pthread_mutexattr_t attr; - int result = pthread_mutexattr_init(&attr); - assert(result == 0 && "Failed to init mutex attribute"); - - result = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); - assert(result == 0 && "Failed to set mutex type"); - - result = pthread_mutex_init(&mutex_, nullptr); - assert(result == 0 && "Failed to init `mutex_`"); - - pthread_mutexattr_destroy(&attr); - - // Initialize @get_delete_obj_sync_mutex_ - result = pthread_mutex_init(&get_delete_obj_sync_mutex_, nullptr); - assert(result == 0 && "Failed to init `get_delete_obj_sync_mutex_`"); } void LogAgent::PopulateOpenParams( @@ -201,7 +182,7 @@ LogStreamInfo* LogAgent::SearchLogStreamInfoByHandle(SaLogStreamHandleT h) { // Search for client in the @client_list_ based on client @handle LogClient* LogAgent::SearchClientByHandle(uint32_t handle) { TRACE_ENTER(); - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); LogClient* client = nullptr; for (const auto& c : client_list_) { if (c != nullptr && c->GetHandle() == handle) { @@ -215,7 +196,7 @@ LogClient* LogAgent::SearchClientByHandle(uint32_t handle) { // Search for client in the @client_list_ based on client @id LogClient* LogAgent::SearchClientById(uint32_t id) { TRACE_ENTER(); - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); LogClient* client = nullptr; for (const auto& c : client_list_) { if (c != nullptr && c->GetClientId() == id) { @@ -230,7 +211,7 @@ LogClient* LogAgent::SearchClientById(uint32_t id) { // This method should be called in @saLogFinalize() context void LogAgent::RemoveLogClient(LogClient** client) { TRACE_ENTER(); - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); assert(*client != nullptr); auto it = std::remove(client_list_.begin(), client_list_.end(), *client); if (it != client_list_.end()) { @@ -246,7 +227,7 @@ void LogAgent::RemoveLogClient(LogClient** client) { void LogAgent::RemoveAllLogClients() { TRACE_ENTER(); - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); for (auto& client : client_list_) { if (client == nullptr) continue; delete client; @@ -257,7 +238,7 @@ void LogAgent::RemoveAllLogClients() { // This method should be called in @saLogInitialize() context void LogAgent::AddLogClient(LogClient* client) { TRACE_ENTER(); - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); assert(client != nullptr); client_list_.push_back(client); } @@ -265,7 +246,7 @@ void LogAgent::AddLogClient(LogClient* client) { // Do recover all log clients in list @client_list_ bool LogAgent::RecoverAllLogClients() { TRACE_ENTER(); - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); for (auto& client : client_list_) { // We just get SC absence in MDS thread // Don't try to recover the client list. @@ -287,7 +268,7 @@ bool LogAgent::RecoverAllLogClients() { void LogAgent::NoLogServer() { TRACE_ENTER(); - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); // When SC is absent, surely, no active SC. // Then reset LOG server destination address. atomic_data_.lgs_mds_dest = 0; @@ -444,7 +425,7 @@ SaAisErrorT LogAgent::saLogSelectionObjectGet( } if (true) { - ScopeLock critical_section(get_delete_obj_sync_mutex_); + std::unique_lock<std::mutex> critical_section(critial_section_mutex_); // Get log client from the global list client = SearchClientByHandle(logHandle); @@ -495,7 +476,7 @@ SaAisErrorT LogAgent::saLogDispatch(SaLogHandleT logHandle, } if (true) { - ScopeLock critical_section(get_delete_obj_sync_mutex_); + std::unique_lock<std::mutex> critical_section(critial_section_mutex_); // Get log client from the global list client = SearchClientByHandle(logHandle); @@ -583,7 +564,7 @@ SaAisErrorT LogAgent::saLogFinalize(SaLogHandleT logHandle) { ScopeData data{&client_data, &is_locked}; if (true) { - ScopeLock critical_section(get_delete_obj_sync_mutex_); + std::unique_lock<std::mutex> critical_section(critial_section_mutex_); // Get log client from the global list client = SearchClientByHandle(logHandle); @@ -633,7 +614,7 @@ SaAisErrorT LogAgent::saLogFinalize(SaLogHandleT logHandle) { TRACE("%s lga_state = Recovery state (1)", __func__); if (client->is_client_initialized() == false) { TRACE("\t Client is not initialized. Remove it from database"); - ScopeLock critical_section(get_delete_obj_sync_mutex_); + std::unique_lock<std::mutex> critical_section(critial_section_mutex_); RemoveLogClient(&client); return ais_rc; } @@ -647,7 +628,7 @@ SaAisErrorT LogAgent::saLogFinalize(SaLogHandleT logHandle) { if (ais_rc == SA_AIS_OK || ais_rc == SA_AIS_ERR_BAD_HANDLE) { TRACE("%s delete_one_client", __func__); if (true) { - ScopeLock critical_section(get_delete_obj_sync_mutex_); + std::unique_lock<std::mutex> critical_section(critial_section_mutex_); RemoveLogClient(&client); } } @@ -840,7 +821,7 @@ SaAisErrorT LogAgent::saLogStreamOpen_2( if (ais_rc != SA_AIS_OK) return ais_rc; if (true) { - ScopeLock critical_section(get_delete_obj_sync_mutex_); + std::unique_lock<std::mutex> critical_section(critial_section_mutex_); // Get log client from the global list client = SearchClientByHandle(logHandle); @@ -891,7 +872,7 @@ SaAisErrorT LogAgent::saLogStreamOpen_2( // Remove the client from the database if recovery failed if (client->is_recovery_done() == false) { - ScopeLock critical_section(get_delete_obj_sync_mutex_); + std::unique_lock<std::mutex> critical_section(critial_section_mutex_); // Make sure that client is just removed from the list // if no other users use it if (client->FetchRefCounter() == 1) { @@ -1145,7 +1126,7 @@ SaAisErrorT LogAgent::saLogWriteLogAsync(SaLogStreamHandleT logStreamHandle, ScopeData data{&client_data, &stream_data, &is_locked}; if (true) { - ScopeLock critical_section(get_delete_obj_sync_mutex_); + std::unique_lock<std::mutex> critical_section(critial_section_mutex_); // Retrieve the @LogStreamInfo based on @logStreamHandle stream = SearchLogStreamInfoByHandle(logStreamHandle); @@ -1267,7 +1248,7 @@ SaAisErrorT LogAgent::saLogWriteLogAsync(SaLogStreamHandleT logStreamHandle, // Remove the client from the database if recovery failed if (client->is_recovery_done() == false) { - ScopeLock critical_section(get_delete_obj_sync_mutex_); + std::unique_lock<std::mutex> critical_section(critial_section_mutex_); // Make sure that client is just removed from the list // if no other users use it if (client->FetchRefCounter() == 1) { @@ -1324,7 +1305,7 @@ SaAisErrorT LogAgent::saLogStreamClose(SaLogStreamHandleT logStreamHandle) { ScopeData data{&client_data, &stream_data, &is_locked}; if (true) { - ScopeLock critical_section(get_delete_obj_sync_mutex_); + std::unique_lock<std::mutex> critical_section(critial_section_mutex_); stream = SearchLogStreamInfoByHandle(logStreamHandle); if (stream == nullptr) { @@ -1372,7 +1353,7 @@ SaAisErrorT LogAgent::saLogStreamClose(SaLogStreamHandleT logStreamHandle) { // No server is available. Remove the stream from client database. // Server side will manage to release resources of this stream when up. TRACE("%s No server", __func__); - ScopeLock critical_section(get_delete_obj_sync_mutex_); + std::unique_lock<std::mutex> critical_section(critial_section_mutex_); client->RemoveLogStreamInfo(&stream); ais_rc = SA_AIS_OK; return ais_rc; @@ -1397,7 +1378,7 @@ SaAisErrorT LogAgent::saLogStreamClose(SaLogStreamHandleT logStreamHandle) { // Remove the client from the database if recovery failed if (client->is_recovery_done() == false) { - ScopeLock critical_section(get_delete_obj_sync_mutex_); + std::unique_lock<std::mutex> critical_section(critial_section_mutex_); // Make sure that client is just removed from the list // if no other users use it if (client->FetchRefCounter() == 1) { @@ -1445,7 +1426,7 @@ SaAisErrorT LogAgent::saLogStreamClose(SaLogStreamHandleT logStreamHandle) { } if (ais_rc == SA_AIS_OK) { - ScopeLock critical_section(get_delete_obj_sync_mutex_); + std::unique_lock<std::mutex> critical_section(critial_section_mutex_); client->RemoveLogStreamInfo(&stream); } diff --git a/src/log/agent/lga_agent.h b/src/log/agent/lga_agent.h index 0c32ea33b..2ca40f505 100644 --- a/src/log/agent/lga_agent.h +++ b/src/log/agent/lga_agent.h @@ -20,6 +20,7 @@ #include <atomic> #include <memory> +#include <mutex> #include <string> #include <vector> #include <saAis.h> @@ -264,7 +265,7 @@ class LogAgent { // Used to synchronize adding/removing @client to/from @client_list_ // MUST use this mutex whenever accessing to client from @client_list_ // This mutex is RECURSIVE. - pthread_mutex_t mutex_; + std::recursive_mutex mutex_; // Protect the short critical section: Search<xx>ByHandle() to // FetchAndUpdateObjectState(). Avoid race condition b/w @@ -275,8 +276,8 @@ class LogAgent { // and updating @ref_counter_. // NOTE: Use native mutex instead of base::Mutex because base::Mutex // has coredump in MDS thread when unlock the mutex. Not yet analyze. - // base::Mutex get_delete_obj_sync_mutex_; - pthread_mutex_t get_delete_obj_sync_mutex_; + // base::Mutex critial_section_mutex_; + std::mutex critial_section_mutex_; // Hold list of current log clients std::vector<LogClient*> client_list_; @@ -370,11 +371,11 @@ inline bool LogAgent::waiting_log_server_up() const { } inline void LogAgent::EnterCriticalSection() { - osaf_mutex_lock_ordie(&get_delete_obj_sync_mutex_); + critial_section_mutex_.lock(); } inline void LogAgent::LeaveCriticalSection() { - osaf_mutex_unlock_ordie(&get_delete_obj_sync_mutex_); + critial_section_mutex_.unlock(); } #endif // SRC_LOG_AGENT_LGA_AGENT_H_ diff --git a/src/log/agent/lga_client.cc b/src/log/agent/lga_client.cc index 2eb37a0f7..56f1100c5 100644 --- a/src/log/agent/lga_client.cc +++ b/src/log/agent/lga_client.cc @@ -59,22 +59,6 @@ LogClient::LogClient(const SaLogCallbacksT* cb, uint32_t id, SaVersionT ver) if ((rc = m_NCS_IPC_ATTACH(&mailbox_)) != NCSCC_RC_SUCCESS) { assert(rc != NCSCC_RC_SUCCESS && "Attach the mbx failed"); } - - // As there is high risk of calling of one @LogClient method - // in the body of other method, such case would cause deadlock - // even they are in the same thread context. - // To avoid such risk, use RECURSIVE MUTEX for @LogClient - pthread_mutexattr_t attr; - int result = pthread_mutexattr_init(&attr); - assert(result == 0 && "Failed to init mutex attribute"); - - result = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); - assert(result == 0 && "Failed to set mutex type"); - - result = pthread_mutex_init(&mutex_, nullptr); - assert(result == 0 && "Failed to init mutex"); - - pthread_mutexattr_destroy(&attr); } LogClient::~LogClient() { @@ -98,8 +82,6 @@ LogClient::~LogClient() { m_NCS_IPC_DETACH(&mailbox_, LogClient::ClearMailBox, nullptr); // Free the allocated mailbox for this client m_NCS_IPC_RELEASE(&mailbox_, nullptr); - - pthread_mutex_destroy(&mutex_); } bool LogClient::ClearMailBox(NCSCONTEXT arg, NCSCONTEXT msg) { @@ -142,7 +124,7 @@ void LogClient::InvokeCallback(const lgsv_msg_t* msg) { if (callbacks_.saLogFilterSetCallback) { LogStreamInfo* stream; stream = SearchLogStreamInfoById(cbk_info->lgs_stream_id); - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); if (stream != nullptr) { callbacks_.saLogFilterSetCallback( stream->GetHandle(), cbk_info->serverity_filter_cbk.log_severity); @@ -257,7 +239,7 @@ void LogClient::AddLogStreamInfo(LogStreamInfo* stream, SaLogStreamOpenFlagsT flag, SaLogHeaderTypeT htype) { TRACE_ENTER(); - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); assert(stream != nullptr); // Give additional info to @stream stream->SetOpenFlags(flag); @@ -270,7 +252,7 @@ void LogClient::AddLogStreamInfo(LogStreamInfo* stream, // and should be called after getting @saLogStreamClose() or @saLogFinalize(). void LogClient::RemoveLogStreamInfo(LogStreamInfo** stream) { TRACE_ENTER(); - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); assert(*stream != nullptr); auto i = std::remove(stream_list_.begin(), stream_list_.end(), *stream); if (i != stream_list_.end()) { @@ -282,7 +264,7 @@ void LogClient::RemoveLogStreamInfo(LogStreamInfo** stream) { LogStreamInfo* LogClient::SearchLogStreamInfoById(uint32_t id) { TRACE_ENTER(); - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); LogStreamInfo* stream = nullptr; for (auto const& s : stream_list_) { if (s != nullptr && s->GetStreamId() == id) { @@ -357,7 +339,7 @@ bool LogClient::RecoverMe() { return false; } - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); for (const auto& stream : stream_list_) { if (stream == nullptr) continue; TRACE("Recover client = %d, stream = %d", client_id_, @@ -376,7 +358,7 @@ bool LogClient::RecoverMe() { bool LogClient::HaveLogStreamInUse() { TRACE_ENTER(); - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); for (const auto& s : stream_list_) { if ((s != nullptr) && (s->ref_counter_object_.ref_counter() > 0)) return true; @@ -386,7 +368,7 @@ bool LogClient::HaveLogStreamInUse() { void LogClient::NoLogServer() { TRACE_ENTER(); - ScopeLock scopeLock(mutex_); + std::unique_lock<std::recursive_mutex> scopeLock(mutex_); // No SCs, no valid client id. client_id_ = 0; atomic_data_.recovered_flag = false; diff --git a/src/log/agent/lga_client.h b/src/log/agent/lga_client.h index e6e2c911e..db220cb8a 100644 --- a/src/log/agent/lga_client.h +++ b/src/log/agent/lga_client.h @@ -22,6 +22,7 @@ #include <vector> #include <list> #include <atomic> +#include <mutex> #include <saLog.h> #include "base/mutex.h" #include "mds/mds_papi.h" @@ -287,7 +288,7 @@ class LogClient { // Used to synchronize adding/removing to/from @stream_list_ // MUST use this mutex whenever accessing to stream from @stream_list_ // Using native mutex as base::Mutex does not support RECURSIVE. - pthread_mutex_t mutex_; + std::recursive_mutex mutex_; // Hold information how many thread are referring to this object // Refer to `RefCounter` class for more info. diff --git a/src/log/agent/lga_common.h b/src/log/agent/lga_common.h index 87bd9e717..0467eaea8 100644 --- a/src/log/agent/lga_common.h +++ b/src/log/agent/lga_common.h @@ -32,23 +32,4 @@ enum class LogServerState { kHasActiveLogServer }; -//> -// Introduce its own scope lock mutex because @base::Lock does not support -// RECURSIVE MUTEX. -//< -class ScopeLock { - public: - explicit ScopeLock(pthread_mutex_t& m) : mutex_{&m} { Lock(); }; - ~ScopeLock() { UnLock(); } - - private: - void Lock() { osaf_mutex_lock_ordie(mutex_); } - void UnLock() { osaf_mutex_unlock_ordie(mutex_); } - - private: - pthread_mutex_t* mutex_; - - DELETE_COPY_AND_MOVE_OPERATORS(ScopeLock); -}; - #endif // SRC_LOG_AGENT_LGA_COMMON_H_ diff --git a/src/log/agent/lga_state.cc b/src/log/agent/lga_state.cc index 970fe27d2..51b8f3680 100644 --- a/src/log/agent/lga_state.cc +++ b/src/log/agent/lga_state.cc @@ -256,20 +256,19 @@ void lga_recovery2_unlock(void) { osaf_mutex_unlock_ordie(&lga_recov2_lock); } typedef struct { RecoveryState state; - pthread_mutex_t lock; + std::mutex lock; } lga_state_s; -static lga_state_s lga_state = {.state = RecoveryState::kNormal, - .lock = PTHREAD_MUTEX_INITIALIZER}; +static lga_state_s lga_state = {.state = RecoveryState::kNormal}; static void set_lga_recovery_state(RecoveryState state) { - ScopeLock lock(lga_state.lock); + std::unique_lock<std::mutex> lock(lga_state.lock); lga_state.state = state; } bool is_lga_recovery_state(RecoveryState state) { bool rc = false; - ScopeLock lock(lga_state.lock); + std::unique_lock<std::mutex> lock(lga_state.lock); if (state == lga_state.state) rc = true; return rc; } diff --git a/src/log/agent/lga_util.cc b/src/log/agent/lga_util.cc index 0bb91aaee..6241b9ed6 100644 --- a/src/log/agent/lga_util.cc +++ b/src/log/agent/lga_util.cc @@ -30,7 +30,7 @@ #include "log/agent/lga_common.h" // Variables used during startup/shutdown only -static pthread_mutex_t init_lock = PTHREAD_MUTEX_INITIALIZER; +static std::mutex init_lock; static unsigned int client_counter = 0; /** @@ -81,7 +81,7 @@ static unsigned int lga_create() { */ unsigned int lga_startup() { unsigned int rc = NCSCC_RC_SUCCESS; - ScopeLock lock(init_lock); + std::unique_lock<std::mutex> lock(init_lock); std::atomic<MDS_HDL>& mds_hdl = LogAgent::instance()->atomic_get_mds_hdl(); TRACE_ENTER(); @@ -109,7 +109,7 @@ done: * The function help to trace number of clients */ void lga_increase_user_counter(void) { - ScopeLock lock(init_lock); + std::unique_lock<std::mutex> lock(init_lock); ++client_counter; TRACE_2("client_counter: %u", client_counter); @@ -121,7 +121,7 @@ void lga_increase_user_counter(void) { */ void lga_decrease_user_counter(void) { TRACE_ENTER2("client_counter: %u", client_counter); - ScopeLock lock(init_lock); + std::unique_lock<std::mutex> lock(init_lock); if (client_counter > 0) --client_counter; -- 2.17.1 _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel