Merge pull request ceph#63594 from ifed01/wip-ifed-max-perf-count

ifed01 · web-flow · commit e4f4743cee57 · 2025-08-21T22:13:21.000+03:00
common/perf_counters: track maximum increment value for LONGRUNAVG perf

Reviewed-by: Ronnen Friedman &lt;rfriedma@ibm.com&gt;
diff --git a/src/common/perf_counters.cc b/src/common/perf_counters.cc
@@ -183,6 +183,31 @@ void PerfCounters::inc(int idx, uint64_t amt)
   }
 }
 
+void PerfCounters::inc_with_max(int idx, uint64_t amt)
+{
+#ifndef WITH_CRIMSON
+  if (!m_cct->_conf->perf)
+    return;
+#endif
+
+  ceph_assert(idx > m_lower_bound);
+  ceph_assert(idx < m_upper_bound);
+  perf_counter_data_any_d& data(m_data[idx - m_lower_bound - 1]);
+  if (!(data.type & PERFCOUNTER_U64))
+    return;
+  if (data.type & PERFCOUNTER_LONGRUNAVG) {
+    data.avgcount++;
+    data.u64 += amt;
+    uint64_t m;
+    do {
+      m = data.max_u64_inc.load();
+    } while(amt > m && !data.max_u64_inc.compare_exchange_weak(m, amt));
+    data.avgcount2++;
+  } else {
+    data.u64 += amt;
+  }
+}
+
 void PerfCounters::dec(int idx, uint64_t amt)
 {
 #ifndef WITH_CRIMSON
@@ -259,6 +284,32 @@ void PerfCounters::tinc(int idx, utime_t amt)
   }
 }
 
+void PerfCounters::tinc_with_max(int idx, utime_t amt)
+{
+#ifndef WITH_CRIMSON
+  if (!m_cct->_conf->perf)
+    return;
+#endif
+
+  ceph_assert(idx > m_lower_bound);
+  ceph_assert(idx < m_upper_bound);
+  perf_counter_data_any_d& data(m_data[idx - m_lower_bound - 1]);
+  if (!(data.type & PERFCOUNTER_TIME))
+    return;
+  if (data.type & PERFCOUNTER_LONGRUNAVG) {
+    uint64_t new_m = amt.to_nsec();
+    data.avgcount++;
+    data.u64 += new_m;
+    uint64_t m;
+    do {
+      m = data.max_u64_inc.load();
+    } while(new_m > m && !data.max_u64_inc.compare_exchange_weak(m, new_m));
+    data.avgcount2++;
+  } else {
+    data.u64 += amt.to_nsec();
+  }
+}
+
 void PerfCounters::tinc(int idx, ceph::timespan amt)
 {
 #ifndef WITH_CRIMSON
@@ -280,6 +331,32 @@ void PerfCounters::tinc(int idx, ceph::timespan amt)
   }
 }
 
+void PerfCounters::tinc_with_max(int idx, ceph::timespan amt)
+{
+#ifndef WITH_CRIMSON
+  if (!m_cct->_conf->perf)
+    return;
+#endif
+
+  ceph_assert(idx > m_lower_bound);
+  ceph_assert(idx < m_upper_bound);
+  perf_counter_data_any_d& data(m_data[idx - m_lower_bound - 1]);
+  if (!(data.type & PERFCOUNTER_TIME))
+    return;
+  if (data.type & PERFCOUNTER_LONGRUNAVG) {
+    uint64_t new_m = amt.count();
+    data.avgcount++;
+    data.u64 += new_m;
+    uint64_t m;
+    do {
+      m = data.max_u64_inc.load();
+    } while(new_m > m && !data.max_u64_inc.compare_exchange_weak(m, new_m));
+    data.avgcount2++;
+  } else {
+    data.u64 += amt.count();
+  }
+}
+
 void PerfCounters::tset(int idx, utime_t amt)
 {
 #ifndef WITH_CRIMSON
@@ -361,8 +438,7 @@ pair<uint64_t, uint64_t> PerfCounters::get_tavg_ns(int idx) const
     return make_pair(0, 0);
   if (!(data.type & PERFCOUNTER_LONGRUNAVG))
     return make_pair(0, 0);
-  pair<uint64_t,uint64_t> a = data.read_avg();
-  return make_pair(a.second, a.first);
+  return data.read_avg();
 }
 
 void PerfCounters::reset()
