Merge pull request ceph#58115 from ronen-fr/wip-rf-memmodel

ronen-fr · web-flow · commit 88f123005546 · 2024-08-05T14:59:38.000+03:00
common: MemoryModel: performance improvements and API changes

Reviewed-by: Venky Shankar &lt;vshankar@redhat.com&gt;
Reviewed-by: Milind Changire &lt;mchangir@redhat.com&gt;
diff --git a/src/common/MemoryModel.cc b/src/common/MemoryModel.cc
@@ -1,96 +1,140 @@
-#include "MemoryModel.h"
-#include "include/compat.h"
 #include "debug.h"
+
+#include "include/compat.h"
+
+#include "MemoryModel.h"
 #if defined(__linux__)
 #include <malloc.h>
 #endif
 
-#include <fstream>
+#include <charconv>
+
+#include "common/fmt_common.h"
+
 
 #define dout_subsys ceph_subsys_
 
 using namespace std;
+using mem_snap_t = MemoryModel::mem_snap_t;
 
-MemoryModel::MemoryModel(CephContext *cct_)
-  : cct(cct_)
+inline bool MemoryModel::cmp_against(
+    const std::string &ln,
+    std::string_view param,
+    long &v) const
 {
+  if (ln.size() < (param.size() + 10)) {
+    return false;
+  }
+  if (ln.starts_with(param)) {
+    auto p = ln.c_str();
+    auto s = p + param.size();
+    // charconv does not like leading spaces
+    while (*s && isblank(*s)) {
+      s++;
+    }
+    from_chars(s, p + ln.size(), v);
+    return true;
+  }
+  return false;
 }
 
