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?
"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.
(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.
Created attachment 396954 [details] patch
Committed r260359: <https://trac.webkit.org/changeset/260359> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396954 [details].
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).
s/up to the top/until the/
(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.
You are welcome. I just faced "out of file descriptors" abort here, otherwise I'd probably not notice at all.
New bug created to link with the new patch https://bugs.webkit.org/show_bug.cgi?id=210916
Thanks. I guess you can close this one.
(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 =)