Closed Bug 972170 Opened 6 years ago Closed 6 years ago

report system zram statistics in about:memory

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed

People

(Reporter: bkelly, Assigned: erahm)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 files, 4 obsolete files)

On the tarako b2g device we are using the zram swap system.  For evaluating system performance and tuning it would be nice to capture the various zram stats in about:memory reports.

From Ting:

What:		/sys/block/zram<id>/disksize
Description:
		The disksize file is read-write and specifies the disk size
		which represents the limit on the *uncompressed* worth of data
		that can be stored in this disk.

What:		/sys/block/zram<id>/num_reads
Description:
		The num_reads file is read-only and specifies the number of
		reads (failed or successful) done on this device.

What:		/sys/block/zram<id>/num_writes
Description:
		The num_writes file is read-only and specifies the number of
		writes (failed or successful) done on this device.

What:		/sys/block/zram<id>/orig_data_size
Description:
		The orig_data_size file is read-only and specifies uncompressed
		size of data stored in this disk. This excludes zero-filled
		pages (zero_pages) since no memory is allocated for them.
		Unit: bytes

What:		/sys/block/zram<id>/compr_data_size
Description:
		The compr_data_size file is read-only and specifies compressed
		size of data stored in this disk. So, compression ratio can be
		calculated using orig_data_size and this statistic.
		Unit: bytes
xpcom/base/SystemMemoryReporter.cpp is the file to edit.

erahm, this would be a great bug for you.
Component: about:memory → XPCOM
Product: Toolkit → Core
Assignee: nobody → erahm
Looking into it now.
Comment on attachment 8376006 [details] [diff] [review]
Add zram reporting to about:memory

This is a proposed solution for reporting zram statistics in the system entry of about:memory. Guidance on how to structure the values better would be greatly appreciated.

Note: this is a patch against mozilla-central, a second patch will be added for 1.3T
Attachment #8376006 - Flags: feedback?(n.nethercote)
erahm, can you post example output? That helps with reviewing patches for new reporters. Thanks.
Flags: needinfo?(erahm)
76,632,382 (100.0%) -- 
└──76,632,382 (100.0%) -- sys/block/zram0
   ├──67,108,864 (87.57%) ── disksize
   ├───6,885,376 (08.98%) ── orig_data_size
   ├───2,491,914 (03.25%) ── compr_data_size
   └─────146,228 (00.19%) -- (2 tiny)
         ├───79,596 (00.10%) ── num_writes
         └───66,632 (00.09%) ── num_reads
Flags: needinfo?(erahm)
These numbers are not in the same dimension. Perhaps they should not be accumulated into sys/block/zram0?
Agreed none of them make sense lumped together, well except that they're all zram.

bytes:
- disksize
- orig_data_size
- compr_data_size

cumulative counts:
- num_writes
- num_reads
Patch for 1.3T branch
Attachment #8376006 - Attachment is obsolete: true
Attachment #8376006 - Flags: feedback?(n.nethercote)
Comment on attachment 8376006 [details] [diff] [review]
Add zram reporting to about:memory

