Add meminfo UMA collection.

metrics_daemon.cc

Index: metrics_daemon.cc
diff --git a/metrics_daemon.cc b/metrics_daemon.cc
index 4820590d1b035af0355d1d19701309df8d35bfe4..7797596cb9745e28052c4c0d589f8bf00e3f1316 100644
--- a/metrics_daemon.cc
+++ b/metrics_daemon.cc
@@ -102,6 +102,8 @@ const char MetricsDaemon::kMetricWriteSectorsShortName[] =
 const int MetricsDaemon::kMetricDiskStatsShortInterval = 1;  // seconds
 const int MetricsDaemon::kMetricDiskStatsLongInterval = 30;  // seconds
 
+const int MetricsDaemon::kMetricMeminfoInterval = 30;        // seconds
+
 // Assume a max rate of 250Mb/s for reads (worse for writes) and 512 byte
 // sectors.
 const int MetricsDaemon::kMetricSectorsIOMax = 500000;  // sectors/second
@@ -248,6 +250,9 @@ void MetricsDaemon::Init(bool testing, MetricsLibraryInterface* metrics_lib,
     DiskStatsReporterInit();
   }
 
+  // Start collecting meminfo stats.
+  ScheduleMeminfoCallback(kMetricMeminfoInterval);
+
   // Don't setup D-Bus and GLib in test mode.
   if (testing)
     return;
@@ -617,6 +622,146 @@ void MetricsDaemon::DiskStatsCallback() {
   }
 }
 
+void MetricsDaemon::ScheduleMeminfoCallback(int wait) {
+  if (testing_) {
+    return;
+  }
+  g_timeout_add_seconds(wait, MeminfoCallbackStatic, this);
+}
+
+// static
+gboolean MetricsDaemon::MeminfoCallbackStatic(void* handle) {
+  return (static_cast<MetricsDaemon*>(handle))->MeminfoCallback();
+}
+
+gboolean MetricsDaemon::MeminfoCallback() {
+  char meminfo[4096];
+  int nchars;
+  const char* meminfo_path = "/proc/meminfo";
+  int file = HANDLE_EINTR(open(meminfo_path, O_RDONLY));

[kmixter1, 2011/04/08 23:04:05]

Consider simplifying this by just using file_util::ReadFileToString - though it
doesn't give you the ability to truncate the file at 4K.

[Luigi Semenzato, 2011/04/11 16:28:26]

That's good, thanks.  Output of /proc entries is limited to 4k anyway.

+  if (file < 0) {
+    PLOG(WARNING) << "cannot open " << meminfo_path;
+    return false;
+  }
+  nchars = HANDLE_EINTR(read(file, meminfo, sizeof(meminfo)));
+  HANDLE_EINTR(close(file));
+
+  if (nchars < 0) {
+    LOG(WARNING) << "cannot read from " << meminfo_path;
+    return false;
+  }
+  if (nchars == sizeof(meminfo)) {
+    LOG(WARNING) << meminfo_path << " was truncated";
+    nchars -= 1;
+  }
+  meminfo[nchars] = '\0';
+  return ProcessMeminfo(meminfo);
+}
+
+gboolean MetricsDaemon::ProcessMeminfo(const char* meminfo) {
+  /* This array has one string for every item of /proc/meminfo that we want to

[kmixter1, 2011/04/08 23:04:05]

Local style is to use // here and below.

[Luigi Semenzato, 2011/04/11 16:28:26]

Cool, thanks.

+   * report to UMA.  They must be listed in the same order in which
+   * /proc/meminfo prints them.
+   */
+  struct {
+    const char* name;   /* print name */
+    const char* match;  /* string to match in output of proc/meminfo */
+    int log_scale;      /* report with log scale instead of linear percent */
+  } fields[] = {
+    { "MemTotal",    "MemTotal" },  // SPECIAL CASE: total system memory

[kmixter1, 2011/04/08 23:04:05]

This change increases our current number of histograms by a large %.  Note that
you'll need to add all of these to internal chrome source's xml separately.

[Luigi Semenzato, 2011/04/11 16:28:26]

Yes---separate CL (different repo)

+    { "MemFree",     "MemFree"  },
+    { "Buffers",     "Buffers" },
+    { "Cached",      "Cached" },
+    // { "SwapCached",  "SwapCached" },

[kmixter1, 2011/04/08 23:04:05]

Seems like if you're going to add spaces for alignment (which is not commonly
done) you should make sure the commented out lines also line up.

[Luigi Semenzato, 2011/04/11 16:28:26]

Off with the spaces!

+    { "Active",       "Active" },
+    { "Inactive",     "Inactive" },
+    { "ActiveAnon",   "Active(anon)"   },
+    { "InactiveAnon", "Inactive(anon)" },
+    { "ActiveFile" ,  "Active(file)"   },
+    { "InactiveFile", "Inactive(file)" },
+    { "Unevictable",  "Unevictable", 1 },
+    // { "Mlocked",      "Mlocked" },
+    // { "SwapTotal",    "SwapTotal" },
+    // { "SwapFree",     "SwapFree" },
+    // { "Dirty",        "Dirty" },
+    // { "Writeback",    "Writeback" },
+    { "AnonPages",    "AnonPages" },
+    { "Mapped",       "Mapped" },
+    { "Shmem",        "Shmem", 1 },
+    { "Slab",         "Slab",  1 },

[kmixter1, 2011/04/08 23:04:05]

Suggest either getting rid of spaces for alignment or making the log_scale
columns align too.

[Luigi Semenzato, 2011/04/11 16:28:26]

Getting rid!

+    // { "SReclaimable", "SReclaimable" },
+    // { "SUnreclaim",   "SUnreclaim" },
+  };
+  const int nfields = sizeof(fields) / sizeof(fields[0]);

