From 58d1b88ec5918515a502a9d3f7dec34b04feb50a Mon Sep 17 00:00:00 2001 From: Patrick Donnelly Date: Tue, 13 Aug 2019 10:21:25 -0700 Subject: [PATCH] common: refactor RefCountedObj and reduce header dependencies - Protect constructors/destructors; RefCountedObj is an abstract class. - Move some helpers to separate unit to avoid circular dependency with CephContext which could otherwise use RefCountedObj. Signed-off-by: Patrick Donnelly --- src/common/CMakeLists.txt | 1 + src/common/RefCountedObj.cc | 40 +++++++++++++++++ src/common/RefCountedObj.h | 88 +++++++++++++++---------------------- src/crimson/CMakeLists.txt | 1 + 4 files changed, 77 insertions(+), 53 deletions(-) create mode 100644 src/common/RefCountedObj.cc diff --git a/src/common/CMakeLists.txt b/src/common/CMakeLists.txt index eadaf285d48..1ecaf936754 100644 --- a/src/common/CMakeLists.txt +++ b/src/common/CMakeLists.txt @@ -24,6 +24,7 @@ set(common_srcs OutputDataSocket.cc PluginRegistry.cc Readahead.cc + RefCountedObj.cc SloppyCRCMap.cc SubProcess.cc Thread.cc diff --git a/src/common/RefCountedObj.cc b/src/common/RefCountedObj.cc new file mode 100644 index 00000000000..e77b631d89e --- /dev/null +++ b/src/common/RefCountedObj.cc @@ -0,0 +1,40 @@ +// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- +// vim: ts=8 sw=2 smarttab +// +#include "include/ceph_assert.h" + +#include "common/RefCountedObj.h" +#include "common/ceph_context.h" +#include "common/dout.h" +#include "common/valgrind.h" + +RefCountedObject::~RefCountedObject() +{ + ceph_assert(nref == 0); +} + +void RefCountedObject::put() const { + CephContext *local_cct = cct; + auto v = --nref; + if (local_cct) { + lsubdout(local_cct, refs, 1) << "RefCountedObject::put " << this << " " + << (v + 1) << " -> " << v + << dendl; + } + if (v == 0) { + ANNOTATE_HAPPENS_AFTER(&nref); + ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(&nref); + delete this; + } else { + ANNOTATE_HAPPENS_BEFORE(&nref); + } +} + +void RefCountedObject::_get() const { + auto v = ++nref; + ceph_assert(v > 1); /* it should never happen that _get() sees nref == 0 */ + if (cct) { + lsubdout(cct, refs, 1) << "RefCountedObject::get " << this << " " + << (v - 1) << " -> " << v << dendl; + } +} diff --git a/src/common/RefCountedObj.h b/src/common/RefCountedObj.h index 3ebbe78fd2b..024d2f4d2c9 100644 --- a/src/common/RefCountedObj.h +++ b/src/common/RefCountedObj.h @@ -16,69 +16,50 @@ #define CEPH_REFCOUNTEDOBJ_H #include "common/ceph_mutex.h" -#include "common/ceph_context.h" -#include "common/valgrind.h" -#include "common/debug.h" +#include "common/ref.h" -#include - -// re-include our assert to clobber the system one; fix dout: -#include "include/ceph_assert.h" +#include struct RefCountedObject { public: - RefCountedObject(CephContext *c = NULL, int n=1) : nref(n), cct(c) {} - virtual ~RefCountedObject() { - ceph_assert(nref == 0); + void set_cct(class CephContext *c) { + cct = c; } - + + uint64_t get_nref() const { + return nref; + } + const RefCountedObject *get() const { - int v = ++nref; - if (cct) - lsubdout(cct, refs, 1) << "RefCountedObject::get " << this << " " - << (v - 1) << " -> " << v - << dendl; + _get(); return this; } RefCountedObject *get() { - int v = ++nref; - if (cct) - lsubdout(cct, refs, 1) << "RefCountedObject::get " << this << " " - << (v - 1) << " -> " << v - << dendl; + _get(); return this; } - void put() const { - CephContext *local_cct = cct; - auto v = --nref; - if (local_cct) - lsubdout(local_cct, refs, 1) << "RefCountedObject::put " << this << " " - << (v + 1) << " -> " << v - << dendl; - if (v == 0) { - ANNOTATE_HAPPENS_AFTER(&nref); - ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(&nref); - delete this; - } else { - ANNOTATE_HAPPENS_BEFORE(&nref); - } - } - void set_cct(CephContext *c) { - cct = c; - } + void put() const; - uint64_t get_nref() const { - return nref; - } +protected: + RefCountedObject() = default; + RefCountedObject(const RefCountedObject& o) : cct(o.cct) {} + RefCountedObject& operator=(const RefCountedObject& o) = delete; + RefCountedObject(RefCountedObject&&) = delete; + RefCountedObject& operator=(RefCountedObject&&) = delete; + RefCountedObject(class CephContext* c = nullptr, int n = 1) : cct(c), nref(n) {} + + virtual ~RefCountedObject(); private: + void _get() const; + #ifndef WITH_SEASTAR - mutable std::atomic nref; + mutable std::atomic nref{1}; #else // crimson is single threaded at the moment - mutable uint64_t nref; + mutable uint64_t nref{1}; #endif - CephContext *cct; + class CephContext *cct{nullptr}; }; #ifndef WITH_SEASTAR @@ -88,14 +69,9 @@ private: * * a refcounted condition, will be removed when all references are dropped */ - struct RefCountedCond : public RefCountedObject { - bool complete; - ceph::mutex lock = ceph::make_mutex("RefCountedCond::lock"); - ceph::condition_variable cond; - int rval; - - RefCountedCond() : complete(false), rval(0) {} + RefCountedCond() = default; + ~RefCountedCond() = default; int wait() { std::unique_lock l(lock); @@ -115,6 +91,12 @@ struct RefCountedCond : public RefCountedObject { void done() { done(0); } + +private: + bool complete = false; + ceph::mutex lock = ceph::make_mutex("RefCountedCond::lock"); + ceph::condition_variable cond; + int rval = 0; }; /** @@ -180,6 +162,6 @@ static inline void intrusive_ptr_release(const RefCountedObject *p) { p->put(); } -using RefCountedPtr = boost::intrusive_ptr; +using RefCountedPtr = ceph::ref_t; #endif diff --git a/src/crimson/CMakeLists.txt b/src/crimson/CMakeLists.txt index 72c65761676..a373cb3ba51 100644 --- a/src/crimson/CMakeLists.txt +++ b/src/crimson/CMakeLists.txt @@ -72,6 +72,7 @@ add_library(crimson-common STATIC ${PROJECT_SOURCE_DIR}/src/common/Thread.cc ${PROJECT_SOURCE_DIR}/src/common/HeartbeatMap.cc ${PROJECT_SOURCE_DIR}/src/common/PluginRegistry.cc + ${PROJECT_SOURCE_DIR}/src/common/RefCountedObj.cc ${PROJECT_SOURCE_DIR}/src/global/pidfile.cc ${PROJECT_SOURCE_DIR}/src/librbd/Features.cc ${PROJECT_SOURCE_DIR}/src/log/Log.cc -- 2.39.5