Closed Bug 948204 Opened 11 years ago Closed 10 years ago

Extend system memory reporter to report pmem

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29

People

(Reporter: jld, Assigned: jld)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 3 obsolete files)

bug948204-pmem-report-hg3.diff
7.49 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review 
The Android "pmem" subsystem reserves ranges of contiguous physical memory at boot for use in communicating with the graphics hardware.  These are not counted in the MemTotal field of /proc/meminfo and are thus omitted from the total in bug 945973's system memory reporter.

We can obtain these sizes from /sys/kernel/pmem_regions/*/size.  Additionally, /sys/kernel/pmem_regions/*/mapped_regions shows us how much is allocated (and where, if we run into fragmentation problems), and theoretically identify the allocating process except that all of ours seem to be accounted to the kernel (pid 0).

It should be helpful to have more visibility into this area.

Note #1: Several vendors designed their own alternatives to pmem, which appear to be being obsoleted in favor of a new interface called ION — http://lwn.net/Articles/480055/ — so supporting pmem isn't the end of this (but that can be a separate bug).

Note #2: We still won't see all the real physical memory — as I understand it, these devices typically allocate some of the memory to the cellular baseband processor, so that's not even visible to the kernel.
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
Attached patch bug948204-pmem-report-hg0.diff (obsolete) — — Splinter Review 
Example output, on a hamachi (256 MB) running CubeVid:

202.65 MB (100.0%) -- mem
…
├───22.95 MB (11.33%) -- pmem
│   ├──17.91 MB (08.84%) -- free
│   │  ├──13.00 MB (06.41%) ── pmem_adsp
│   │  ├───2.96 MB (01.46%) ── pmem
│   │  └───1.95 MB (00.96%) ── pmem_audio
│   └───5.04 MB (02.49%) -- used(pid=0)/pmem
│       ├──2.70 MB (01.33%) ── 0x398000
│       └──2.34 MB (01.16%) ++ (4 tiny)
…
Attachment #8358021 - Flags: review?(n.nethercote)
Comment on attachment 8358021 [details] [diff] [review]
bug948204-pmem-report-hg0.diff

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

Thanks for doing this! Looks good, though we should chat on IRC about the exact structure of the tree.

::: xpcom/base/SystemMemoryReporter.cpp
@@ +153,5 @@
>  
>      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

Nit: Period at end of sentence.

