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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=159346

--- Comment #8 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160713/9a753dd5/attachment-0001.html>