Merge PR ceph#60174 into main

* refs/pull/60174/head:
	common/Finisher: pass name as std::string_view to ctor
	common/Finisher: add method get_thread_name()
	mgr/ActivePyModule: build thread name with fmt
	mgr/ActivePyModule: return std::string_view instead of std::string copy
	common/Finisher: use fmt to build strings
	common/Finisher: un-inline ctor and dtor
	common/Finisher: add `const` to several fields
	common/Finisher: merge duplicate field initializers
	common/Finisher: call notify_one() instead of notify_all()
	common/Finisher: wake up after pushing to the queue
	common/Finisher: do not wake up the thread if already running
	common/Finisher: call logger without holding the lock
	common/Finisher: use `std::lock_guard` instead of `std::unique_lock`
	common/Finisher: merge all queue() container methods into one template

Reviewed-by: Patrick Donnelly <[email protected]>

Author: batrick

src/common/Finisher.cc

@@ -2,11 +2,40 @@
 // vim: ts=8 sw=2 smarttab
 
 #include "Finisher.h"
+#include "common/perf_counters.h"
+
+#include <fmt/core.h>
 
 #define dout_subsys ceph_subsys_finisher
 #undef dout_prefix
 #define dout_prefix *_dout << "finisher(" << this << ") "
 
+Finisher::Finisher(CephContext *cct_) :
+  cct(cct_), finisher_lock(ceph::make_mutex("Finisher::finisher_lock")),
+  thread_name("fn_anonymous"),
+  finisher_thread(this) {}
+
+Finisher::Finisher(CephContext *cct_, std::string_view name, std::string &&tn) :
+  cct(cct_), finisher_lock(ceph::make_mutex(fmt::format("Finisher::{}", name))),
+  thread_name(std::move(tn)),
+  finisher_thread(this) {
+  PerfCountersBuilder b(cct, fmt::format("finisher-{}", name),
+			l_finisher_first, l_finisher_last);
+  b.add_u64(l_finisher_queue_len, "queue_len");
+  b.add_time_avg(l_finisher_complete_lat, "complete_latency");
+  logger = b.create_perf_counters();
+  cct->get_perfcounters_collection()->add(logger);
+  logger->set(l_finisher_queue_len, 0);
+  logger->set(l_finisher_complete_lat, 0);
+}
+
+Finisher::~Finisher() {
+  if (logger && cct) {
+    cct->get_perfcounters_collection()->remove(logger);
+    delete logger;
+  }
+}
+
 void Finisher::start()
 {
   ldout(cct, 10) << __func__ << dendl;
@@ -20,7 +49,7 @@ void Finisher::stop()
   finisher_stop = true;
   // we don't have any new work to do, but we want the worker to wake up anyway
   // to process the stop condition.
-  finisher_cond.notify_all();
+  finisher_cond.notify_one();
   finisher_lock.unlock();
   finisher_thread.join(); // wait until the worker exits completely
   ldout(cct, 10) << __func__ << " finish" << dendl;
@@ -40,7 +69,7 @@ void Finisher::wait_for_empty()
 
 bool Finisher::is_empty()
 {
-  std::unique_lock ul(finisher_lock);
+  const std::lock_guard l{finisher_lock};
   return finisher_queue.empty();
 }
 

src/common/Finisher.h

@@ -19,10 +19,8 @@
 #include "include/common_fwd.h"
 #include "common/Thread.h"
 #include "common/ceph_mutex.h"
-#include "common/perf_counters.h"
 #include "common/Cond.h"
 
-
 /// Finisher queue length performance counter ID.
 enum {
   l_finisher_first = 997082,
@@ -37,23 +35,23 @@ enum {
  * contexts to complete is thread-safe.
  */
 class Finisher {
-  CephContext *cct;
+  CephContext *const cct;
   ceph::mutex finisher_lock; ///< Protects access to queues and finisher_running.
   ceph::condition_variable finisher_cond; ///< Signaled when there is something to process.
   ceph::condition_variable finisher_empty_cond; ///< Signaled when the finisher has nothing more to process.
-  bool         finisher_stop; ///< Set when the finisher should stop.
-  bool         finisher_running; ///< True when the finisher is currently executing contexts.
-  bool	       finisher_empty_wait; ///< True mean someone wait finisher empty.
+  bool         finisher_stop = false; ///< Set when the finisher should stop.
+  bool         finisher_running = false; ///< True when the finisher is currently executing contexts.
+  bool	       finisher_empty_wait = false; ///< True mean someone wait finisher empty.
 
   /// Queue for contexts for which complete(0) will be called.
   std::vector<std::pair<Context*,int>> finisher_queue;
   std::vector<std::pair<Context*,int>> in_progress_queue;
 
-  std::string thread_name;
+  const std::string thread_name;
 
   /// Performance counter for the finisher's queue length.
   /// Only active for named finishers.
-  PerfCounters *logger;
+  PerfCounters *logger = nullptr;
 
   void *finisher_thread_entry();
 
@@ -66,56 +64,34 @@ class Finisher {
  public:
   /// Add a context to complete, optionally specifying a parameter for the complete function.
   void queue(Context *c, int r = 0) {
-    std::unique_lock ul(finisher_lock);
-    bool was_empty = finisher_queue.empty();
-    finisher_queue.push_back(std::make_pair(c, r));
-    if (was_empty) {
-      finisher_cond.notify_one();
+    {
+      const std::lock_guard l{finisher_lock};
+      const bool should_notify = finisher_queue.empty() && !finisher_running;
+      finisher_queue.push_back(std::make_pair(c, r));
+      if (should_notify) {
+	finisher_cond.notify_one();
+      }
     }
+
     if (logger)
       logger->inc(l_finisher_queue_len);
   }
 
-  void queue(std::list<Context*>& ls) {
+  // TODO use C++20 concept checks instead of SFINAE
+  template<typename T>
+  auto queue(T &ls) -> decltype(std::distance(ls.begin(), ls.end()), void()) {
     {
-      std::unique_lock ul(finisher_lock);
-      if (finisher_queue.empty()) {
-	finisher_cond.notify_all();
-      }
-      for (auto i : ls) {
-	finisher_queue.push_back(std::make_pair(i, 0));
-      }
-      if (logger)
-	logger->inc(l_finisher_queue_len, ls.size());
-    }
-    ls.clear();
-  }
-  void queue(std::deque<Context*>& ls) {
-    {
-      std::unique_lock ul(finisher_lock);
-      if (finisher_queue.empty()) {
-	finisher_cond.notify_all();
-      }
-      for (auto i : ls) {
+      const std::lock_guard l{finisher_lock};
+      const bool should_notify = finisher_queue.empty() && !finisher_running;
+      for (Context *i : ls) {
 	finisher_queue.push_back(std::make_pair(i, 0));
       }
-      if (logger)
-	logger->inc(l_finisher_queue_len, ls.size());
-    }
-    ls.clear();
-  }
-  void queue(std::vector<Context*>& ls) {
-    {
-      std::unique_lock ul(finisher_lock);
-      if (finisher_queue.empty()) {
-	finisher_cond.notify_all();
+      if (should_notify) {
+	finisher_cond.notify_one();
       }
-      for (auto i : ls) {
-	finisher_queue.push_back(std::make_pair(i, 0));
-      }
-      if (logger)
-	logger->inc(l_finisher_queue_len, ls.size());
     }
+    if (logger)
+      logger->inc(l_finisher_queue_len, ls.size());
     ls.clear();
   }
 
@@ -137,36 +113,17 @@ class Finisher {
 
   bool is_empty();
 
+  std::string_view get_thread_name() const noexcept {
+    return thread_name;
+  }
+
   /// Construct an anonymous Finisher.
   /// Anonymous finishers do not log their queue length.
-  explicit Finisher(CephContext *cct_) :
-    cct(cct_), finisher_lock(ceph::make_mutex("Finisher::finisher_lock")),
-    finisher_stop(false), finisher_running(false), finisher_empty_wait(false),
-    thread_name("fn_anonymous"), logger(0),
-    finisher_thread(this) {}
+  explicit Finisher(CephContext *cct_);
 
   /// Construct a named Finisher that logs its queue length.
-  Finisher(CephContext *cct_, std::string name, std::string tn) :
-    cct(cct_), finisher_lock(ceph::make_mutex("Finisher::" + name)),
-    finisher_stop(false), finisher_running(false), finisher_empty_wait(false),
-    thread_name(tn), logger(0),
-    finisher_thread(this) {
-    PerfCountersBuilder b(cct, std::string("finisher-") + name,
-			  l_finisher_first, l_finisher_last);
-    b.add_u64(l_finisher_queue_len, "queue_len");
-    b.add_time_avg(l_finisher_complete_lat, "complete_latency");
-    logger = b.create_perf_counters();
-    cct->get_perfcounters_collection()->add(logger);
-    logger->set(l_finisher_queue_len, 0);
-    logger->set(l_finisher_complete_lat, 0);
-  }
-
-  ~Finisher() {
-    if (logger && cct) {
-      cct->get_perfcounters_collection()->remove(logger);
-      delete logger;
-    }
-  }
+  Finisher(CephContext *cct_, std::string_view name, std::string &&tn);
+  ~Finisher();
 };
 
 /// Context that is completed asynchronously on the supplied finisher.

src/mgr/ActivePyModule.h

@@ -27,6 +27,8 @@
 #include "PyModuleRunner.h"
 #include "PyModule.h"
 
+#include <fmt/core.h>
+
 #include <vector>
 #include <string>
 
@@ -46,16 +48,14 @@ class ActivePyModule : public PyModuleRunner
 
   std::string m_command_perms;
   const MgrSession* m_session = nullptr;
-  std::string fin_thread_name;
 public:
   Finisher finisher; // per active module finisher to execute commands
 
 public:
   ActivePyModule(const PyModuleRef &py_module_,
       LogChannelRef clog_)
     : PyModuleRunner(py_module_, clog_),
-      fin_thread_name(std::string("m-fin-" + py_module->get_name()).substr(0,15)),
-      finisher(g_ceph_context, thread_name, fin_thread_name)
+      finisher(g_ceph_context, thread_name, fmt::format("m-fin-{}", py_module->get_name()).substr(0,15))
 
   {
   }
@@ -97,14 +97,14 @@ class ActivePyModule : public PyModuleRunner
     uri = str;
   }
 
-  std::string get_uri() const
+  std::string_view get_uri() const
   {
     return uri;
   }
 
-  std::string get_fin_thread_name() const
+  std::string_view get_fin_thread_name() const
   {
-    return fin_thread_name;
+    return finisher.get_thread_name();
   }
 
   bool is_authorized(const std::map<std::string, std::string>& arguments) const;

src/mgr/ActivePyModules.cc

@@ -770,9 +770,9 @@ std::map<std::string, std::string> ActivePyModules::get_services() const
   std::map<std::string, std::string> result;
   std::lock_guard l(lock);
   for (const auto& [name, module] : modules) {
-    std::string svc_str = module->get_uri();
+    const std::string_view svc_str = module->get_uri();
     if (!svc_str.empty()) {
-      result[name] = svc_str;
+      result.emplace(name, svc_str);
     }
   }