[kmixter1, 2011/04/08 23:04:05]

arraysize(fields)

[Luigi Semenzato, 2011/04/11 16:28:26]

Yes!

+  int total_memory = 0;
+
+  /* Scan meminfo output and collect field values.  Each field name has to
+   * match a meminfo entry (case insensitive) after removing non-alpha
+   * characters from the entry. */
+  int i = 0;
+  const char* p = meminfo;
+
+  for (;;) {
+    if (i == nfields) {
+      /* all fields are matched */
+      return true;
+    }
+    if (*p == '\0') {
+      /* end of input reached while scanning */
+      LOG(WARNING) << "cannot find field " << fields[i].match <<

[kmixter1, 2011/04/08 23:04:05]

with LOG statements, the style is to have wrapped lines start with << which
align with the << of the first line.

[Luigi Semenzato, 2011/04/11 16:28:26]

Yes!

+          " and following";
+      return false;
+    }
+    if (strncmp(fields[i].match, p, strlen(fields[i].match)) == 0) {
+      /* name matches: parse value and report */
+      int meminfo_value;
+      char metrics_name[128];
+      char* rest;
+      p += strlen(fields[i].match);

[kmixter1, 2011/04/08 23:04:05]

I would suggest finding a way to replace all this pointer/parsing stuff with
SplitString('\n') and SplitString(' ') or Tokenize from string_util.h.  Less
error prone and easier to maintain.

[Luigi Semenzato, 2011/04/11 16:28:26]

Yes!

On 2011/04/08 23:04:05, kmixter1 wrote:

+      while (!isdigit(*p)) {
+        if (*p == '\0') {
+          LOG(WARNING) << "error parsing meminfo value";
+          return false;
+        }
+        p++;
+      }
+      meminfo_value = (int) strtol(p, &rest, 10);

[kmixter1, 2011/04/08 23:04:05]

static_cast

+      if (p < rest) {
+        p = rest;
+      } else {
+        LOG(WARNING) << "missing meminfo value";
+        return false;
+      }
+      if (i == 0) {
+        /* special case: total memory */
+        total_memory = meminfo_value;
+      } else {
+        snprintf(metrics_name, sizeof(metrics_name),
+                 "Platform.Meminfo%s", fields[i].name);
+        if (fields[i].log_scale) {
+          /* report value in kbytes, log scale, 4Gb max */
+          SendMetric(metrics_name, meminfo_value, 1, 4 * 1000 * 1000, 100);

[kmixter1, 2011/04/08 23:04:05]

This change increases our current number of histograms by a large %.  Note that
you'll need to add all of these to internal chrome source's xml separately.

[kmixter1, 2011/04/08 23:04:05]

So, doesn't this create a histogram with 4 million separately countable buckets.
 Wouldn't this change essentially up the memory usage by 4M*4B*13=208MB
ironically to monitor memory usage? :)

[Luigi Semenzato, 2011/04/11 16:28:26]

Thanks, yes, I have gone through that a couple of times already.

On 2011/04/08 23:04:05, kmixter1 wrote:

[Luigi Semenzato, 2011/04/11 16:28:26]

Yes that would be ironic.

But no, the number of buckets is 100, the last argument.  You may be thinking of
SendLinearMetric.


On 2011/04/08 23:04:05, kmixter1 wrote:

[kmixter1, 2011/04/12 19:04:33]

Apologies - I misread the code.

+        } else {
+          /* report value as percent of total memory */
+          int percent = meminfo_value * 100 / total_memory;

[kmixter1, 2011/04/08 23:04:05]

this will crash if the kernel ever doesn't report meminfo as the first line. 
unlikely, but probably worth a safeguard.

[Luigi Semenzato, 2011/04/11 16:28:26]

Actually, it won't, because it won't get there if it can't parse the total
memory in the first line.  I will make that clearer.

On 2011/04/08 23:04:05, kmixter1 wrote:

+          SendLinearMetric(metrics_name, percent, 100, 101);
+        }
+      }
+      /* start looking for next field */
+      i++;
+    }
+    /* move to next input line */
+    while (*p != '\n') {
+      if (*p == '\0') {
+        LOG(WARNING) << "error parsing meminfo line";
+        return false;
+      }
+      p++;
+    }
+    p++;
+  }
+}
+
 // static
 void MetricsDaemon::ReportDailyUse(void* handle, int tag, int count) {
   if (count <= 0)
@@ -636,3 +781,13 @@ void MetricsDaemon::SendMetric(const string& name, int sample,
              << min << " " << max << " " << nbuckets;
   metrics_lib_->SendToUMA(name, sample, min, max, nbuckets);
 }
+
+void MetricsDaemon::SendLinearMetric(const string& name, int sample,
+                                     int max, int nbuckets) {

[kmixter1, 2011/04/08 23:04:05]

I suggest not passing nbuckets since it's not used and might mislead someone to
think it's only using memory proportional to nbuckets.

[Luigi Semenzato, 2011/04/11 16:28:26]

nbuckets is actually used.  There is currently a constraint (nbuckets == max +
1) but I have a TODO there to remove it.

+  DLOG(INFO) << "received linear metric: " << name << " " << sample << " "
+             << max << " " << nbuckets;
+  /* TODO(semenzato): add a proper linear histogram to the Chrome external
+   * metrics API. */
+  LOG_IF(FATAL, nbuckets != max + 1) << "unsupported histogram scale";
+  metrics_lib_->SendEnumToUMA(name, sample, max);

[kmixter1, 2011/04/08 23:04:05]

Ignoring the memory usage issue, I thought somewhere you had to document what
each value of the "enum" meant for server display.

[Luigi Semenzato, 2011/04/11 16:28:26]

That's because "enum" is commonly used to send enumerated types, but it's really
just a linear histogram.

+}