210345 2020-04-10 10:26:25 -0700 [GTK][WPE] lowWatermarkPages() in MemoryPressureMonitor.cpp only searches the "low" value inside the first "Node" section 2020-04-23 12:30:08 -0700 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build Unspecified Linux RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=210916 P2 Normal --- 209942 210346 1 psaavedra psaavedra annulen aperez bugs-noreply clopez darin mcrha oldest_to_newest 1640248 0 psaavedra 2020-04-10 10:26:25 -0700 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? 1640660 1 psaavedra 2020-04-12 02:52:13 -0700 "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. 1643361 2 psaavedra 2020-04-19 23:47:09 -0700 (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. 1643379 3 396954 psaavedra 2020-04-20 00:13:32 -0700 Created attachment 396954 patch 1643412 4 ews-feeder 2020-04-20 03:22:57 -0700 Committed r260359: <https://trac.webkit.org/changeset/260359> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396954. 1644751 5 mcrha 2020-04-23 06:36:53 -0700 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). 1644752 6 mcrha 2020-04-23 06:40:41 -0700 s/up to the top/until the/ 1644776 7 psaavedra 2020-04-23 08:33:27 -0700 (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. 1644781 8 mcrha 2020-04-23 08:42:37 -0700 You are welcome. I just faced "out of file descriptors" abort here, otherwise I'd probably not notice at all. 1644785 9 psaavedra 2020-04-23 08:58:13 -0700 New bug created to link with the new patch https://bugs.webkit.org/show_bug.cgi?id=210916 1644790 10 mcrha 2020-04-23 09:10:25 -0700 Thanks. I guess you can close this one. 1644910 11 aperez 2020-04-23 12:30:08 -0700 (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 =) 396954 2020-04-20 00:13:32 -0700 2020-04-20 03:22:58 -0700 patch bug-210345-20200420091304.patch text/plain 3825 psaavedra U3VidmVyc2lvbiBSZXZpc2lvbjogMjYwMzUzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGU5ZDdiOTZlOGU2OWYzMjg5 YzViOGNhMzg1NTkxY2UyOWM0NmYyNjIuLjQxNWE1ODBhMDA5OTFlNDMwZjVkZmI2OTk3ZGM1ZTE5 NThkMzc0YjIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMjAtMDQtMjAgIFBhYmxvIFNh YXZlZHJhICA8cHNhYXZlZHJhQGlnYWxpYS5jb20+CisKKyAgICAgICAgW0dUS11bV1BFXSBsb3dX YXRlcm1hcmtQYWdlcygpIGluIE1lbW9yeVByZXNzdXJlTW9uaXRvci5jcHAgb25seSBzZWFyY2hl cyB0aGUgImxvdyIgdmFsdWUgaW5zaWRlIHRoZSBmaXJzdCAiTm9kZSIgc2VjdGlvbgorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjEwMzQ1CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvbGlu dXgvTWVtb3J5UHJlc3N1cmVNb25pdG9yLmNwcDoKKyAgICAgICAgKFdlYktpdDo6bG93V2F0ZXJt YXJrUGFnZXMpOgorCiAyMDIwLTA0LTE5ICBCcmFkeSBFaWRzb24gIDxiZWlkc29uQGFwcGxlLmNv bT4KIAogICAgICAgICBBZGQgV0tTY3JpcHRNZXNzYWdlSGFuZGxlciBBUEkgdGhhdCBhc3luY2hy b25vdXNseSByZXNwb25kcyB3aXRoIGEgcHJvbWlzZS4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJL aXQvVUlQcm9jZXNzL2xpbnV4L01lbW9yeVByZXNzdXJlTW9uaXRvci5jcHAgYi9Tb3VyY2UvV2Vi S2l0L1VJUHJvY2Vzcy9saW51eC9NZW1vcnlQcmVzc3VyZU1vbml0b3IuY3BwCmluZGV4IGVlNDM5 MmMwOGVlOTFjM2ExZDM1MTIyMzIwMTAzMzEzNzhhMzE3NDEuLjQ4YTRjYjI4ZmQwMTE0NDY2YmE4 NGQ2MWI0OTQxYzk3MjdiZTc4NmEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNz L2xpbnV4L01lbW9yeVByZXNzdXJlTW9uaXRvci5jcHAKKysrIGIvU291cmNlL1dlYktpdC9VSVBy b2Nlc3MvbGludXgvTWVtb3J5UHJlc3N1cmVNb25pdG9yLmNwcApAQCAtNjYsOSArNjYsNDggQEAg c3RhdGljIGNvbnN0IHVuc2lnbmVkIG1heENncm91cFBhdGggPSA0MDk2OyAvLyBQQVRIX01BWCA9 IDQwOTYgZnJvbSAoTGludXgpIGluY2wKICNkZWZpbmUgU1RSSU5HSUZZKHZhbCkgU1RSSU5HSUZZ X0VYUEFOREVEKHZhbCkKICNkZWZpbmUgWk9ORUlORk9fVE9LRU5fQlVGRkVSX1NJWkUgMTI4CiAK Ky8vIFRoZSBsb3dXYXRlcm1hcmsgaXMgdGhlIHN1bSBvZiB0aGUgbG93IHdhdGVybWFya3MgYWNy b3NzIGFsbCB6b25lcyBhcyB0aGUKKy8vIE1lbUF2YWlsYWJsZSBpbmZvIHdhcyBpbXBsZW1lbnRl ZCBpbiAvcHJvYy9tZW1pbmZvIHNpbmNlIHZlcnNpb24gMy4xNCBvZiB0aGUKKy8vIGtlcm5lbCAo YWRkZWQgYnkgY29tbWl0IDM0ZTQzMWIwYSwgc291cmNlIGdpdC5rZXJuZWwub3JnKToKKy8vCisv LyBNZW1BdmFpbGFibGU6IEFuIGVzdGltYXRlIG9mIGhvdyBtdWNoIG1lbW9yeSBpcyBhdmFpbGFi bGUgZm9yIHN0YXJ0aW5nIG5ldworLy8gICAgICAgICAgICAgICBhcHBsaWNhdGlvbnMsIHdpdGhv dXQgc3dhcHBpbmcuIENhbGN1bGF0ZWQgZnJvbSBNZW1GcmVlLAorLy8gICAgICAgICAgICAgICBT UmVjbGFpbWFibGUsIHRoZSBzaXplIG9mIHRoZSBmaWxlIExSVSBsaXN0cywgYW5kIHRoZSBsb3cK Ky8vICAgICAgICAgICAgICAgd2F0ZXJtYXJrcyBpbiBlYWNoIHpvbmUuCisvLyAgICAgICAgICAg ICAgIFRoZSBlc3RpbWF0ZSB0YWtlcyBpbnRvIGFjY291bnQgdGhhdCB0aGUgc3lzdGVtIG5lZWRz IHNvbWUKKy8vICAgICAgICAgICAgICAgcGFnZSBjYWNoZSB0byBmdW5jdGlvbiB3ZWxsLCBhbmQg dGhhdCBub3QgYWxsIHJlY2xhaW1hYmxlCisvLyAgICAgICAgICAgICAgIHNsYWIgd2lsbCBiZSBy ZWNsYWltYWJsZSwgZHVlIHRvIGl0ZW1zIGJlaW5nIGluIHVzZS4gVGhlCisvLyAgICAgICAgICAg ICAgIGltcGFjdCBvZiB0aG9zZSBmYWN0b3JzIHdpbGwgdmFyeSBmcm9tIHN5c3RlbSB0byBzeXN0 ZW0uCisvLworLy8gVGhlIGZzY2FuZigpIHJlYWRzIHRoZSBpbnB1dCBzdHJlYW0gZmlsZSB1bnRp bCB0aGUgYXJndW1lbnQgbGlzdCBwYXNzZWQgYXMKKy8vIHBhcmFtZXRlcnMgaXMgc3VjY2Vzc2Z1 bGx5IGZpbGxlZC4KKy8vCisvLyBJbiBvdXIgaW1tcGxlbWV0YXRpb24gdGhlIGB3aGlsZSAoIWZl b2Yoem9uZUluZm9GaWxlKSlgIGxvb3AgZm9sbG93cyB0aGUgbmV4dAorLy8gbG9naWM6CisvLwor Ly8gLSB0aGUgZmlyc3QgYGZzY2FuZih6b25lSW5mb0ZpbGUsICIgTm9kZSAlKnUsIHpvbmUgJS4u LlteXG5dXG4iLCBidWZmZXIpO2AKKy8vICAgaXRlcmF0ZXMgdGhlIGBOb2RlYCBzZWN0aW9ucy4K Ky8vIC0gVGhlbiwgd2hlbiB3ZSBmb3VuZCBhIE5vcm1hbCBub2RlLCB3ZSBzdGFydCB0byByZWFk IGVhY2ggc2luZ2xlCisvLyAgIGBmc2NhbmYoem9uZUluZm9GaWxlLCAiJS4uLnMiLCBidWZmZXIp O2AgdW50aWwgZmluZCBhIGBsb3dgIHRva2VuLgorLy8gLSBXZSByZWFkIHRoZSBuZXh0IHRva2Vu IHdoaWNoIGlzIHRoZSBhY3R1YWwgYGxvd2AgdmFsdWUgYW5kIHdlIGFkZCBpdCB0byB0aGUKKy8v ICAgYHN1bUxvd2Agc3VtbWF0aW9uLgorLy8KKy8vIFRoZSBzZWNvbmQgZnNjYW5mKCkgcmVhZHMg dG9rZW5zIG9uZSBieSBvbmUgYmVjYXVzZSB0aGUgZm9ybWF0IG9mIGVhY2ggcm93IGlzCisvLyBu b3QgaG9tb2dlbmVvdXMgKDIsIDMgb3IgNiB2YWx1ZXMpOgorLy8KKy8vICAgTm9kZSAwLCB6b25l ICAgTm9ybWFsCisvLyAgICAgcGFnZXMgZnJlZSAgICAgMjczMDMKKy8vICAgICAgICAgICBtaW4g ICAgICAyMDUwMAorLy8gICAgICAgICAgIGxvdyAgICAgIDI0MDg5CisvLyAgICAgICAgICAgaGln aCAgICAgMjc2NzgKKy8vICAgICAgICAgICBzcGFubmVkICAzNDE0MDE2CisvLyAgICAgICAgICAg cHJlc2VudCAgMzQxNDAxNgorLy8gICAgICAgICAgIG1hbmFnZWQgIDMzMzcyOTMKKy8vICAgICAg ICAgICBwcm90ZWN0aW9uOiAoMCwgMCwgMCwgMCwgMCkKIHN0YXRpYyBzaXplX3QgbG93V2F0ZXJt YXJrUGFnZXMoRklMRSogem9uZUluZm9GaWxlKQogewogICAgIHNpemVfdCBsb3cgPSAwOworICAg IHNpemVfdCBzdW1Mb3cgPSAwOwogICAgIGNoYXIgYnVmZmVyW1pPTkVJTkZPX1RPS0VOX0JVRkZF Ul9TSVpFICsgMV07CiAgICAgYm9vbCBpbk5vcm1hbFpvbmUgPSBmYWxzZTsKIApAQCAtODMsMTEg KzEyMiwxMyBAQCBzdGF0aWMgc2l6ZV90IGxvd1dhdGVybWFya1BhZ2VzKEZJTEUqIHpvbmVJbmZv RmlsZSkKICAgICAgICAgciA9IGZzY2FuZih6b25lSW5mb0ZpbGUsICIlIiBTVFJJTkdJRlkoWk9O RUlORk9fVE9LRU5fQlVGRkVSX1NJWkUpICJzIiwgYnVmZmVyKTsKICAgICAgICAgaWYgKHIgPT0g MSAmJiBpbk5vcm1hbFpvbmUgJiYgIXN0cmNtcChidWZmZXIsICJsb3ciKSkgewogICAgICAgICAg ICAgciA9IGZzY2FuZih6b25lSW5mb0ZpbGUsICIlenUiLCAmbG93KTsKLSAgICAgICAgICAgIGlm IChyID09IDEpCi0gICAgICAgICAgICAgICAgYnJlYWs7CisgICAgICAgICAgICBpZiAociA9PSAx KSB7CisgICAgICAgICAgICAgICAgc3VtTG93ICs9IGxvdzsKKyAgICAgICAgICAgICAgICBjb250 aW51ZTsKKyAgICAgICAgICAgIH0KICAgICAgICAgfQogICAgIH0KLSAgICByZXR1cm4gbG93Owor ICAgIHJldHVybiBzdW1Mb3c7CiB9CiAKIHN0YXRpYyBpbmxpbmUgc2l6ZV90IHN5c3RlbVBhZ2VT aXplKCkK