@@ +507,5 @@
>  
> +  nsresult CollectPmemReports(nsIHandleReportCallback* aHandleReport,
> +                              nsISupports* aData)
> +  {
> +    DIR* d = opendir("/sys/kernel/pmem_regions");

A brief comment about the files expected within this directory would be helpful.

@@ +522,5 @@
> +      const char* name = ent->d_name;
> +      uint64_t size;
> +      int scanned;
> +
> +      if (name[0] == '.') {

Do we have to worry about '..' too?

@@ +525,5 @@
> +
> +      if (name[0] == '.') {
> +        continue;
> +      }
> +      // Read total size.

Nit: blank line before comment.

@@ +531,5 @@
> +      FILE* sizeFile = fopen(sizePath.get(), "r");
> +      if (NS_WARN_IF(!sizeFile)) {
> +        continue;
> +      }
> +      scanned = fscanf(sizeFile, "%"SCNu64, &size);

A brief comment here about the expected contents of this file and their meaning (like the comments you have below) would be good. It appears that the contents looks like this:

  36700160(0x2300000)

@@ +560,5 @@
> +            continue;
> +          }
> +          for (const char* nextParen = strchr(buf, '(');
> +               nextParen != nullptr;
> +               nextParen = strchr(nextParen + 1, '(')) {

Just think how much easier this would be in a language like Python or Perl or even JavaScript.

@@ +570,5 @@
> +              break;
> +            }
> +
> +            nsPrintfCString path("mem/pmem/used(pid=%d)/%s/0x%"PRIx64,
> +                                 pid, name, mapStart);

First, it would be better to have a path separator after "used", so that there
is a sub-tree that contains all the used memory, and so you can see a single
total "used" number.

And I wonder if it's better to put |name| in the path before |pid|? That would
better reflect the structure of the data as provided by the kernel, and the
"used" sub-tree would look more like the |free| sub-tree.

Also, is printing a separate entry for each segment under the same pid useful?
Perhaps they could be combined.

If you did all three of those, the example would look like this:

> ├───22.95 MB (11.33%) -- pmem
> │   ├──17.91 MB (08.84%) -- free
> │   │  ├──13.00 MB (06.41%) ── pmem_adsp
> │   │  ├───2.96 MB (01.46%) ── pmem
> │   │  └───1.95 MB (00.96%) ── pmem_audio
> │   └───5.04 MB (02.49%) -- used
>         └──5.04 MB (02.49%) -- pmem
>            └──5.04 MB (02.49%) -- segment(pid=0) [5]

What do you think?

@@ +573,5 @@
> +            nsPrintfCString path("mem/pmem/used(pid=%d)/%s/0x%"PRIx64,
> +                                 pid, name, mapStart);
> +            nsPrintfCString desc("Physical memory reserved for the \"%s\" pool "
> +                                 "and allocated to a buffer.", name);
> +            REPORT(path, mapLen, desc);

If this fails it will leave the fd and dirent open. So you can't use the REPORT macro here, which is annoying.

@@ +584,5 @@
> +      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(path, freeSize, desc);

And this will leave the dirent open.
Attachment #8358021 - Flags: review?(n.nethercote) → review+
Attached patch bug948204-pmem-report-hg1.diff (obsolete) — — Splinter Review 
Quoting from IRC:
> 15:26 < jld> njn: The idea behind including the address is that it might help to see the size distribution of the allocations.  Also, having the offset might help identify the owner, if that could be logged or otherwise extracted from whatever's doing the allocations.

Otherwise, fixed as suggested.  Carrying over r=njn.
Attachment #8358021 - Attachment is obsolete: true
Attachment #8358132 - Flags: review+
Updated example:

├───22.95 MB (11.33%) -- pmem
│   ├──17.91 MB (08.84%) -- free
│   │  ├──13.00 MB (06.41%) ── pmem_adsp
│   │  ├───2.96 MB (01.46%) ── pmem
│   │  └───1.95 MB (00.96%) ── pmem_audio
│   └───5.04 MB (02.49%) -- used/pmem
│       ├──2.70 MB (01.33%) ── segment(pid=0, offset=0x398000)
│       └──2.34 MB (01.16%) ++ (4 tiny)
Comment on attachment 8358132 [details] [diff] [review]
bug948204-pmem-report-hg1.diff

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

Land it!

::: xpcom/base/SystemMemoryReporter.cpp
@@ +129,5 @@
>      }                                                                         \
>    } while (0)
>  
> +#define REPORT(_path, _amount, _desc) \
> +    REPORT_WITH_CLEANUP(_path, _amount, _desc, )

Use |(void)0| instead of an empty arg?

@@ +540,5 @@
> +
> +      // Skip "." and ".." (and any other dotfiles).
> +      if (name[0] == '.') {
> +        continue;
> +      }

Oh, I misread it the first time -- I didn't realize you were just looking at the first char. Still, the comment helps, so thanks for that.
Attachment #8358132 - Flags: review+
Attached patch bug948204-pmem-report-hg2.diff (obsolete) — — Splinter Review 
Updated more.  Carrying over r=njn.
Attachment #8358132 - Attachment is obsolete: true
Attachment #8358155 - Flags: review+
Suggested checkin branch: b2g-inbound.
Keywords: checkin-needed
Sorry; I should have run that past try.  I've added some whitespace that would be insignificant in C or pre-11 C++, and so far so good: https://tbpl.mozilla.org/?tree=Try&rev=a5307e98b212
Attachment #8358155 - Attachment is obsolete: true
Comment on attachment 8358616 [details] [diff] [review]
bug948204-pmem-report-hg3.diff

I'm not sure if this really needs to be re-reviewed, given the trivialness of the incremental change, but I already had to get backed out twice today so maybe I'll err on the side of safety.
Attachment #8358616 - Flags: review?(n.nethercote)
Comment on attachment 8358616 [details] [diff] [review]
bug948204-pmem-report-hg3.diff

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

That whitespace is really needed? Huh.

At this point, try server is a better reviewer than me!
Attachment #8358616 - Flags: review?(n.nethercote) → review+
https://hg.mozilla.org/mozilla-central/rev/4ec03e801a0f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: