Bug 159346 – [GLib] Use a GSource instead of a thread to poll memory pressure eventFD in linux implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 282540 [details]
Patch

[WebKit Commit Bot]

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.

[Antonio Gomes]

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?

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 283522 [details]
Updated patch

Use Optional for file descriptors as suggested by Antonio.

[WebKit Commit Bot]

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.

[Antonio Gomes]

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?

[Carlos Garcia Campos]

(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.

[Antonio Gomes]

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.

[Carlos Garcia Campos]

Committed r203216: <http://trac.webkit.org/changeset/203216>

[Philippe Normand]

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]

[Philippe Normand]

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 ^^ :)

[Philippe Normand]

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:

[Antonio Gomes]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Reopening to submit a follow up

[Carlos Garcia Campos]

Created attachment 283642 [details]
Follow up

[WebKit Commit Bot]

Comment on attachment 283642 [details]
Follow up

Clearing flags on attachment: 283642

Committed r203224: <http://trac.webkit.org/changeset/203224>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.