msg/async: refine worker creation in NetworkStack

This commit updates NetworkStack::create_worker() to a virtual
function to reduce type check redundancy.

Since calling a pure virtual function in a constructor method is
strictly prohibited, NetworkStack::create_worker() is currently
not implemented as a virtual function.
It requires duplicated type check with NetworkStack::create(),
making the code a little bit tricky.

Considering NetworkStack instances can only be instantiated
through NetworkStack::create(), this commit moves worker creation
out of its constructor to NetworkStack::create(), makes it a pure
virtual, and lets inherited classes implement it to remove the
type check redundancy. By making it a non-static, we can also
reduce unnecessary class member function exposure.

Signed-off-by: Insu Jang <insu_jang@tmax.co.kr>

diff --git a/src/msg/async/PosixStack.h b/src/msg/async/PosixStack.h
index 4aed9dd6444319e2c55460c3530aa2afd0817fba..e1d85f1f4b72097d3eb1a687f9b7c8c9b2c7314f 100644 (file)
--- a/src/msg/async/PosixStack.h
+++ b/src/msg/async/PosixStack.h
@@ -40,6 +40,10 @@ class PosixWorker : public Worker {
 class PosixNetworkStack : public NetworkStack {
   std::vector<std::thread> threads;
 
+  virtual Worker* create_worker(CephContext *c, unsigned worker_id) override {
+    return new PosixWorker(c, worker_id);
+  }
+
  public:
   explicit PosixNetworkStack(CephContext *c, const std::string &t);
 

diff --git a/src/msg/async/Stack.cc b/src/msg/async/Stack.cc
index 6885fd4f9dbcfdeaf71131631a1168eb29a0f811..bf1cd8203b6cf6f08b878821b6b3e6c951fb7d38 100644 (file)
--- a/src/msg/async/Stack.cc
+++ b/src/msg/async/Stack.cc
@@ -66,47 +66,42 @@ std::function<void ()> NetworkStack::add_thread(unsigned worker_id)
 std::shared_ptr<NetworkStack> NetworkStack::create(CephContext *c,
                                                   const std::string &t)
 {
+  std::shared_ptr<NetworkStack> stack = nullptr;
+
   if (t == "posix")
-    return std::make_shared<PosixNetworkStack>(c, t);
+    stack.reset(new PosixNetworkStack(c, t));
 #ifdef HAVE_RDMA
   else if (t == "rdma")
-    return std::make_shared<RDMAStack>(c, t);
+    stack.reset(new RDMAStack(c, t));
 #endif
 #ifdef HAVE_DPDK
   else if (t == "dpdk")
-    return std::make_shared<DPDKStack>(c, t);
+    stack.reset(new DPDKStack(c, t));
 #endif
 
-  lderr(c) << __func__ << " ms_async_transport_type " << t <<
+  if (stack == nullptr) {
+    lderr(c) << __func__ << " ms_async_transport_type " << t <<
     " is not supported! " << dendl;
-  ceph_abort();
-  return nullptr;
-}
-
-Worker* NetworkStack::create_worker(CephContext *c, const std::string &type, unsigned worker_id)
-{
-  if (type == "posix")
-    return new PosixWorker(c, worker_id);
-#ifdef HAVE_RDMA
-  else if (type == "rdma")
-    return new RDMAWorker(c, worker_id);
-#endif
-#ifdef HAVE_DPDK
-  else if (type == "dpdk")
-    return new DPDKWorker(c, worker_id);
-#endif
+    ceph_abort();
+    return nullptr;
+  }
+  
+  const int InitEventNumber = 5000;
+  for (unsigned worker_id = 0; worker_id < stack->num_workers; ++worker_id) {
+    Worker *w = stack->create_worker(c, worker_id);
+    int ret = w->center.init(InitEventNumber, worker_id, t);
+    if (ret)
+      throw std::system_error(-ret, std::generic_category());
+    stack->workers.push_back(w);
+  }
 
-  lderr(c) << __func__ << " ms_async_transport_type " << type <<
-    " is not supported! " << dendl;
-  ceph_abort();
-  return nullptr;
+  return stack;
 }
 
 NetworkStack::NetworkStack(CephContext *c, const std:: string &t): type(t), started(false), cct(c)
 {
   ceph_assert(cct->_conf->ms_async_op_threads > 0);
 
-  const int InitEventNumber = 5000;
   num_workers = cct->_conf->ms_async_op_threads;
   if (num_workers >= EventCenter::MAX_EVENTCENTER) {
     ldout(cct, 0) << __func__ << " max thread limit is "
@@ -115,14 +110,6 @@ NetworkStack::NetworkStack(CephContext *c, const std:: string &t): type(t), star
                   << dendl;
     num_workers = EventCenter::MAX_EVENTCENTER;
   }
-
-  for (unsigned worker_id = 0; worker_id < num_workers; ++worker_id) {
-    Worker *w = create_worker(cct, type, worker_id);
-    int ret = w->center.init(InitEventNumber, worker_id, type);
-    if (ret)
-      throw std::system_error(-ret, std::generic_category());
-    workers.push_back(w);
-  }
 }
 
 void NetworkStack::start()

diff --git a/src/msg/async/Stack.h b/src/msg/async/Stack.h
index 7b8b62f36fb1f592a0b94194192b3708fe9be911..1bf4baa519c729c9dc18c222d8c5643c3eaccca7 100644 (file)
--- a/src/msg/async/Stack.h
+++ b/src/msg/async/Stack.h
@@ -300,6 +300,8 @@ class NetworkStack {
 
   std::function<void ()> add_thread(unsigned i);
 
+  virtual Worker* create_worker(CephContext *c, unsigned i) = 0;
+
  protected:
   CephContext *cct;
   std::vector<Worker*> workers;
@@ -316,8 +318,6 @@ class NetworkStack {
   static std::shared_ptr<NetworkStack> create(
     CephContext *c, const std::string &type);
 
-  static Worker* create_worker(
-    CephContext *c, const std::string &t, unsigned i);
   // backend need to override this method if backend doesn't support shared
   // listen table.
   // For example, posix backend has in kernel global listen table. If one

diff --git a/src/msg/async/dpdk/DPDKStack.h b/src/msg/async/dpdk/DPDKStack.h
index 37626bee492008a98449f10671274efab43ed59b..43ae8003c39c0139b54dba42838b3c7ee653a945 100644 (file)
--- a/src/msg/async/dpdk/DPDKStack.h
+++ b/src/msg/async/dpdk/DPDKStack.h
@@ -248,6 +248,11 @@ class DPDKWorker : public Worker {
 
 class DPDKStack : public NetworkStack {
   vector<std::function<void()> > funcs;
+
+  virtual Worker* create_worker(CephContext *c, unsigned worker_id) override {
+    return new DPDKWorker(c, worker_id);
+  }
+
  public:
   explicit DPDKStack(CephContext *cct, const string &t): NetworkStack(cct, t) {
     funcs.resize(cct->_conf->ms_async_max_op_threads);

diff --git a/src/msg/async/rdma/RDMAStack.h b/src/msg/async/rdma/RDMAStack.h
index ac33fcae8e7877047fb6fbfb50963efa489c5948..d2eafb74c14fc59633b5b323d7d5999c19bbb32e 100644 (file)
--- a/src/msg/async/rdma/RDMAStack.h
+++ b/src/msg/async/rdma/RDMAStack.h
@@ -326,6 +326,10 @@ class RDMAStack : public NetworkStack {
 
   std::atomic<bool> fork_finished = {false};
 
+  virtual Worker* create_worker(CephContext *c, unsigned worker_id) override {
+    return new RDMAWorker(c, worker_id);
+  }
+
  public:
   explicit RDMAStack(CephContext *cct, const std::string &t);
   virtual ~RDMAStack();