Bug 210345 – [GTK][WPE] lowWatermarkPages() in MemoryPressureMonitor.cpp only searches the "low" value inside the first "Node" section

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Pablo Saavedra]

Related to https://bugs.webkit.org/show_bug.cgi?id=209942
See details in https://bugs.webkit.org/show_bug.cgi?id=209942#c18


The lowWatermarkPages() in Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp is only searching for the “low” value inside the first “Node” section, without taking into account the kind of the memory zone.

Shouldn't we be searching for the “Normal” zone node, instead of the first one seen? In my laptop here the first zone is a DMA memory area:

  Node 0, zone      DMA
    per-node stats
        nr_inactive_anon 277959
        nr_active_anon 863174
        ...
    pages free      3951
          min       13 
          low       16
          high      19
          ...

… and picking “16” as the “low” value seems wrong. We will want to fix this to find the node which describes the “Normal” zone:

  Node 0, zone     Normal
    pages free     252746
          min      14907
          low      26572
          high     31069
          ...

Related question: What if there are more multiple nodes which describe nodes of “Normal” type? Should the “low” values be added together in that case?

[Pablo Saavedra]

"I believe that the special zones (DMA, DMA32, and on 32-bit machines, Normal) will only be present on one node, generally node 0. All other nodes will generally have only Normal (on 64-bit kernels) or HighMem (on 32-bit kernels) memory."

from: https://utcc.utoronto.ca/~cks/space/blog/linux/KernelMemoryZones


Also, this function is only called from calculateMemoryAvailable()


```
// If MemAvailable was not present in /proc/meminfo, because it's an old kernel version,        
// we can do the same calculation with the information we have from meminfo and the low watermaks.
// See https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34e431b0ae398fc54ea69ff85ec700722c9da773                                                                       
static size_t calculateMemoryAvailable(size_t memoryFree, size_t activeFile, size_t inactiveFile, size_t slabReclaimable, FILE* zoneInfoFile)
{   
    if (memoryFree == notSet || activeFile == notSet || inactiveFile == notSet || slabReclaimable == notSet)
        return notSet;                                                                          
                                                                                                
    size_t lowWatermark = lowWatermarkPages(zoneInfoFile);                                      
    if (lowWatermark == notSet)
        return notSet;           
```


MemAvailable is included in /proc/meminfo since version 3.14 of the kernel; it was added by commit 34e431b0a (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=34e431b0a)


```
+MemAvailable: An estimate of how much memory is available for starting new
+              applications, without swapping. Calculated from MemFree,
+              SReclaimable, the size of the file LRU lists, and the low
+              watermarks in each zone.
+              The estimate takes into account that the system needs some
+              page cache to function well, and that not all reclaimable
+              slab will be reclaimable, due to items being in use. The
+              impact of those factors will vary from system to system.
``

Therefore, the lowWatermark is the sum of the low watermarks across all zones.

However, the Linux kernel maintainers decided the End of Life for 3.14 LTS on September 11, 2016. Maybe is time time to delete this code.

[Pablo Saavedra]

(In reply to Pablo Saavedra from comment #1)

> 
> MemAvailable is included in /proc/meminfo since version 3.14 of the kernel;
> it was added by commit 34e431b0a
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/
> ?id=34e431b0a)
> 
> 
> ```
> +MemAvailable: An estimate of how much memory is available for starting new
> +              applications, without swapping. Calculated from MemFree,
> +              SReclaimable, the size of the file LRU lists, and the low
> +              watermarks in each zone.
> +              The estimate takes into account that the system needs some
> +              page cache to function well, and that not all reclaimable
> +              slab will be reclaimable, due to items being in use. The
> +              impact of those factors will vary from system to system.
> ``
> 
> Therefore, the lowWatermark is the sum of the low watermarks across all
> zones.
> 
> However, the Linux kernel maintainers decided the End of Life for 3.14 LTS
> on September 11, 2016. Maybe is time time to delete this code.

Based on https://bugs.webkit.org/show_bug.cgi?id=210346#c3:

```
We would like to decide whether we want to keep supporting kernels
older than 3.14 — but I think it's better to play safe and keep the
code for now.

If (or when) we decide to remove it, we can use a new bug for that.
```

... let's keep this as it is and just fix the code to have in account all the eventual Nodes.

[Pablo Saavedra]

Created attachment 396954 [details]
patch

[EWS]

Committed r260359: <https://trac.webkit.org/changeset/260359>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396954 [details].

[Milan Crha]

This change is leaking file handles. I see constant additions of the triple:

  /proc/meminfo
  /proc/zoneinfo
  /proc/18738/cgroup

in `lsof -p PID`. It lasts up to the top opened files limit is reached.

You should, in MemoryPressureMonitor::start(), fclose() those which had been opened, both in the 'continue' and in the end of the while() (not only after it, which handles the 'break' path).

[Milan Crha]

s/up to the top/until the/

[Pablo Saavedra]

(In reply to Milan Crha from comment #5)
> This change is leaking file handles. I see constant additions of the triple:
> 
>   /proc/meminfo
>   /proc/zoneinfo
>   /proc/18738/cgroup
> 
> in `lsof -p PID`. It lasts up to the top opened files limit is reached.
> 
> You should, in MemoryPressureMonitor::start(), fclose() those which had been
> opened, both in the 'continue' and in the end of the while() (not only after
> it, which handles the 'break' path).


Fixup in a couple of minutes. Thanks for take a look it.

[Milan Crha]

You are welcome. I just faced "out of file descriptors" abort here, otherwise I'd probably not notice at all.

[Pablo Saavedra]

New bug created to link with the new patch https://bugs.webkit.org/show_bug.cgi?id=210916

[Milan Crha]

Thanks. I guess you can close this one.

[Adrian Perez]

(In reply to Milan Crha from comment #10)
> Thanks. I guess you can close this one.

Yes, we'll handle the fix in bug #210916 — and thanks a lot to you
for noticing and reporting the issue =)