The eventFD file descriptor is pollable, so it would be much better to use a poll instead of a blocking read in a secondary thread and then communicate back to the main thread. This is very easy to do with GSource in GLib, so we could use that when GLib is available and keep the current implementation as a fallback.
Created attachment 282540 [details] Patch
Attachment 282540 [details] did not pass style-queue: ERROR: Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:96: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:240: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/MemoryPressureHandler.h:146: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/MemoryPressureHandler.h:153: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 282540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282540&action=review Looking good. One overall suggestion to use Optional rather than -1/0. > Source/WebCore/platform/MemoryPressureHandler.h:176 > + int m_eventFD { -1 }; > + int m_pressureLevelFD { -1 }; since we are touch these now, can we make it Optional<int> instead of using magic -1 or 0? It would allow us to if-check if (m_eventFD) instead of if (m_eventFD != -1) Also we could reset with m_eventFD = Nullopt; And access its value with either *m_eventFD or m_eventFD.value(). WDYT?
(In reply to comment #3) > Comment on attachment 282540 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282540&action=review > > Looking good. One overall suggestion to use Optional rather than -1/0. > > > Source/WebCore/platform/MemoryPressureHandler.h:176 > > + int m_eventFD { -1 }; > > + int m_pressureLevelFD { -1 }; > > since we are touch these now, can we make it Optional<int> instead of using > magic -1 or 0? > > It would allow us to if-check > > if (m_eventFD) > > instead of > > if (m_eventFD != -1) > > Also we could reset with > > m_eventFD = Nullopt; > > And access its value with either *m_eventFD or m_eventFD.value(). > > WDYT? The thing is that open() returns -1 in case of failure, but we can probably handle that particular case separately and set it to Nullopt.
Created attachment 283522 [details] Updated patch Use Optional for file descriptors as suggested by Antonio.
Attachment 283522 [details] did not pass style-queue: ERROR: Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:96: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:242: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/MemoryPressureHandler.h:146: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/MemoryPressureHandler.h:153: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 283522 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=283522&action=review > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:108 > + EventFDSource* eventFDSource = reinterpret_cast<EventFDSource*>(m_source.get()); that reinterpret_cast was a bit tricky to understand, but Zan cleared it up (line 101 allocates m_source with "sizeof(EventFDSource)". > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:145 > + LOG(MemoryPressure, "Invalidate eventfd."); s/Invalidate/Invalid/g > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:230 > + m_eventFDPoller = std::make_unique<EventFDPoller>(m_eventFD.value(), [this] { So my understand is that m_eventFDPoller is an alternative memory pressure source that works independent from the existing 'cgroup' solution, right? However, it gets initialized after the cgroup initialization stuff (lines 209 and 215). So, if cgroup fails to initialize - which happens in most cases - then ::install bails out earlier (lines 218 and 225) and line 230 is not executed. Should line 230 be our first option, and cgroup the second? > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:234 > + // FIXME: Current memcg does not provide any way for users to know how serious the memory pressure is. > + // So we assume all notifications from memcg are critical for now. If memcg had better inferfaces > + // to get a detailed memory pressure level in the future, we should update here accordingly. > + bool critical = true; I believe a similar comment could be in MemoryPressureHandler::EventFDPoller::readAndNotify(), before we call m_nofityHandler(); Maybe a reduced version of it?
(In reply to comment #7) > Comment on attachment 283522 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=283522&action=review > > > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:108 > > + EventFDSource* eventFDSource = reinterpret_cast<EventFDSource*>(m_source.get()); > > that reinterpret_cast was a bit tricky to understand, but Zan cleared it up > (line 101 allocates m_source with "sizeof(EventFDSource)". Yes, it's the way GSource works, since it's not a GObject, we can't created a derived GSource, so the "constructor" takes a struct size, so it can allocate a larger struct for derived GSources. > > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:145 > > + LOG(MemoryPressure, "Invalidate eventfd."); > > s/Invalidate/Invalid/g I think I copy pasted this, the intention was probably Invalidated. > > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:230 > > + m_eventFDPoller = std::make_unique<EventFDPoller>(m_eventFD.value(), [this] { > > So my understand is that m_eventFDPoller is an alternative memory pressure > source that works independent from the existing 'cgroup' solution, right? No, cgroups work with an evenfd. You create an eventfd and pass the file descriptor to the cgroup event control, by writing a line to /sys/fs/cgroup/memory/cgroup.event_control. The kernel uses the passed fd to notify about critical memory situations. In bug #155255 I also use an eventfd because it's light an efficient and we can reuse most of the cgroups code. > However, it gets initialized after the cgroup initialization stuff (lines > 209 and 215). So, if cgroup fails to initialize - which happens in most > cases - then ::install bails out earlier (lines 218 and 225) and line 230 is > not executed. > > Should line 230 be our first option, and cgroup the second? > > > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:234 > > + // FIXME: Current memcg does not provide any way for users to know how serious the memory pressure is. > > + // So we assume all notifications from memcg are critical for now. If memcg had better inferfaces > > + // to get a detailed memory pressure level in the future, we should update here accordingly. > > + bool critical = true; > > I believe a similar comment could be in > MemoryPressureHandler::EventFDPoller::readAndNotify(), before we call > m_nofityHandler(); Maybe a reduced version of it? Actually we can have different notifications for memory pressure from cgroups, but we would need two eventfds, and i'm not sure it's really worth it. For now this patch doesn't try to change any behavior, just keep what we currently do, but with a GSource in case of using GLib because it's more efficient than using a thread.
Comment on attachment 283522 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=283522&action=review r=me. I think I can't make use of it yet out of the box on my Linux box, because applications (including webkit based browsers) can not read '/sys/fs/cgroup/memory/memory.pressure_level'. $ ls -ls /sys/fs/cgroup/memory/ (..) 0 --w--w--w-. 1 root root 0 Jul 5 11:37 cgroup.event_control (..) 0 ----------. 1 root root 0 Jul 5 11:37 memory.pressure_level (..) I have to manually change the permissions. >>> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:230 >>> + m_eventFDPoller = std::make_unique<EventFDPoller>(m_eventFD.value(), [this] { >> >> So my understand is that m_eventFDPoller is an alternative memory pressure source that works independent from the existing 'cgroup' solution, right? >> >> However, it gets initialized after the cgroup initialization stuff (lines 209 and 215). So, if cgroup fails to initialize - which happens in most cases - then ::install bails out earlier (lines 218 and 225) and line 230 is not executed. >> >> Should line 230 be our first option, and cgroup the second? > > No, cgroups work with an evenfd. You create an eventfd and pass the file descriptor to the cgroup event control, by writing a line to /sys/fs/cgroup/memory/cgroup.event_control. The kernel uses the passed fd to notify about critical memory situations. In bug #155255 I also use an eventfd because it's light an efficient and we can reuse most of the cgroups code. Ok.
Committed r203216: <http://trac.webkit.org/changeset/203216>
Comment on attachment 283522 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=283522&action=review > Source/WebCore/ChangeLog:16 > + (WebCore::MemoryPressureHandler::EventFDPoller::EventFDPoller): Helper class do the eventFD polling. This change isn't right it seems. Crash trace: STDERR: 1 0x7fa2ba41736d /home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x1d) [0x7fa2ba41736d] STDERR: 2 0x7fa2bfa0db05 /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WTF::Optional<int>::value()+0x45) [0x7fa2bfa0db05] STDERR: 3 0x7fa2c168a49a /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::MemoryPressureHandler::logErrorAndCloseFDs(char const*)+0xaa) [0x7fa2c168a49a] STDERR: 4 0x7fa2c1688dec /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::MemoryPressureHandler::install()+0xcc) [0x7fa2c1688dec] STDERR: 5 0x7fa2bf964ba1 /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebKit::NetworkProcess::initializeNetworkProcess(WebKit::NetworkProcessCreationParameters const&)+0xc1) [0x7fa2bf964ba1] STDERR: 6 0x7fa2bfb62576 /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(void IPC::callMemberFunctionImpl<WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(WebKit::NetworkProcessCreationParameters const&), std::tuple<WebKit::NetworkProcessCreationParameters>, 0ul>(WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(WebKit::NetworkProcessCreationParameters const&), std::tuple<WebKit::NetworkProcessCreationParameters>&&, std::integer_sequence<unsigned long, 0ul>)+0x96) [0x7fa2bfb62576] STDERR: 7 0x7fa2bfb622ac /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(void IPC::callMemberFunction<WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(WebKit::NetworkProcessCreationParameters const&), std::tuple<WebKit::NetworkProcessCreationParameters>, std::integer_sequence<unsigned long, 0ul> >(std::tuple<WebKit::NetworkProcessCreationParameters>&&, WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(WebKit::NetworkProcessCreationParameters const&))+0x6c) [0x7fa2bfb622ac] STDERR: 8 0x7fa2bfb60b9a /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(void IPC::handleMessage<Messages::NetworkProcess::InitializeNetworkProcess, WebKit::NetworkProcess, void (WebKit::NetworkProcess::*)(WebKit::NetworkProcessCreationParameters const&)>(IPC::MessageDecoder&, WebKit::NetworkProcess*, void (WebKit::NetworkProcess::*)(WebKit::NetworkProcessCreationParameters const&))+0x14a) [0x7fa2bfb60b9a] STDERR: 9 0x7fa2bfb5fde5 /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebKit::NetworkProcess::didReceiveNetworkProcessMessage(IPC::Connection&, IPC::MessageDecoder&)+0x85) [0x7fa2bfb5fde5] STDERR: 10 0x7fa2bf96486b /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebKit::NetworkProcess::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&)+0xab) [0x7fa2bf96486b] STDERR: 11 0x7fa2bf457243 /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(IPC::Connection::dispatchMessage(IPC::MessageDecoder&)+0x33) [0x7fa2bf457243] STDERR: 12 0x7fa2bf451f86 /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(IPC::Connection::dispatchMessage(std::unique_ptr<IPC::MessageDecoder, std::default_delete<IPC::MessageDecoder> >)+0x166) [0x7fa2bf451f86] STDERR: 13 0x7fa2bf45736b /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(IPC::Connection::dispatchOneMessage()+0x11b) [0x7fa2bf45736b] STDERR: 14 0x7fa2bf45cc5d /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0x43cec5d) [0x7fa2bf45cc5d] STDERR: 15 0x7fa2bf45cc39 /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0x43cec39) [0x7fa2bf45cc39] STDERR: 16 0x7fa2bf41d770 /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WTF::Function<void ()>::operator()() const+0x40) [0x7fa2bf41d770] STDERR: 17 0x7fa2ba4380cd /home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTF::RunLoop::performWork()+0x11d) [0x7fa2ba4380cd] STDERR: 18 0x7fa2ba488a9c /home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x21cca9c) [0x7fa2ba488a9c] STDERR: 19 0x7fa2ba488a78 /home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x21cca78) [0x7fa2ba488a78] STDERR: 20 0x7fa2ba488a51 /home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x21cca51) [0x7fa2ba488a51] STDERR: 21 0x7fa2ba4889f8 /home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x21cc9f8) [0x7fa2ba4889f8] STDERR: 22 0x7fa2b3601601 /home/phil/WebKit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(+0x53601) [0x7fa2b3601601] STDERR: 23 0x7fa2b3602438 /home/phil/WebKit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(g_main_context_dispatch+0x33) [0x7fa2b3602438] STDERR: 24 0x7fa2b360261c /home/phil/WebKit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(+0x5461c) [0x7fa2b360261c] STDERR: 25 0x7fa2b3602a42 /home/phil/WebKit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(g_main_loop_run+0x1d5) [0x7fa2b3602a42] STDERR: 26 0x7fa2ba4881ef /home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTF::RunLoop::run()+0xaf) [0x7fa2ba4881ef] STDERR: 27 0x7fa2bfa065cd /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(int WebKit::ChildProcessMain<WebKit::NetworkProcess, WebKit::NetworkProcessMain>(int, char**)+0xfd) [0x7fa2bfa065cd] STDERR: 28 0x7fa2bfa064bb /home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(NetworkProcessMainUnix+0x1b) [0x7fa2bfa064bb] STDERR: 29 0x400d16 /home/phil/WebKit/WebKitBuild/Debug/bin/WebKitNetworkProcess(main+0x46) [0x400d16] STDERR: 30 0x7fa2ae6055f0 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0) [0x7fa2ae6055f0] STDERR: 31 0x400bf9 /home/phil/WebKit/WebKitBuild/Debug/bin/WebKitNetworkProcess(_start+0x29) [0x400bf9]
Comment on attachment 283522 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=283522&action=review > Source/WebCore/ChangeLog:17 > + (WebCore::MemoryPressureHandler::logErrorAndCloseFDs): Check if file descriptors are -1 not 0. I mean this change ^^ :)
STDERR: Failed to open memory.pressure_level, error : No such file or directory STDERR: ASSERTION FAILED: m_isEngaged STDERR: ../../Source/WTF/wtf/Optional.h(156) : T &WTF::Optional<int>::value() [T = int] STDERR:
Comment on attachment 283522 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=283522&action=review > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:187 > + if (!m_eventFD) { > + close(m_eventFD.value()); hum, I believe this is the problem.
(In reply to comment #14) > Comment on attachment 283522 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=283522&action=review > > > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:187 > > + if (!m_eventFD) { > > + close(m_eventFD.value()); > > hum, I believe this is the problem. Indeed, I messed it up when converting it to Optional :-( Let me fix it.
Reopening to submit a follow up
Created attachment 283642 [details] Follow up
Comment on attachment 283642 [details] Follow up Clearing flags on attachment: 283642 Committed r203224: <http://trac.webkit.org/changeset/203224>
All reviewed patches have been landed. Closing bug.