@@ -479,17 +555,27 @@ void PerfCounters::dump_formatted_generic(Formatter *f, bool schema,
     } else {
       if (d->type & PERFCOUNTER_LONGRUNAVG) {
         Formatter::ObjectSection longrunavg_section{*f, d->name};
-	pair<uint64_t,uint64_t> a = d->read_avg();
+	std::tuple<uint64_t,uint64_t,uint64_t> a = d->read_avg_ex();
 	if (d->type & PERFCOUNTER_U64) {
-	  f->dump_unsigned("avgcount", a.second);
-	  f->dump_unsigned("sum", a.first);
+	  f->dump_unsigned("sum", std::get<0>(a));
+	  f->dump_unsigned("avgcount", std::get<1>(a));
+          uint64_t max = std::get<2>(a);
+          if (max != 0) {
+	    f->dump_unsigned("max_inc", std::get<2>(a));
+          }
 	} else if (d->type & PERFCOUNTER_TIME) {
-	  f->dump_unsigned("avgcount", a.second);
+          uint64_t sum_ns = std::get<0>(a);
+          uint64_t count = std::get<1>(a);
+	  f->dump_unsigned("avgcount", count);
 	  f->dump_format_unquoted("sum", "%" PRId64 ".%09" PRId64,
-				  a.first / 1000000000ull,
-				  a.first % 1000000000ull);
-          uint64_t count = a.second;
-          uint64_t sum_ns = a.first;
+				  sum_ns / 1000000000ull,
+				  sum_ns % 1000000000ull);
+          uint64_t max_ns = std::get<2>(a);
+          if (max_ns != 0) {
+	    f->dump_format_unquoted("max_inc", "%" PRId64 ".%09" PRId64,
+				    max_ns / 1000000000ull,
+				    max_ns % 1000000000ull);
+          }
           if (count) {
             uint64_t avg_ns = sum_ns / count;
             f->dump_format_unquoted("avgtime", "%" PRId64 ".%09" PRId64,
diff --git a/src/common/perf_counters.h b/src/common/perf_counters.h
@@ -22,6 +22,7 @@
 #include <set>
 #include <string>
 #include <vector>
+#include <tuple>
 #include <memory>
 #include <atomic>
 #include <cstdint>
@@ -178,12 +179,10 @@ class PerfCounters
         description(other.description),
         nick(other.nick),
 	 type(other.type),
-	 unit(other.unit),
-	 u64(other.u64.load()) {
-      auto a = other.read_avg();
-      u64 = a.first;
-      avgcount = a.second;
-      avgcount2 = a.second;
+	 unit(other.unit) {
+      std::tie(u64, avgcount, max_u64_inc) = other.read_avg_ex();
+      avgcount2 = avgcount.load();
+
       if (other.histogram) {
         histogram.reset(new PerfHistogram<>(*other.histogram));
       }
@@ -196,6 +195,7 @@ class PerfCounters
     enum perfcounter_type_d type;
     enum unit_t unit;
     std::atomic<uint64_t> u64 = { 0 };
+    std::atomic<uint64_t> max_u64_inc = { 0 };
     std::atomic<uint64_t> avgcount = { 0 };
     std::atomic<uint64_t> avgcount2 = { 0 };
     std::unique_ptr<PerfHistogram<>> histogram;
@@ -204,6 +204,7 @@ class PerfCounters
     {
       if (type != PERFCOUNTER_U64) {
 	    u64 = 0;
+	    max_u64_inc = 0;
 	    avgcount = 0;
 	    avgcount2 = 0;
       }
@@ -223,6 +224,15 @@ class PerfCounters
       } while (avgcount != count);
       return { sum, count };
     }
+    std::tuple<uint64_t,uint64_t, uint64_t> read_avg_ex() const {
+      uint64_t _sum, _count, _max;
+      do {
+	_count = avgcount2;
+	_sum = u64;
+	_max = max_u64_inc;
+      } while (avgcount != _count);
+      return { _sum, _count, _max };
+    }
   };
 
   template <typename T>
@@ -244,6 +254,7 @@ class PerfCounters
   ~PerfCounters();
 
   void inc(int idx, uint64_t v = 1);
+  void inc_with_max(int idx, uint64_t v = 1);
   void dec(int idx, uint64_t v = 1);
   void set(int idx, uint64_t v);
   uint64_t get(int idx) const;
@@ -252,6 +263,8 @@ class PerfCounters
   void tset(int idx, ceph::timespan v);
   void tinc(int idx, utime_t v);
   void tinc(int idx, ceph::timespan v);
+  void tinc_with_max(int idx, utime_t v);
+  void tinc_with_max(int idx, ceph::timespan v);
   utime_t tget(int idx) const;
 
   void hinc(int idx, int64_t x, int64_t y);
diff --git a/src/os/bluestore/BlueFS.cc b/src/os/bluestore/BlueFS.cc
@@ -457,18 +457,6 @@ void BlueFS::_init_logger()
                 "Average allocation latency for primary/shared device",
                 "bsal",
                 PerfCountersBuilder::PRIO_USEFUL);
-  b.add_time(l_bluefs_wal_alloc_max_lat, "alloc_wal_max_lat",
-             "Max allocation latency for wal device",
-             "awxt",
-             PerfCountersBuilder::PRIO_INTERESTING);
-  b.add_time(l_bluefs_db_alloc_max_lat, "alloc_db_max_lat",
-             "Max allocation latency for db device",
-             "adxt",
-             PerfCountersBuilder::PRIO_INTERESTING);
-  b.add_time(l_bluefs_slow_alloc_max_lat, "alloc_slow_max_lat",
-             "Max allocation latency for primary/shared device",
-             "asxt",
-             PerfCountersBuilder::PRIO_INTERESTING);
 
   logger = b.create_perf_counters();
   cct->get_perfcounters_collection()->add(logger);
@@ -4355,25 +4343,13 @@ void BlueFS::_update_allocate_stats(uint8_t id, const ceph::timespan& d)
 {
   switch(id) {
     case BDEV_SLOW:
-      logger->tinc(l_bluefs_slow_alloc_lat, d);
-      if (d > max_alloc_lat[id]) {
-        logger->tset(l_bluefs_slow_alloc_max_lat, utime_t(d));
-        max_alloc_lat[id] = d;
-      }
+      logger->tinc_with_max(l_bluefs_slow_alloc_lat, d);
       break;
     case BDEV_DB:
-      logger->tinc(l_bluefs_db_alloc_lat, d);
-      if (d > max_alloc_lat[id]) {
-        logger->tset(l_bluefs_db_alloc_max_lat, utime_t(d));
-        max_alloc_lat[id] = d;
-      }
+      logger->tinc_with_max(l_bluefs_db_alloc_lat, d);
       break;
     case BDEV_WAL:
-      logger->tinc(l_bluefs_wal_alloc_lat, d);
-      if (d > max_alloc_lat[id]) {
-        logger->tset(l_bluefs_wal_alloc_max_lat, utime_t(d));
-        max_alloc_lat[id] = d;
-      }
+      logger->tinc_with_max(l_bluefs_wal_alloc_lat, d);
       break;
   }
 }
diff --git a/src/os/bluestore/BlueFS.h b/src/os/bluestore/BlueFS.h
@@ -86,9 +86,6 @@ enum {
   l_bluefs_wal_alloc_lat,
   l_bluefs_db_alloc_lat,
   l_bluefs_slow_alloc_lat,
-  l_bluefs_wal_alloc_max_lat,
-  l_bluefs_db_alloc_max_lat,
-  l_bluefs_slow_alloc_max_lat,
   l_bluefs_last,
 };
 
@@ -557,8 +554,6 @@ class BlueFS {
     l_bluefs_max_bytes_db,
   };
 
-  ceph::timespan max_alloc_lat[MAX_BDEV] = {ceph::make_timespan(0)};
-
   // cache
   struct {
     ceph::mutex lock = ceph::make_mutex("BlueFS::nodes.lock");
diff --git a/src/test/perf_counters.cc b/src/test/perf_counters.cc
@@ -123,6 +123,13 @@ TEST(PerfCounters, SinglePerfCounters) {
   ASSERT_EQ(sd("{\"test_perfcounter_1\":{\"element1\":1,\"element2\":0.500000000,"
 	    "\"element3\":{\"avgcount\":3,\"sum\":120.000000000,\"avgtime\":40.000000000}}}"), msg);
 
+  fake_pf->reset();
+  fake_pf->tinc_with_max(TEST_PERFCOUNTERS1_ELEMENT_3, utime_t(100, 0));
+  fake_pf->tinc_with_max(TEST_PERFCOUNTERS1_ELEMENT_3, utime_t(50, 0));
+  ASSERT_EQ("", client.do_request("{ \"prefix\": \"perf dump\", \"format\": \"json\" }", &msg));
+  ASSERT_EQ(sd("{\"test_perfcounter_1\":{\"element1\":1,"
+	    "\"element2\":0.000000000,\"element3\":{\"avgcount\":2,\"sum\":150.000000000,\"max_inc\":100.000000000,\"avgtime\":75.000000000}}}"), msg);
+
   fake_pf->reset();
   msg.clear();
   ASSERT_EQ("", client.do_request("{ \"prefix\": \"perf dump\", \"format\": \"json\" }", &msg));
