Hi, Have you had time to look at this? Thanks!
Regards, Vu > -----Original Message----- > From: Vu Minh Nguyen [mailto:[email protected]] > Sent: Friday, September 1, 2017 12:19 PM > To: [email protected]; [email protected]; > [email protected] > Cc: [email protected]; Vu Minh Nguyen > <[email protected]> > Subject: [PATCH 1/1] log: duplicated code in lga_client and lga_stream > [#2567] > > Introduce `RefCounter` class to remove duplicated code in LogClient > and LogStreamInfo. > > Introduce also one new parameter `caller`, the purpose is mainly > for debug - know who is the caller to `RefCounter` methods. > --- > src/log/Makefile.am | 5 +- > src/log/agent/lga_agent.cc | 82 +++++++++++++++------------ > src/log/agent/lga_client.cc | 62 ++++----------------- > src/log/agent/lga_client.h | 45 ++++++++------- > src/log/agent/lga_common.h | 8 --- > src/log/agent/lga_mds.cc | 7 +-- > src/log/agent/lga_ref_counter.cc | 73 ++++++++++++++++++++++++ > src/log/agent/lga_ref_counter.h | 117 > +++++++++++++++++++++++++++++++++++++++ > src/log/agent/lga_stream.cc | 46 +-------------- > src/log/agent/lga_stream.h | 40 ++++++------- > 10 files changed, 300 insertions(+), 185 deletions(-) > create mode 100644 src/log/agent/lga_ref_counter.cc > create mode 100644 src/log/agent/lga_ref_counter.h > > diff --git a/src/log/Makefile.am b/src/log/Makefile.am > index b66b8a7..3d951eb 100644 > --- a/src/log/Makefile.am > +++ b/src/log/Makefile.am > @@ -31,7 +31,9 @@ lib_libSaLog_la_SOURCES = \ > src/log/agent/lga_util.cc \ > src/log/agent/lga_mds.cc \ > src/log/agent/lga_state.cc \ > - src/log/agent/lga_agent.cc > + src/log/agent/lga_agent.cc \ > + src/log/agent/lga_ref_counter.cc > + > > nodist_EXTRA_lib_libSaLog_la_SOURCES = dummy.cc > > @@ -66,6 +68,7 @@ noinst_HEADERS += \ > src/log/agent/lga_agent.h \ > src/log/agent/lga_stream.h \ > src/log/agent/lga_state.h \ > + src/log/agent/lga_ref_counter.h \ > src/log/common/lgsv_defs.h \ > src/log/common/lgsv_msg.h \ > src/log/logd/lgs.h \ > diff --git a/src/log/agent/lga_agent.cc b/src/log/agent/lga_agent.cc > index 55ee916..042bde3 100644 > --- a/src/log/agent/lga_agent.cc > +++ b/src/log/agent/lga_agent.cc > @@ -56,7 +56,9 @@ class ScopeData { > bool* is_updated; > // The increased value if @is_updated set. The aim of using enum instead > // of `int` type is to force the caller pass either value (1) or value (-1). > - RefCounterDegree value; > + RefCounter::Degree value; > + // Who is the caller of `RestoreRefCounter` > + const char* caller; > }; > > struct LogStreamInfoData { > @@ -66,7 +68,9 @@ class ScopeData { > bool* is_updated; > // The increased value if @is_updated set. The aim of using enum instead > // of `int` type is to force the caller pass either value (1) or value (-1). > - RefCounterDegree value; > + RefCounter::Degree value; > + // Who is the caller of `RestoreRefCounter` > + const char* caller; > }; > > explicit ScopeData(LogClientData*, bool* lock = nullptr); > @@ -107,20 +111,23 @@ ScopeData::~ScopeData() { > LogAgent::instance().EnterCriticalSection(); > LogClient* client = client_data_->client; > bool* is_updated = client_data_->is_updated; > - RefCounterDegree client_degree = client_data_->value; > + RefCounter::Degree client_degree = client_data_->value; > + const char* caller = client_data_->caller; > if (client != nullptr) { > // Do restore the reference counter if the client exists. > - client->RestoreRefCounter(client_degree, *is_updated); > - if (stream_data_ != nullptr) { > - LogStreamInfo* stream = stream_data_->stream; > - bool* stream_is_updated = stream_data_->is_updated; > - RefCounterDegree stream_degree = stream_data_->value; > - if (stream != nullptr) { > - // Do restore the reference counter if the stream exists. > - stream->RestoreRefCounter(stream_degree, *stream_is_updated); > - } > - } // stream_data_ > - } // client > + client->RestoreRefCounter(caller, client_degree, *is_updated); > + } > + > + if (stream_data_ != nullptr) { > + LogStreamInfo* stream = stream_data_->stream; > + bool* stream_is_updated = stream_data_->is_updated; > + RefCounter::Degree stream_degree = stream_data_->value; > + const char* caller = stream_data_->caller; > + if (stream != nullptr) { > + // Do restore the reference counter if the stream exists. > + stream->RestoreRefCounter(caller, stream_degree, > *stream_is_updated); > + } > + } > LogAgent::instance().LeaveCriticalSection(); > } > > @@ -137,13 +144,20 @@ LogAgent::LogAgent() { > // even they are in the same thread context. > // To avoid such risk, use RECURSIVE MUTEX for @LogClient > pthread_mutexattr_t attr; > - pthread_mutexattr_init(&attr); > - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); > - pthread_mutex_init(&mutex_, nullptr); > + 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_ > - pthread_mutex_init(&get_delete_obj_sync_mutex_, nullptr); > + result = pthread_mutex_init(&get_delete_obj_sync_mutex_, nullptr); > + assert(result == 0 && "Failed to init `get_delete_obj_sync_mutex_`"); > } > > void LogAgent::PopulateOpenParams( > @@ -419,7 +433,7 @@ SaAisErrorT LogAgent::saLogSelectionObjectGet( > // such as Restore the reference counter after fetching & updating. > // or unlock recovery mutex. > ScopeData::LogClientData client_data{client, &updated, > - RefCounterDegree::kIncOne}; > + RefCounter::Degree::kIncOne, __func__}; > ScopeData data{&client_data}; > > if (selectionObject == nullptr) { > @@ -439,7 +453,7 @@ SaAisErrorT LogAgent::saLogSelectionObjectGet( > return ais_rc; > } > > - if (client->FetchAndIncreaseRefCounter(&updated) == -1) { > + if (client->FetchAndIncreaseRefCounter(__func__, &updated) == -1) { > // @client is being deleted. Don't touch @this client > ais_rc = SA_AIS_ERR_TRY_AGAIN; > return ais_rc; > @@ -470,7 +484,7 @@ SaAisErrorT LogAgent::saLogDispatch(SaLogHandleT > logHandle, > // such as Restore the reference counter fetching & updating. > // or unlock recovery mutex. > ScopeData::LogClientData client_data{client, &updated, > - RefCounterDegree::kIncOne}; > + RefCounter::Degree::kIncOne, __func__}; > ScopeData data{&client_data}; > > if (is_dispatch_flag_valid(dispatchFlags) == false) { > @@ -490,7 +504,7 @@ SaAisErrorT LogAgent::saLogDispatch(SaLogHandleT > logHandle, > return ais_rc; > } > > - if (client->FetchAndIncreaseRefCounter(&updated) == -1) { > + if (client->FetchAndIncreaseRefCounter(__func__, &updated) == -1) { > // @client is being deleted. DO NOT touch this @client > ais_rc = SA_AIS_ERR_TRY_AGAIN; > return ais_rc; > @@ -564,7 +578,7 @@ SaAisErrorT LogAgent::saLogFinalize(SaLogHandleT > logHandle) { > // such as Restore the reference counter after fetching & updating. > // or unlock recovery mutex. > ScopeData::LogClientData client_data{client, &updated, > - RefCounterDegree::kDecOne}; > + RefCounter::Degree::kDecOne, __func__}; > ScopeData data{&client_data, &is_locked}; > > if (true) { > @@ -578,7 +592,7 @@ SaAisErrorT LogAgent::saLogFinalize(SaLogHandleT > logHandle) { > return ais_rc; > } > > - if (client->FetchAndDecreaseRefCounter(&updated) != 0) { > + if (client->FetchAndDecreaseRefCounter(__func__, &updated) != 0) { > // DO NOT delete this @client as it is being used by somewhere (>0) > // Or it is being deleted by other thread (=-1) > ais_rc = SA_AIS_ERR_TRY_AGAIN; > @@ -812,7 +826,7 @@ SaAisErrorT LogAgent::saLogStreamOpen_2( > // such as Restore the reference counter after fetching & updating. > // or unlock recovery mutex. > ScopeData::LogClientData client_data{client, &updated, > - RefCounterDegree::kIncOne}; > + RefCounter::Degree::kIncOne, __func__}; > ScopeData data{&client_data, &is_locked}; > > if (lga_is_extended_name_valid(logStreamName) == false) { > @@ -838,7 +852,7 @@ SaAisErrorT LogAgent::saLogStreamOpen_2( > return ais_rc; > } > > - if (client->FetchAndIncreaseRefCounter(&updated) == -1) { > + if (client->FetchAndIncreaseRefCounter(__func__, &updated) == -1) { > // @client is being deleted. DO NOT touch this @client > ais_rc = SA_AIS_ERR_TRY_AGAIN; > return ais_rc; > @@ -1122,9 +1136,9 @@ SaAisErrorT > LogAgent::saLogWriteLogAsync(SaLogStreamHandleT logStreamHandle, > // such as Restore the reference counter after fetching & updating. > // or unlock recovery mutex. > ScopeData::LogClientData client_data{client, &cUpdated, > - RefCounterDegree::kIncOne}; > + RefCounter::Degree::kIncOne, __func__}; > ScopeData::LogStreamInfoData stream_data{stream, &sUpdated, > - RefCounterDegree::kIncOne}; > + RefCounter::Degree::kIncOne, __func__}; > ScopeData data{&client_data, &stream_data, &is_locked}; > > if (true) { > @@ -1146,13 +1160,13 @@ SaAisErrorT > LogAgent::saLogWriteLogAsync(SaLogStreamHandleT logStreamHandle, > return ais_rc; > } > > - if (client->FetchAndIncreaseRefCounter(&cUpdated) == -1) { > + if (client->FetchAndIncreaseRefCounter(__func__, &cUpdated) == -1) { > // @client is being deleted. DO NOT touch this @client > ais_rc = SA_AIS_ERR_TRY_AGAIN; > return ais_rc; > } > > - if (stream->FetchAndIncreaseRefCounter(&sUpdated) == -1) { > + if (stream->FetchAndIncreaseRefCounter(__func__, &sUpdated) == -1) { > ais_rc = SA_AIS_ERR_TRY_AGAIN; > return ais_rc; > } > @@ -1292,9 +1306,9 @@ SaAisErrorT > LogAgent::saLogStreamClose(SaLogStreamHandleT logStreamHandle) { > // such as Restore the reference counter after fetching & updating. > // or unlock recovery mutex. > ScopeData::LogClientData client_data{client, &cUpdated, > - RefCounterDegree::kIncOne}; > + RefCounter::Degree::kIncOne, __func__}; > ScopeData::LogStreamInfoData stream_data{stream, &sUpdated, > - RefCounterDegree::kDecOne}; > + RefCounter::Degree::kDecOne, __func__}; > ScopeData data{&client_data, &stream_data, &is_locked}; > > if (true) { > @@ -1307,7 +1321,7 @@ SaAisErrorT > LogAgent::saLogStreamClose(SaLogStreamHandleT logStreamHandle) { > return ais_rc; > } > > - if (stream->FetchAndDecreaseRefCounter(&sUpdated) != 0) { > + if (stream->FetchAndDecreaseRefCounter(__func__, &sUpdated) != 0) { > // @stream is being used somewhere (>0), DO NOT delete this @stream. > // or @stream is being deleted on other thread (=-1) > ais_rc = SA_AIS_ERR_TRY_AGAIN; > @@ -1322,7 +1336,7 @@ SaAisErrorT > LogAgent::saLogStreamClose(SaLogStreamHandleT logStreamHandle) { > return ais_rc; > } > > - if (client->FetchAndIncreaseRefCounter(&cUpdated) == -1) { > + if (client->FetchAndIncreaseRefCounter(__func__, &cUpdated) == -1) { > ais_rc = SA_AIS_ERR_TRY_AGAIN; > return ais_rc; > } > diff --git a/src/log/agent/lga_client.cc b/src/log/agent/lga_client.cc > index 2d7c623..386c849 100644 > --- a/src/log/agent/lga_client.cc > +++ b/src/log/agent/lga_client.cc > @@ -32,7 +32,7 @@ > // LogClient > //-------------------------------------------------------------------------- ---- > LogClient::LogClient(const SaLogCallbacksT* cb, uint32_t id, SaVersionT ver) > - : client_id_{id}, ref_counter_{0} { > + : client_id_{id}, ref_counter_object_{} { > TRACE_ENTER(); > // Reset registered callback info > memset(&callbacks_, 0, sizeof(callbacks_)); > @@ -65,13 +65,16 @@ LogClient::LogClient(const SaLogCallbacksT* cb, > uint32_t id, SaVersionT ver) > // even they are in the same thread context. > // To avoid such risk, use RECURSIVE MUTEX for @LogClient > pthread_mutexattr_t attr; > - pthread_mutexattr_init(&attr); > - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); > - pthread_mutex_init(&mutex_, nullptr); > - pthread_mutexattr_destroy(&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"); > > - // Initialize @ref_counter_mutex_ > - pthread_mutex_init(&ref_counter_mutex_, nullptr); > + result = pthread_mutex_init(&mutex_, nullptr); > + assert(result == 0 && "Failed to init mutex"); > + > + pthread_mutexattr_destroy(&attr); > } > > LogClient::~LogClient() { > @@ -94,51 +97,9 @@ LogClient::~LogClient() { > // Free the allocated mailbox for this client > m_NCS_IPC_RELEASE(&mailbox_, nullptr); > > - pthread_mutex_destroy(&ref_counter_mutex_); > pthread_mutex_destroy(&mutex_); > } > > -// Return (-1) if @this object is being deleted. > -// Otherwise, increase the reference counter. > -int32_t LogClient::FetchAndIncreaseRefCounter(bool* updated) { > - TRACE_ENTER(); > - ScopeLock scopeLock(ref_counter_mutex_); > - int32_t backup = ref_counter_; > - *updated = false; > - TRACE("%s: value = %d", __func__, ref_counter_); > - // The @this object is being deleted. > - if (ref_counter_ == -1) return backup; > - ref_counter_ += 1; > - *updated = true; > - return backup; > -} > - > -// Return (0) if @this object is allowed to deleted > -// Otherwise, @this object is being used or being deleted by > -// other thread. > -int32_t LogClient::FetchAndDecreaseRefCounter(bool* updated) { > - TRACE_ENTER(); > - ScopeLock scopeLock(ref_counter_mutex_); > - int32_t backup = ref_counter_; > - *updated = false; > - TRACE("%s: value = %d", __func__, ref_counter_); > - // The @this object is being used or being deleted. > - if (ref_counter_ != 0) return backup; > - ref_counter_ = -1; > - *updated = true; > - return backup; > -} > - > -void LogClient::RestoreRefCounter(RefCounterDegree value, bool updated) > { > - TRACE_ENTER(); > - if (updated == false) return; > - ScopeLock scopeLock(ref_counter_mutex_); > - ref_counter_ -= value; > - TRACE("%s: value = %d", __func__, ref_counter_); > - // Don't expect the @ref_counter_ is less than (-1) > - assert(ref_counter_ >= -1); > -} > - > bool LogClient::ClearMailBox(NCSCONTEXT arg, NCSCONTEXT msg) { > TRACE_ENTER(); > lgsv_msg_t *cbk, *pnext; > @@ -413,7 +374,8 @@ bool LogClient::HaveLogStreamInUse() { > TRACE_ENTER(); > ScopeLock scopeLock(mutex_); > for (const auto& s : stream_list_) { > - if ((s != nullptr) && (s->ref_counter_ > 0)) return true; > + if ((s != nullptr) && (s->ref_counter_object_.ref_counter() > 0)) > + return true; > } > return false; > } > diff --git a/src/log/agent/lga_client.h b/src/log/agent/lga_client.h > index eb1efd4..b86869c 100644 > --- a/src/log/agent/lga_client.h > +++ b/src/log/agent/lga_client.h > @@ -21,13 +21,14 @@ > #include <stdint.h> > #include <vector> > #include <atomic> > +#include <saLog.h> > #include "base/mutex.h" > #include "mds/mds_papi.h" > -#include <saLog.h> > #include "log/agent/lga_stream.h" > #include "log/common/lgsv_msg.h" > #include "log/common/lgsv_defs.h" > #include "log/agent/lga_state.h" > +#include "log/agent/lga_ref_counter.h" > > //> > // @LogClient class > @@ -44,7 +45,7 @@ > // After adding @this LogClient object to the database of @LogAgent, > // The proper way to retrieve @this object is via SearchClientById() > // or SeachClientByHandle(), and the next call MUST be > -// FetchAndUpdateObjectState() on that object to update its ref counter. > +// FetchAndUpdateRefCounter() on that object to update its ref counter. > // > // This object contains lot of important information about the log client, > // such as the mailbox @mailbox_ for putting all MDS msg which belong to > @@ -57,10 +58,10 @@ > // @saLogStreamOpen successfully, and removed from the database > // when @saLogStreamClose is called successfully. > // > -// And each @LogClient object has one special attribute, called > @ref_counter_, > -// that attribute should be updated when it is refered, modify or deleting. > -// FetchAndUpdateObjectState()/RestoreObjectState() are methods to > -// increase/restore the @ref_counter_ > +// And each @LogClient object owns one special object, > ref_counter_object_, > +// that object should be updated when it is referred, modified or deleted. > +// FetchAnd<xxx>RefCounter()/RestoreRefCounter() are methods to update > +// ref_counter_object_ object. > // > // @LogClient object can be deleted ONLY if no log stream which it owns > // are in "being use" state (@ref_counter > 0). > @@ -144,18 +145,23 @@ class LogClient { > bool is_stale_client() const; > > // Fetch and increase reference counter. > - // Increase one if @this object is not being deleted by other thread. > - // @updated will be set to true if there is an increase, false otherwise. > - int32_t FetchAndIncreaseRefCounter(bool* updated); > + // Refer to `RefCounter` class for more info. > + int32_t FetchAndIncreaseRefCounter(const char* caller, bool* updated) { > + return ref_counter_object_.FetchAndIncreaseRefCounter(caller, > updated); > + } > > // Fetch and decrease reference counter. > - // Decrease one if @this object is not being used or deleted by other > thread. > - // @updated will be set to true if there is an decrease, false otherwise. > - int32_t FetchAndDecreaseRefCounter(bool* updated); > + // Refer to `RefCounter` class for more info. > + int32_t FetchAndDecreaseRefCounter(const char* caller, bool* updated) { > + return ref_counter_object_.FetchAndDecreaseRefCounter(caller, > updated); > + } > > // Restore the reference counter back. > - // Passing @value is either (1) [increase] or (-1) [decrease] > - void RestoreRefCounter(RefCounterDegree value, bool updated); > + // Refer to `RefCounter` class for more info. > + void RestoreRefCounter(const char* caller, RefCounter::Degree value, > + bool updated) { > + return ref_counter_object_.RestoreRefCounter(caller, value, updated); > + } > > // true if @this client does NOT recovery succcessfully. > // false, otherwise. > @@ -249,15 +255,8 @@ class LogClient { > pthread_mutex_t mutex_; > > // Hold information how many thread are referring to this object > - // If the object is being deleted, the value is (-1). > - // If the object is not being deleted/used, the value is (0) > - // If there are N thread referring to this object, the counter will be N. > - int32_t ref_counter_; > - > - // Dedicate an own mutex to protect @ref_counter_. > - // The reason not sharing the @mutex_ is to avoid deadlock with MDS > thread > - // E.g: lock(mutex_) --> send MDS sync --> receiving ack from MDS --> > deadlock > - pthread_mutex_t ref_counter_mutex_; > + // Refer to `RefCounter` class for more info. > + RefCounter ref_counter_object_; > > // Hold all log streams belong to @this client > std::vector<LogStreamInfo*> stream_list_; > diff --git a/src/log/agent/lga_common.h b/src/log/agent/lga_common.h > index ef63cbb..87bd9e7 100644 > --- a/src/log/agent/lga_common.h > +++ b/src/log/agent/lga_common.h > @@ -22,14 +22,6 @@ > #include "base/osaf_utility.h" > #include "base/macros.h" > > -// Degree of reference counter decrease/increase > -enum RefCounterDegree { > - // Say, I want referring to object. Count me in. > - kIncOne = 1, > - // Say, I want to delete the object. > - kDecOne = -1 > -}; > - > // LOG server state. The state changes according to getting MDS events. > enum class LogServerState { > // No Active LOG server. Could happen a short time during > failover/switchover. > diff --git a/src/log/agent/lga_mds.cc b/src/log/agent/lga_mds.cc > index 81791c0..ca94957 100644 > --- a/src/log/agent/lga_mds.cc > +++ b/src/log/agent/lga_mds.cc > @@ -111,8 +111,7 @@ static uint32_t lga_enc_finalize_msg(NCS_UBAID > *uba, lgsv_msg_t *msg) { > return total_bytes; > } > > -static uint32_t encode_sanamet(NCS_UBAID *uba, uint8_t *p8, SaNameT > *name) > -{ > +static uint32_t encode_sanamet(NCS_UBAID *uba, uint8_t *p8, SaNameT > *name) { > uint32_t total_bytes = 0; > p8 = ncs_enc_reserve_space(uba, 2); > if (!p8) { > @@ -527,7 +526,7 @@ static uint32_t lga_lgs_msg_proc(lgsv_msg_t > *lgsv_msg, > > // @client is being deleted in other thread. DO NOT touch this. > bool updated = false; > - if (client->FetchAndIncreaseRefCounter(&updated) == -1) { > + if (client->FetchAndIncreaseRefCounter(__func__, &updated) == -1) { > LogAgent::instance().LeaveCriticalSection(); > lga_msg_destroy(lgsv_msg); > TRACE_LEAVE(); > @@ -619,7 +618,7 @@ static uint32_t lga_lgs_msg_proc(lgsv_msg_t > *lgsv_msg, > break; > } > > - client->RestoreRefCounter(RefCounterDegree::kIncOne, updated); > + client->RestoreRefCounter(__func__, RefCounter::Degree::kIncOne, > updated); > TRACE_LEAVE(); > return rc; > } > diff --git a/src/log/agent/lga_ref_counter.cc > b/src/log/agent/lga_ref_counter.cc > new file mode 100644 > index 0000000..25574f7 > --- /dev/null > +++ b/src/log/agent/lga_ref_counter.cc > @@ -0,0 +1,73 @@ > +/* -*- OpenSAF -*- > + * > + * Copyright Ericsson AB 2017 - All Rights Reserved. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY > + * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are > licensed > + * under the GNU Lesser General Public License Version 2.1, February 1999. > + * The complete license can be accessed from the following location: > + * http://opensource.org/licenses/lgpl-license.php > + * See the Copying file included with the OpenSAF distribution for full > + * licensing terms. > + * > + * Author(s): Ericsson AB > + * > + */ > + > +#include "log/agent/lga_ref_counter.h" > +#include <assert.h> > +#include "base/logtrace.h" > + > +//------------------------------------------------------------------------- ----- > +// RefCounter > +//------------------------------------------------------------------------- ----- > +RefCounter::RefCounter() : ref_counter_{0}, ref_counter_mutex_{} {} > + > +// Return (-1) if @this object is being deleted. > +// Otherwise, increase the reference counter. > +int32_t RefCounter::FetchAndIncreaseRefCounter(const char* caller, > + bool* updated) { > + TRACE_ENTER(); > + assert(caller != nullptr && updated != nullptr); > + base::Lock scopeLock(ref_counter_mutex_); > + int32_t backup = ref_counter_; > + *updated = false; > + TRACE("%s(%s): counter(%d) = %d", __func__, caller, *updated, > ref_counter_); > + // The @this object is being deleted. > + if (ref_counter_ == -1) return backup; > + ref_counter_ += 1; > + *updated = true; > + return backup; > +} > + > +// Return (0) if @this object is allowed to deleted > +// Otherwise, @this object is being used or being deleted by > +// other thread. > +int32_t RefCounter::FetchAndDecreaseRefCounter(const char* caller, > + bool* updated) { > + TRACE_ENTER(); > + assert(caller != nullptr && updated != nullptr); > + base::Lock scopeLock(ref_counter_mutex_); > + int32_t backup = ref_counter_; > + *updated = false; > + TRACE("%s(%s): counter(%d) = %d", __func__, caller, *updated, > ref_counter_); > + // @this object is being used or being deleted. > + if (ref_counter_ != 0) return backup; > + ref_counter_ = -1; > + *updated = true; > + return backup; > +} > + > +void RefCounter::RestoreRefCounter(const char* caller, > + Degree value, bool updated) { > + TRACE_ENTER(); > + assert(caller != nullptr && "No caller"); > + if (updated == false) return; > + base::Lock scopeLock(ref_counter_mutex_); > + int degree = (value == Degree::kIncOne ? 1 : -1); > + ref_counter_ -= degree; > + TRACE("%s(%s): counter(%d) = %d", __func__, caller, degree, > ref_counter_); > + // Don't expect the @ref_counter_ is less than (-1) > + assert(ref_counter_ >= -1); > +} > diff --git a/src/log/agent/lga_ref_counter.h > b/src/log/agent/lga_ref_counter.h > new file mode 100644 > index 0000000..4fbd28f > --- /dev/null > +++ b/src/log/agent/lga_ref_counter.h > @@ -0,0 +1,117 @@ > +/* -*- OpenSAF -*- > + * > + * Copyright Ericsson AB 2017 - All Rights Reserved. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY > + * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are > licensed > + * under the GNU Lesser General Public License Version 2.1, February 1999. > + * The complete license can be accessed from the following location: > + * http://opensource.org/licenses/lgpl-license.php > + * See the Copying file included with the OpenSAF distribution for full > + * licensing terms. > + * > + * Author(s): Ericsson AB > + * > + */ > + > +#ifndef SRC_LOG_AGENT_LGA_REF_COUNTER_H_ > +#define SRC_LOG_AGENT_LGA_REF_COUNTER_H_ > + > +#include <stdint.h> > +#include "log/agent/lga_common.h" > +#include "base/mutex.h" > + > +//> > +// The reference counter class is used to synchronize access > +// to its object owner. The class provides helper methods > +// to let the owner object know how many thread are referring > +// to owner object, is owner object deleting. > +// > +// So, by using this RefCounter object, the owner can avoid data race. > +// > +// When the object owner is refering for reading/modifying, the caller > +// firstly should check if the object is being deleted or not by > +// using method `FetchAndIncreaseRefCounter()`. If there is deletion going, > +// means the return value is (-1), the caller should not touch this object > +// by returning try again, otherwise could get coredump. > +// > +// And if the object owner is going to be deleted, the caller firstly check > +// if if the object is being used by other thread or not by calling method > +// `FetchAndDecreaseRefCounter`. If there is thread referring to this object, > +// the method will return non-zero (!=0). In that case, the caller > +// should not delete the object, otherwise could get coredump. > +// > +// When calling any methods of this class, the first parameter, `caller`, > +// is used mainly for trace purpose. It will be helpful to detect who > +// is the caller to these methods. > +// > +// Usage example: > +// class Owner { > +// public: > +// RefCounter ref_counter_object_; > +//} > +// > +// <! Case #1: Using Owner object !> > +// if (ref_counter_object_.FetchAndIncreaseRefCounter() == -1) { > +// // Return try again and come back later > +// } > +// > +// // It is ok to refer to Owner object. > +// > +// > +// <! Case #2: Delete Owner object !> > +// if (ref_counter_object_.FetchAndDecreaseRefCounter() != 0) { > +// // The owner is using or deleting by other thread. Return try again. > +// } > +// > +// // It is ok to delete the Owner object. > +//< > +class RefCounter { > + public: > + // Degree of reference counter. Introduce to force caller either using > + // kIncOne (1) or kDecOne(-1). Other value is not allowed. > + enum Degree { > + // Say, I want referring to object. Count me in. > + kIncOne = 1, > + // Say, I want to delete the object. > + kDecOne = -1 > + }; > + > + RefCounter(); > + ~RefCounter() {}; > + > + // Fetch and increase reference counter. > + // Increase one if @this object is not being deleted by other thread. > + // @updated will be set to true if there is an increase, false otherwise. > + // @caller shows who is the caller, the main purpose is for debugging. > + int32_t FetchAndIncreaseRefCounter(const char* caller, bool* updated); > + > + // Fetch and decrease reference counter. > + // Decrease one if @this object is not being used or deleted by other > thread. > + // @updated will be set to true if there is an decrease, false otherwise. > + // @caller shows who is the caller, the main purpose is for debugging. > + int32_t FetchAndDecreaseRefCounter(const char* caller, bool* updated); > + > + // Restore the reference counter back if @updated is true. > + // Passed @value is either (1) [increase] or (-1) [decrease] > + // @caller shows who is the caller, the main purpose is for debugging. > + void RestoreRefCounter(const char* caller, Degree value, bool updated); > + > + // Return current ref_counter_ value > + int32_t ref_counter() const { return ref_counter_; } > + > + private: > + // Hold information how many thread are referring to this object > + // If the object is being deleted, the value is (-1). > + // If the object is not being deleted/used, the value is (0) > + // If there are N thread referring to this object, the counter will be N. > + int32_t ref_counter_; > + > + // The mutex to protect @ref_counter_. > + base::Mutex ref_counter_mutex_; > + > + DELETE_COPY_AND_MOVE_OPERATORS(RefCounter); > +}; > + > +#endif // SRC_LOG_AGENT_LGA_REF_COUNTER_H_ > diff --git a/src/log/agent/lga_stream.cc b/src/log/agent/lga_stream.cc > index d64ee13..8481f42 100644 > --- a/src/log/agent/lga_stream.cc > +++ b/src/log/agent/lga_stream.cc > @@ -28,7 +28,7 @@ > #include "mds/mds_papi.h" > > LogStreamInfo::LogStreamInfo(const std::string& name, uint32_t id) > - : stream_name_{name}, ref_counter_mutex_{}, ref_counter_{0} { > + : stream_name_{name} { > TRACE_ENTER(); > stream_id_ = id; > recovered_flag_ = true; > @@ -49,50 +49,6 @@ LogStreamInfo::~LogStreamInfo() { > } > } > > -// Return (-1) if @this object is being deleted. > -// Otherwise, increase the reference counter. > -int32_t LogStreamInfo::FetchAndIncreaseRefCounter(bool* updated) { > - TRACE_ENTER(); > - base::Lock scopeLock(ref_counter_mutex_); > - int32_t backup = ref_counter_; > - *updated = false; > - TRACE("%s: value = %d", __func__, ref_counter_); > - // The @this object is being deleted. > - if (ref_counter_ == -1) return backup; > - ref_counter_ += 1; > - *updated = true; > - return backup; > -} > - > -// Return (0) if @this object is allowed to deleted > -// Otherwise, @this object is being used or being deleted by > -// other thread. > -int32_t LogStreamInfo::FetchAndDecreaseRefCounter(bool* updated) { > - TRACE_ENTER(); > - base::Lock scopeLock(ref_counter_mutex_); > - int32_t backup = ref_counter_; > - *updated = false; > - TRACE("%s: value = %d", __func__, ref_counter_); > - // The @this object is being used or being deleted. > - if (ref_counter_ != 0) return backup; > - ref_counter_ = -1; > - *updated = true; > - return backup; > -} > - > -void LogStreamInfo::RestoreRefCounter(RefCounterDegree value, bool > updated) { > - TRACE_ENTER(); > - if (updated == false) return; > - base::Lock scopeLock(ref_counter_mutex_); > - // @value is negative number in case restoring the counter > - // for @FetchAndDecreaseRefCounter(). > - // Should be a positive number, otherwise. > - ref_counter_ -= value; > - TRACE("%s: value = %d", __func__, ref_counter_); > - // Don't expect the @ref_counter_ is less than (-1) > - assert(ref_counter_ >= -1); > -} > - > bool LogStreamInfo::SendOpenStreamMsg(uint32_t client_id) { > TRACE_ENTER(); > SaAisErrorT ais_rc = SA_AIS_OK; > diff --git a/src/log/agent/lga_stream.h b/src/log/agent/lga_stream.h > index 5b9f6f9..abacbe3 100644 > --- a/src/log/agent/lga_stream.h > +++ b/src/log/agent/lga_stream.h > @@ -23,6 +23,7 @@ > #include <saLog.h> > #include "base/mutex.h" > #include "log/agent/lga_common.h" > +#include "log/agent/lga_ref_counter.h" > > //< > // @LogStreamInfo class > @@ -53,9 +54,13 @@ > // @LogClient. > // > // To avoid @this LogStreamInfo object deleted while it is being used by > other > -// thread, such as one client thread is performing writing log record, > -// in the meantime, other client thread is closing that that log stream > -// or closing the owning @LogClient. (! refer to @LogAgent description for > more) > +// thread, such as one client thread is performing writing log record, in the > +// meantime, other client thread is closing that that log stream or closing > the > +// owning @LogClient. (! refer to @LogAgent description for more) , one > special > +// object `RefCounter` is included in. Each @LogStreamInfo object owns > one > +// ref_counter_object_, that object should be updated when this object is > +// using, modified or deleted. > FetchAnd<xxx>RefCounter()/RestoreRefCounter() > +// are methods to update ref_counter_object_ object. > // > // When SC restarts from headless, @this object will be notified to do > // recover itself for specific @LogClient Id. > @@ -83,18 +88,20 @@ class LogStreamInfo { > const std::string& GetStreamName() const { return stream_name_; } > > // Fetch and increase reference counter. > - // Increase one if @this object is not being deleted by other thread. > - // @updated will be set to true if there is an increase, false otherwise. > - int32_t FetchAndIncreaseRefCounter(bool* updated); > + int32_t FetchAndIncreaseRefCounter(const char* caller, bool* updated) { > + return ref_counter_object_.FetchAndIncreaseRefCounter(caller, > updated); > + } > > // Fetch and decrease reference counter. > - // Decrease one if @this object is not being used or deleted by other > thread. > - // @updated will be set to true if there is an decrease, false otherwise. > - int32_t FetchAndDecreaseRefCounter(bool* updated); > + int32_t FetchAndDecreaseRefCounter(const char* caller, bool* updated) { > + return ref_counter_object_.FetchAndDecreaseRefCounter(caller, > updated); > + } > > // Restore the reference counter back. > - // Passing @value is either (1) [increase] or (-1) [decrease] > - void RestoreRefCounter(RefCounterDegree value, bool updated); > + void RestoreRefCounter(const char* caller, RefCounter::Degree value, > + bool updated) { > + return ref_counter_object_.RestoreRefCounter(caller, value, updated); > + } > > private: > // Set stream open flags @open_flags_ > @@ -119,16 +126,9 @@ class LogStreamInfo { > // Log stream name mentioned during open log stream > std::string stream_name_; > > - // To protect @ref_counter_ > - // E.g: if @this object is being used in @saLogStreamWrite > - // and other thread trying to delete it, will get TRY_AGAIN. > - base::Mutex ref_counter_mutex_; > - > // Hold information how many thread are referring to this object > - // If the object is being deleted, the value is (-1). > - // If the object is not being deleted/used, the value is (0) > - // If there are N thread referring to this object, the counter will be N. > - int32_t ref_counter_; > + // Refer to `RefCounter` class for more info. > + RefCounter ref_counter_object_; > > // Log stream open flags as defined in AIS.02.01 > SaLogStreamOpenFlagsT open_flags_; > -- > 1.9.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