-void MemoryModel::_sample(snap *psnap)
-{
-  ifstream f;
 
-  f.open(PROCPREFIX "/proc/self/status");
-  if (!f.is_open()) {
-    ldout(cct, 0) << "check_memory_usage unable to open " PROCPREFIX "/proc/self/status" << dendl;
-    return;
-  }
-  while (!f.eof()) {
-    string line;
-    getline(f, line);
-    
-    if (strncmp(line.c_str(), "VmSize:", 7) == 0)
-      psnap->size = atol(line.c_str() + 7);
-    else if (strncmp(line.c_str(), "VmRSS:", 6) == 0)
-      psnap->rss = atol(line.c_str() + 7);
-    else if (strncmp(line.c_str(), "VmHWM:", 6) == 0)
-      psnap->hwm = atol(line.c_str() + 7);
-    else if (strncmp(line.c_str(), "VmLib:", 6) == 0)
-      psnap->lib = atol(line.c_str() + 7);
-    else if (strncmp(line.c_str(), "VmPeak:", 7) == 0)
-      psnap->peak = atol(line.c_str() + 7);
-    else if (strncmp(line.c_str(), "VmData:", 7) == 0)
-      psnap->data = atol(line.c_str() + 7);
+tl::expected<int64_t, std::string> MemoryModel::get_mapped_heap()
+{
+  if (!proc_maps.is_open()) {
+    return tl::unexpected("unable to open proc/maps");
   }
-  f.close();
+  // always rewind before reading
+  proc_maps.clear();
+  proc_maps.seekg(0);
 
-  f.open(PROCPREFIX "/proc/self/maps");
-  if (!f.is_open()) {
-    ldout(cct, 0) << "check_memory_usage unable to open " PROCPREFIX "/proc/self/maps" << dendl;
-    return;
-  }
+  int64_t heap = 0;
 
-  long heap = 0;
-  while (f.is_open() && !f.eof()) {
+  while (proc_maps.is_open() && !proc_maps.eof()) {
     string line;
-    getline(f, line);
-    //ldout(cct, 0) << "line is " << line << dendl;
+    getline(proc_maps, line);
 
-    const char *start = line.c_str();
-    const char *dash = start;
-    while (*dash && *dash != '-') dash++;
+    if (line.length() < 48) {
+      // a malformed line. We expect at least
+      // '560c03f8d000-560c03fae000 rw-p 00000000 00:00 0'
+      continue;
+    }
+
+    const char* start = line.c_str();
+    const char* dash = start;
+    while (*dash && *dash != '-')
+      dash++;
     if (!*dash)
       continue;
-    const char *end = dash + 1;
-    while (*end && *end != ' ') end++;
+    const char* end = dash + 1;
+    while (*end && *end != ' ')
+      end++;
     if (!*end)
       continue;
-    unsigned long long as = strtoll(start, 0, 16);
-    unsigned long long ae = strtoll(dash+1, 0, 16);
-
-    //ldout(cct, 0) << std::hex << as << " to " << ae << std::dec << dendl;
 
+    auto addr_end = end;
     end++;
-    const char *mode = end;
-
-    int skip = 4;
-    while (skip--) {
-      end++;
-      while (*end && *end != ' ') end++;
-    }
-    if (*end)
-      end++;
-
-    long size = ae - as;
-    //ldout(cct, 0) << "size " << size << " mode is '" << mode << "' end is '" << end << "'" << dendl;
+    const char* mode = end;
 
     /*
      * anything 'rw' and anon is assumed to be heap.
+     * But we should count lines with inode '0' and '[heap]' as well
      */
-    if (mode[0] == 'r' && mode[1] == 'w' && !*end)
+    if (mode[0] != 'r' || mode[1] != 'w') {
+      continue;
+    }
+
+    auto the_rest = line.substr(5 + end - start);
+    if (!the_rest.starts_with("00000000 00:00 0")) {
+      continue;
+    }
+
+    std::string_view final_token{the_rest.begin() + sizeof("00000000 00:00 0") - 1,
+                                 the_rest.end()};
+    if (final_token.size() < 3 ||
+        final_token.ends_with("[heap]") || final_token.ends_with("[stack]")) {
+      // calculate and sum the size of the heap segment
+      uint64_t as{0ull};
+      from_chars(start, dash, as, 16);
+      uint64_t ae{0ull};
+      from_chars(dash + 1, addr_end, ae, 16);
+      //     fmt::print("\t\tas:{:x} ae:{:x} -> {}\n", as, ae, ((ae - as) >> 10));
+      long size = ae - as;
       heap += size;
+    }
   }
 
-  psnap->heap = heap >> 10;
+  return heap;
+}
+
+
+tl::expected<mem_snap_t, std::string> MemoryModel::full_sample()
+{
+  if (!proc_status.is_open()) {
+    return tl::unexpected("unable to open proc/status");
+  }
+  // always rewind before reading
+  proc_status.clear();
+  proc_status.seekg(0);
+
+  mem_snap_t s;
+  // we will be looking for 6 entries
+  int yet_to_find = 6;
+
+  while (!proc_status.eof() && yet_to_find > 0) {
+    string ln;
+    getline(proc_status, ln);
+
+    if (cmp_against(ln, "VmSize:", s.size) ||
+	cmp_against(ln, "VmRSS:", s.rss) || cmp_against(ln, "VmHWM:", s.hwm) ||
+	cmp_against(ln, "VmLib:", s.lib) ||
+	cmp_against(ln, "VmPeak:", s.peak) ||
+	cmp_against(ln, "VmData:", s.data)) {
+      yet_to_find--;
+    }
+  }
 
+  // get heap size
+  s.heap = static_cast<long>(get_mapped_heap().value_or(0));
+  return s;
 }
diff --git a/src/common/MemoryModel.h b/src/common/MemoryModel.h
@@ -15,40 +15,67 @@
 #ifndef CEPH_MEMORYMODEL_H
 #define CEPH_MEMORYMODEL_H
 
+#include <fstream>
+#include <string>
+#include <string_view>
 #include "include/common_fwd.h"
+#include "include/compat.h"
+#include "include/expected.hpp"
+
 
 class MemoryModel {
 public:
-  struct snap {
-    long peak;
-    long size;
-    long hwm;
-    long rss;
-    long data;
-    long lib;
-    
-    long heap;
-
-    snap() : peak(0), size(0), hwm(0), rss(0), data(0), lib(0),
-	     heap(0)
-    {}
-
-    long get_total() { return size; }
-    long get_rss() { return rss; }
-    long get_heap() { return heap; }
-  } last;
+  struct mem_snap_t {
+    long peak{0};
+    long size{0};
+    long hwm{0};
+    long rss{0};
+    long data{0};
+    long lib{0};
+    long heap{0};
+
+    long get_total() const { return size; }
+    long get_rss() const { return rss; }
+    long get_heap() const { return heap; }
+  };
 
 private:
-  CephContext *cct;
-  void _sample(snap *p);
+  static inline constexpr const char* proc_stat_fn = PROCPREFIX "/proc/self/status";
+  static inline constexpr const char* proc_maps_fn = PROCPREFIX "/proc/self/maps";
+
+  std::ifstream proc_status{proc_stat_fn};
+  std::ifstream proc_maps{proc_maps_fn};
+
+  /**
+   * @brief Get the mapped heap size
+   *
+   * Read /proc/self/maps to get the heap size.
+   * \retval the mapped heap size, or an error message if the file had not been opened
+   *    when the object was constructed.
+   */
+  tl::expected<int64_t, std::string> get_mapped_heap();
+
+  /**
+   * @brief Compare a line against an expected data label
+   *
+   * If the line starts with the expected label, extract the value and store it in v.
+   * \retval true if the line starts with the expected label
+   */
+  bool cmp_against(const std::string& ln, std::string_view param, long& v) const;
 
 public:
-  explicit MemoryModel(CephContext *cct);
-  void sample(snap *p = 0) {
-    _sample(&last);
-    if (p)
-      *p = last;
-  }
+  /**
+   * @brief extract memory usage information from /proc/self/status &
+   *        /proc/self/maps
+   *
+   * Read /proc/self/status and /proc/self/maps to get memory usage information.
+   * \retval a structure containing the memory usage information, or an error
+   *    message if /proc/status had not been opened when the object was
+   *    constructed.
+   *    Note that no error is returned if only /proc/maps is not open (the heap
+   *    size will be reported as 0).
+   */
+  tl::expected<mem_snap_t, std::string> full_sample();
 };
 
 #endif
diff --git a/src/mds/MDCache.cc b/src/mds/MDCache.cc
@@ -46,7 +46,6 @@
 #include "msg/Message.h"
 #include "msg/Messenger.h"
 
-#include "common/MemoryModel.h"
 #include "common/errno.h"
 #include "common/perf_counters.h"
 #include "common/safe_io.h"
@@ -7832,10 +7831,15 @@ void MDCache::trim_client_leases()
 
 void MDCache::check_memory_usage()
 {
-  static MemoryModel mm(g_ceph_context);
-  static MemoryModel::snap last;
-  mm.sample(&last);
-  static MemoryModel::snap baseline = last;
+  MemoryModel::mem_snap_t memory_snap;
+
+  if (upkeep_memory_stats) {
+    auto maybe_memory_snap = upkeep_memory_stats->full_sample();
+    if (maybe_memory_snap) {
+      memory_snap = *maybe_memory_snap;
+    }
+  }
+  // else - no access to memory stats. The problem was already reported at thread's init.
 
   // check client caps
   ceph_assert(CInode::count() == inode_map.size() + snap_inode_map.size() + num_shadow_inodes);
@@ -7844,17 +7848,17 @@ void MDCache::check_memory_usage()
     caps_per_inode = (double)Capability::count() / (double)CInode::count();
 
   dout(2) << "Memory usage: "
-	   << " total " << last.get_total()
-	   << ", rss " << last.get_rss()
-	   << ", heap " << last.get_heap()
-	   << ", baseline " << baseline.get_heap()
+	   << " total " << memory_snap.get_total()
+	   << ", rss " << memory_snap.get_rss()
+	   << ", heap " << memory_snap.get_heap()
+	   << ", baseline " << upkeep_mem_baseline.get_heap()
 	   << ", " << num_inodes_with_caps << " / " << CInode::count() << " inodes have caps"
 	   << ", " << Capability::count() << " caps, " << caps_per_inode << " caps per inode"
 	   << dendl;
 
   mds->update_mlogger();
-  mds->mlogger->set(l_mdm_rss, last.get_rss());
-  mds->mlogger->set(l_mdm_heap, last.get_heap());
+  mds->mlogger->set(l_mdm_rss, memory_snap.get_rss());
+  mds->mlogger->set(l_mdm_heap, memory_snap.get_heap());
 }
 
 
@@ -14190,6 +14194,27 @@ bool MDCache::is_ready_to_trim_cache(void)
 void MDCache::upkeep_main(void)
 {
   std::unique_lock lock(upkeep_mutex);
+
+  // create a "memory model" for the upkeep thread. The object maintains
+  // the relevant '/proc' files open. We only check for a failure to open
+  // the files once, and then we'll just keep the object around.
+  upkeep_memory_stats = MemoryModel{};
+
+  // get initial sample upon thread creation
+  {
+    auto maybe_base = upkeep_memory_stats->full_sample();
+    if (!maybe_base) {
+      dout(1) << fmt::format(
+		     "{}: Failed to get initial memory sample ({}). No more "
+		     "sampling will be attempted",
+		     __func__, maybe_base.error())
+	      << dendl;
+      upkeep_memory_stats = std::nullopt;
+    } else {
+      upkeep_mem_baseline = *maybe_base;
+    }
+  }
+
   while (!upkeep_trim_shutdown.load()) {
     auto now = clock::now();
     auto since = now-upkeep_last_trim;
diff --git a/src/mds/MDCache.h b/src/mds/MDCache.h
@@ -20,6 +20,7 @@
 #include <thread>
 
 #include "common/DecayCounter.h"
+#include "common/MemoryModel.h"
 #include "include/common_fwd.h"
 #include "include/types.h"
 #include "include/filepath.h"
@@ -1539,6 +1540,8 @@ class MDCache {
   time upkeep_last_trim = clock::zero();
   time upkeep_last_release = clock::zero();
   std::atomic<bool> upkeep_trim_shutdown{false};
+  std::optional<MemoryModel> upkeep_memory_stats;
+  MemoryModel::mem_snap_t upkeep_mem_baseline;
 
   uint64_t kill_shutdown_at = 0;
 