># HG changeset patch
># User Eric Rahm <erahm@mozilla.com>
># Date 1392348539 -28800
>#      Fri Feb 14 11:28:59 2014 +0800
># Node ID cb7513f19a2228bd77559bc655a70f4e5b16abc2
># Parent  d275eebfae041f3c495890e9a9e215494576bc72
>Bug 972170 - Add zram reporting to about:memory
>
>diff --git a/xpcom/base/SystemMemoryReporter.cpp b/xpcom/base/SystemMemoryReporter.cpp
>--- a/xpcom/base/SystemMemoryReporter.cpp
>+++ b/xpcom/base/SystemMemoryReporter.cpp
>@@ -109,33 +109,33 @@ IsAnonymous(const nsACString& aName)
>          StringBeginsWith(aName, NS_LITERAL_CSTRING("[stack:"));
> }
> 
> class SystemReporter MOZ_FINAL : public nsIMemoryReporter
> {
> public:
>   NS_DECL_THREADSAFE_ISUPPORTS
> 
>-#define REPORT_WITH_CLEANUP(_path, _amount, _desc, _cleanup)                  \
>+#define REPORT_WITH_CLEANUP(_path, _amount, _desc, _cleanup, _type)           \
>   do {                                                                        \
>     size_t amount = _amount;  /* evaluate _amount only once */                \
>     if (amount > 0) {                                                         \
>       nsresult rv;                                                            \
>       rv = aHandleReport->Callback(NS_LITERAL_CSTRING("System"), _path,       \
>-                                   KIND_NONHEAP, UNITS_BYTES, amount, _desc,  \
>+                                   KIND_NONHEAP, _type, amount, _desc,        \
>                                    aData);                                    \
>       if (NS_WARN_IF(NS_FAILED(rv))) {                                        \
>         _cleanup;                                                             \
>         return rv;                                                            \
>       }                                                                       \
>     }                                                                         \
>   } while (0)
> 
> #define REPORT(_path, _amount, _desc) \
>-    REPORT_WITH_CLEANUP(_path, _amount, _desc, (void)0)
>+    REPORT_WITH_CLEANUP(_path, _amount, _desc, (void)0, UNITS_BYTES)
> 
>   NS_IMETHOD CollectReports(nsIHandleReportCallback* aHandleReport,
>                             nsISupports* aData)
>   {
>     if (!Preferences::GetBool("memory.system_memory_reporter")) {
>       return NS_OK;
>     }
> 
>@@ -156,16 +156,19 @@ public:
> "reclaimed by the OS at any time."));
> 
>     REPORT(NS_LITERAL_CSTRING("mem/free"), memFree, NS_LITERAL_CSTRING(
> "Memory which is free and not being used for any purpose."));
> 
>     // Report reserved memory not included in memTotal.
>     rv = CollectPmemReports(aHandleReport, aData);
> 
>+    // Report zram usage statistics.
>+    rv = CollectZramReports(aHandleReport, aData);
>+
>     return rv;
>   }
> 
> private:
>   // Keep this in sync with SystemReporter::kindPathSuffixes!
>   enum ProcessSizeKind {
>     AnonymousOutsideBrk  = 0,
>     AnonymousBrkHeap     = 1,
>@@ -588,33 +591,128 @@ private:
>               break;
>             }
> 
>             nsPrintfCString path("mem/pmem/used/%s/segment(pid=%d, "
>                                  "offset=0x%" PRIx64 ")", name, pid, mapStart);
>             nsPrintfCString desc("Physical memory reserved for the \"%s\" pool "
>                                  "and allocated to a buffer.", name);
>             REPORT_WITH_CLEANUP(path, mapLen, desc,
>-                                (fclose(regionsFile), closedir(d)));
>+                                (fclose(regionsFile), closedir(d)),
>+                                UNITS_BYTES);
>             freeSize -= mapLen;
>           }
>         }
>         fclose(regionsFile);
>       }
> 
>       nsPrintfCString path("mem/pmem/free/%s", name);
>       nsPrintfCString desc("Physical memory reserved for the \"%s\" pool and "
>                            "unavailable to the rest of the system, but not "
>                            "currently allocated.", name);
>-      REPORT_WITH_CLEANUP(path, freeSize, desc, closedir(d));
>+      REPORT_WITH_CLEANUP(path, freeSize, desc, closedir(d), UNITS_BYTES);
>     }
>     closedir(d);
>     return NS_OK;
>   }
> 
>+  uint64_t ReadSizeFromFile(const char* filename)
>+  {
>+      FILE* sizeFile = fopen(filename, "r");
>+      if (NS_WARN_IF(!sizeFile)) {
>+        return 0;
>+      }
>+
>+      uint64_t size = 0;
>+      fscanf(sizeFile, "%" SCNu64, &size);
>+      fclose(sizeFile);
>+
>+      return size;
>+  }
>+
>+  nsresult CollectZramReports(nsIHandleReportCallback* aHandleReport,
>+                              nsISupports* aData)
>+  {
>+    DIR* d = opendir("/sys/block");
>+    if (!d) {
>+      if (NS_WARN_IF(errno != ENOENT)) {
>+        return NS_ERROR_FAILURE;
>+      }
>+
>+      return NS_OK;
>+    }
>+
>+    struct dirent* ent;
>+    while ((ent = readdir(d))) {
>+      const char* name = ent->d_name;
>+
>+      // Skip non-zram entries.
>+      if (strncmp("zram", name, 4) != 0) {
>+        continue;
>+      }
>+
>+      // NB: Just using UNITS_COUNT here because the zram numbers are mixed
>+      //     which isn't allowed in about:memory and none of them particularly
>+      //     make sense cumulatively.
>+
>+      // /sys/block/zram<id>/disksize (UNITS_BYTES)
>+      nsPrintfCString diskSizeFile("/sys/block/%s/disksize", name);
>+      nsPrintfCString diskSizeFileDesc(
>+                           "The disksize file specifies the limit on the "
>+                           "amount of uncompressed data that can be stored in "
>+                           "\"%s\"", name);
>+      uint64_t diskSize = ReadSizeFromFile(diskSizeFile.get());
>+      REPORT_WITH_CLEANUP(diskSizeFile, diskSize,
>+                          diskSizeFileDesc, closedir(d), UNITS_COUNT);
>+
>+      // /sys/block/zram<id>/num_reads (UNITS_COUNT_CUMULATIVE)
>+      nsPrintfCString readsFile("/sys/block/%s/num_reads", name);
>+      nsPrintfCString readsFileDesc(
>+                           "The num_reads file specifies the number of "
>+                           "reads (failed or successful) done on "
>+                           "\"%s\"", name);
>+      uint64_t reads = ReadSizeFromFile(readsFile.get());
>+      REPORT_WITH_CLEANUP(readsFile, reads,
>+                          readsFileDesc, closedir(d), UNITS_COUNT);
>+
>+      // /sys/block/zram<id>/num_writes (UNITS_COUNT_CUMULATIVE)
>+      nsPrintfCString writesFile("/sys/block/%s/num_writes", name);
>+      nsPrintfCString writesFileDesc(
>+                           "The num_writes file specifies the number of "
>+                           "writes (failed or successful) done on "
>+                           "\"%s\"", name);
>+      uint64_t writes = ReadSizeFromFile(writesFile.get());
>+      REPORT_WITH_CLEANUP(writesFile, writes,
>+                          writesFileDesc, closedir(d), UNITS_COUNT);
>+
>+      // /sys/block/zram<id>/orig_data_size (UNITS_BYTES)
>+      nsPrintfCString origSizeFile("/sys/block/%s/orig_data_size", name);
>+      nsPrintfCString origSizeFileDesc(
>+                           "The orig_data_size file specifies the uncompressed "
>+                           "size of data stored in \"%s.\" This excludes "
>+                           "zero-filled pages (zero_pages) since no memory is "
>+                           "allocated for them.", name);
>+      uint64_t origSize = ReadSizeFromFile(origSizeFile.get());
>+      REPORT_WITH_CLEANUP(origSizeFile, origSize,
>+                          origSizeFileDesc, closedir(d), UNITS_COUNT);
>+
>+      // /sys/block/zram<id>/compr_data_size (UNITS_BYTES)
>+      nsPrintfCString comprSizeFile("/sys/block/%s/compr_data_size", name);
>+      nsPrintfCString comprSizeFileDesc(
>+                           "The compr_data_size file specifies the compressed "
>+                           "size of data stored in \"%s\"",  name);
>+      uint64_t comprSize = ReadSizeFromFile(comprSizeFile.get());
>+      REPORT_WITH_CLEANUP(comprSizeFile, comprSize,
>+                          comprSizeFileDesc, closedir(d), UNITS_COUNT);
>+    }
>+
>+    closedir(d);
>+    return NS_OK;
>+  }
>+
> #undef REPORT
> };
> 
> NS_IMPL_ISUPPORTS1(SystemReporter, nsIMemoryReporter)
> 
> // Keep this in sync with SystemReporter::ProcessSizeKind!
> const char* SystemReporter::kindPathSuffixes[] = {
>     "anonymous/outside-brk",
Attachment #8376006 - Attachment is obsolete: false
Comment on attachment 8376006 [details] [diff] [review]
Add zram reporting to about:memory

Review of attachment 8376006 [details] [diff] [review]:
-----------------------------------------------------------------

Cool. A few more tweaks and we'll be there.

::: xpcom/base/SystemMemoryReporter.cpp
@@ +113,5 @@
>  {
>  public:
>    NS_DECL_THREADSAFE_ISUPPORTS
>  
> +#define REPORT_WITH_CLEANUP(_path, _amount, _desc, _cleanup, _type)           \

_type should be called _units, and it should go before _amount. (The order for memory reports is always: process, path, kind, units, amount, desc; and we can skip some as necessary.)

@@ +613,5 @@
>      closedir(d);
>      return NS_OK;
>    }
>  
> +  uint64_t ReadSizeFromFile(const char* filename)

s/filename/aFilename/

@@ +630,5 @@
> +
> +  nsresult CollectZramReports(nsIHandleReportCallback* aHandleReport,
> +                              nsISupports* aData)
> +  {
> +    DIR* d = opendir("/sys/block");

A comment here about the structure and content of the files would be great. Something like in comment 0, though not as detailed. See CollectPmemReports() for an example.

@@ +650,5 @@
> +      }
> +
> +      // NB: Just using UNITS_COUNT here because the zram numbers are mixed
> +      //     which isn't allowed in about:memory and none of them particularly
> +      //     make sense cumulatively.

So you'll probably want three distinct trees:

zram-accesses  # COUNTS
- zram0
  - reads
  - writes

zram-disksize   # BYTES
- zram0
  - used      # == compr_data_size
  - unused    # == disksize - compr_data_size

zram-orig-data-size  # BYTES
- zram0
(In reply to Eric Rahm [:erahm] from comment #9)
> Created attachment 8376064 [details] [diff] [review]
> Add zram reporting to about:memory for 1.3T
> 
> Patch for 1.3T branch

Does Tarako use pmem? Even if it doesn't, it might just be simpler to backport the patch from bug 948204 so you don't need a different patch.
So nominate bug 948204 for 1.3T, then mark this as depending on it?
> So nominate bug 948204 for 1.3T, then mark this as depending on it?

If you want to go that route, then yes. If Tarako has pmem, we should definitely do that. Otherwise, I'll leave it up to you.
Whiteboard: [MemShrink] → [MemShrink:P2]
Patch for mozilla-central
Attachment #8376006 - Attachment is obsolete: true
Attachment #8376064 - Attachment is obsolete: true
Patch for 1.3T
Attachment #8377952 - Attachment is obsolete: true
Attachment #8377952 - Attachment is obsolete: false
New format:


83,878 (100.0%) -- zram-accesses
└──83,878 (100.0%) -- zram0
   ├──51,528 (61.43%) ── writes
   └──32,350 (38.57%) ── reads

16.44 MB (100.0%) -- zram-compr-data-size
└──16.44 MB (100.0%) ── zram0

64.00 MB (100.0%) -- zram-disksize
└──64.00 MB (100.0%) -- zram0
   ├──45.69 MB (71.39%) ── used
   └──18.31 MB (28.61%) ── unused
Comment on attachment 8377952 [details] [diff] [review]
Patch 1 - Add zram reporting to about:memory

This patch is for landing on mozilla-central, I regrouped the entries so that logical units were together as recommended.
Attachment #8377952 - Flags: review?(n.nethercote)
Comment on attachment 8377954 [details] [diff] [review]
Patch 2 - Add zram reporting to about:memory for 1.3T

This patch is for 1.3T, I decided not to include the pmem changes as tarako does not have pmem info available (and a smaller patch is probably better).
Attachment #8377954 - Flags: review?(n.nethercote)
Comment on attachment 8377952 [details] [diff] [review]
Patch 1 - Add zram reporting to about:memory

Review of attachment 8377952 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks! Please upload a final version with the nits addressed and mark it with the "checkin-needed" flag, and then one of the sheriffs will land it on mozilla-inbound for you.

::: xpcom/base/SystemMemoryReporter.cpp
@@ +612,5 @@
>      closedir(d);
>      return NS_OK;
>    }
>  
> +  uint64_t ReadSizeFromFile(const char* aFilename)

Mozilla style is to put the function name on a new line, viz:

  uint64_t
  ReadSizeFromFile(...)

@@ +626,5 @@
> +
> +      return size;
> +  }
> +
> +  nsresult CollectZramReports(nsIHandleReportCallback* aHandleReport,

Ditto.

@@ +631,5 @@
> +                              nsISupports* aData)
> +  {
> +    // zram usage stats files can be found under:
> +    //  /sys/block/zram<id>
> +    //  |--> disksize        - Uncompressed size of the disk (bytes)

I found this description confusing. Maybe something like "Maximum amount of uncompressed data that can be stored on the disk"?

@@ +669,5 @@
> +      nsPrintfCString diskUsedPath("zram-disksize/%s/used", name);
> +      nsPrintfCString diskUsedDesc(
> +                           "The uncompressed size of data stored in \"%s.\" "
> +                           "This excludes zero-filled pages (zero_pages) since "
> +                           "no memory is allocated for them.", name);

I'd omit the "(zero_pages)"; I don't think it adds much.
Attachment #8377952 - Flags: review?(n.nethercote) → review+
Comment on attachment 8377952 [details] [diff] [review]
Patch 1 - Add zram reporting to about:memory

Review of attachment 8377952 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/SystemMemoryReporter.cpp
@@ +167,1 @@
>      return rv;

Oh wait, the rv handling isn't quite right. Every function that returns an nsresult should be checked. So do this:

  // Report reserved memory not included in memTotal.
  rv = CollectPmemReports(aHandleReport, aData);
  NS_ENSURE_SUCCESS(rv, rv);

  // Report zram usage statistics.
  rv = CollectZramReports(aHandleReport, aData);
  NS_ENSURE_SUCCESS(rv, rv);

  return NS_OK;
Comment on attachment 8377954 [details] [diff] [review]
Patch 2 - Add zram reporting to about:memory for 1.3T

Review of attachment 8377954 [details] [diff] [review]:
-----------------------------------------------------------------

The nits from the other patch need to be addressed here too.

At this point, you probably have a better idea of the 1.3T landing protocol than I do... I guess ask someone (fabrice?) to land this for you once the final version is ready.

::: xpcom/base/SystemMemoryReporter.cpp
@@ +169,5 @@
>      REPORT(NS_LITERAL_CSTRING("mem/free"), memFree, NS_LITERAL_CSTRING(
>  "Memory which is free and not being used for any purpose."));
>  
> +    // Report zram usage statistics
> +    rv = CollectZramReports(aHandleReport, aData);

Add |NS_ENSURE_SUCCESS(rv, rv);| after this line.
Attachment #8377954 - Flags: review?(n.nethercote) → review+
triage: 1.3T+ to help with debugging memory issues on tarako
blocking-b2g: 1.3T? → 1.3T+
Cleaned up comments and style per review, carried over r+ from njn.
Attachment #8377952 - Attachment is obsolete: true
Attachment #8378463 - Flags: review+
Cleaned up comments and style per review. Carrying over r+ from njn.
Attachment #8377954 - Attachment is obsolete: true
Attachment #8378465 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e05c4e27c8c5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.