Bug 155255 – MemoryPressureHandler doesn't work if cgroups aren't present in Linux

• Component: Platform
• Status: RESOLVED FIXED

[Enrique Ocaña]

The current Linux implementation of MemoryPressureHandler relies on /sys/fs/cgroup/memory to be properly setup. This isn't the case for all desktop systems and embedded devices, so having an alternative method to measure memory pressure should be desirable (especially on embedded devices with limited memory).

[Enrique Ocaña]

Created attachment 273468 [details]
Patch

[Michael Catanzaro]

Related: the current code tries to read /sys/fs/cgroup/memory/memory.pressure_level, but this file has no read permission set, at least on Fedora, so I have no clue how the current code could possibly work. ChangSeok, any thoughts on this?

[Michael Catanzaro]

By the way, I noticed this when debugging seccomp filters months ago, looking at calls to open() that were failing.

In /sys/fs/cgroup/memory, I see:

$ ls -l
total 0
-rw-r--r--. 1 root root 0 Mar 11 12:17 cgroup.clone_children
--w--w--w-. 1 root root 0 Mar 11 12:17 cgroup.event_control
-rw-r--r--. 1 root root 0 Mar 11 12:17 cgroup.procs
-r--r--r--. 1 root root 0 Mar 11 12:17 cgroup.sane_behavior
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.failcnt
--w-------. 1 root root 0 Mar 11 12:17 memory.force_empty
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.kmem.failcnt
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.kmem.limit_in_bytes
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.kmem.max_usage_in_bytes
-r--r--r--. 1 root root 0 Mar 11 12:17 memory.kmem.slabinfo
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.kmem.tcp.failcnt
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.kmem.tcp.limit_in_bytes
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.kmem.tcp.max_usage_in_bytes
-r--r--r--. 1 root root 0 Mar 11 12:17 memory.kmem.tcp.usage_in_bytes
-r--r--r--. 1 root root 0 Mar 11 12:17 memory.kmem.usage_in_bytes
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.limit_in_bytes
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.max_usage_in_bytes
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.memsw.failcnt
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.memsw.limit_in_bytes
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.memsw.max_usage_in_bytes
-r--r--r--. 1 root root 0 Mar 11 12:17 memory.memsw.usage_in_bytes
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.move_charge_at_immigrate
-r--r--r--. 1 root root 0 Mar 11 12:17 memory.numa_stat
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.oom_control
----------. 1 root root 0 Mar 11 12:17 memory.pressure_level
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.soft_limit_in_bytes
-r--r--r--. 1 root root 0 Mar 11 12:17 memory.stat
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.swappiness
-r--r--r--. 1 root root 0 Mar 11 12:17 memory.usage_in_bytes
-rw-r--r--. 1 root root 0 Mar 11 12:17 memory.use_hierarchy
-rw-r--r--. 1 root root 0 Mar 11 12:17 notify_on_release
-rw-r--r--. 1 root root 0 Mar 11 12:17 release_agent
-rw-r--r--. 1 root root 0 Mar 11 12:17 tasks

Note the ----------. permission on memory.pressure_level

[Martin Robinson]

Comment on attachment 273468 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273468&action=review

Looks good, but I think it needs a little bit of cleanup. Thanks!

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:54
> +static const unsigned s_pollTimeSec = 1;

We try to avoid abbreviated variable names in WebKit. How about s_pollingIntervalInSeconds?

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:56
> +static const size_t s_memCriticalLimit = 100 * KB * KB; // 100 MB
> +static const size_t s_memNonCriticalLimit = 300 * KB * KB; // 300 MB

memory isn't abbreviated below, so it probably shouldn't be abbreviated here.

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:118
> +        size_t memFree = static_cast<size_t>(-1); // bytes

I think this comment could be expanded. What does "bytes" signify here. Comments in WebKit should be complete sentences with periods. If you are trying to say that this value is stored in bytes, a better method would simply be to call this variable "memoryFreeInBytes."

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:135
> +            bool critical = memFree < s_memCriticalLimit;

Maybe better to call this underMemoryPressure or something similar.

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:210
> +    } while (false);

Probably better to use a helper function here rather than do { } while.

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:225
> +        do {
> +            m_threadID = createThread(pollMemoryPressure, this, "WebCore: MemoryPressureHandler");
> +            if (!m_threadID) {
> +                logErrorAndCloseFDs("Failed to create a thread for MemoryPressureHandler");
> +                break;
> +            }
> +
> +            LOG(MemoryPressure, "Vmstat memory pressure handler installed.");
> +            vmstatPressureHandlerOk = true;
> +        } while (false);

Probably better to use a helper function here rather than do { } while.

[Enrique Ocaña]

Created attachment 273748 [details]
Patch

[Martin Robinson]

Comment on attachment 273748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273748&action=review

Looks good to me, but I think it makes sense to have someone more closely involved with the port sign off an polling proc every second.

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:56
> +static const size_t s_memoryCriticalLimit = 100 * KB * KB; // 100 MB
> +static const size_t s_memoryNonCriticalLimit = 300 * KB * KB; // 300 MB

It might be worth discussing a bit where these numbers come from. A comment here or a note in the ChangeLog or bug is fine.

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:224
> +    if (!installUsingCgroups() && !installUsingMeminfo())
> +        return;

Much nicer!

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:227
>      if (ReliefLogger::loggingEnabled() && isUnderMemoryPressure())
>          LOG(MemoryPressure, "System is no longer under memory pressure.");

This is quite strange. If isUnderMemoryPressure returns true, a message is printed that the system is not under memory pressure. Do you have understand why this is?

[Enrique Ocaña]

(In reply to comment #6)

> Looks good to me, but I think it makes sense to have someone more closely
> involved with the port sign off an polling proc every second.

Ok.

> > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:56
> > +static const size_t s_memoryCriticalLimit = 100 * KB * KB; // 100 MB
> > +static const size_t s_memoryNonCriticalLimit = 300 * KB * KB; // 300 MB
> It might be worth discussing a bit where these numbers come from. A comment
> here or a note in the ChangeLog or bug is fine.

Those numbers are arbitrary. I found them right for our downstream port, where we're interested in minimizing the RAM usage.

> > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:227
> >      if (ReliefLogger::loggingEnabled() && isUnderMemoryPressure())
> >          LOG(MemoryPressure, "System is no longer under memory pressure.");
> 
> This is quite strange. If isUnderMemoryPressure returns true, a message is
> printed that the system is not under memory pressure. Do you have understand
> why this is?

Hmmm... actually not. Changseok should know.

[Michael Catanzaro]

(In reply to comment #6)
> Looks good to me, but I think it makes sense to have someone more closely
> involved with the port sign off an polling proc every second.

I can't think of any reason not to. Once per second is reasonable.

But it'd probably be nicer to do this with real poll or epoll, rather than sleep, so we don't need a new thread for it?

[Michael Catanzaro]

(In reply to comment #8)
> But it'd probably be nicer to do this with real poll or epoll, rather than
> sleep, so we don't need a new thread for it?

I talked with mrobinson about this; we think it's fine as-is.

[ChangSeok Oh]

(In reply to comment #0)
> The current Linux implementation of MemoryPressureHandler relies on
> /sys/fs/cgroup/memory to be properly setup. This isn't the case for all
> desktop systems and embedded devices, so having an alternative method to
> measure memory pressure should be desirable (especially on embedded devices
> with limited memory).

As I rememeber, the polling way was considered but it was rejected because of a concern that regularly waking up cpu to read meminfo itself badly affected on performance. https://bugs.webkit.org/show_bug.cgi?id=123532#c2 . For the reason, it is intended that MemoryPressureHandler works for only cgroup avaiable platforms. I know there is a small difference between this approach and previous one, using a timer or sleep() in a separate thread. I am also glad to see this alernative memory handler though, I would like to ask if there is anything different from using timer way and this patch is actually useful on real embeded systems. How about performance there? Is it acceptable?

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #6)
> > Looks good to me, but I think it makes sense to have someone more closely
> > involved with the port sign off an polling proc every second.
> 
> I can't think of any reason not to. Once per second is reasonable.
> 
> But it'd probably be nicer to do this with real poll or epoll, rather than
> sleep, so we don't need a new thread for it?

I'm not sure /proc/meminfo is pollable, the kernel probably gives the contents directly when reading, but I haven't actually checked, it's just a guess.

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #0)
> > The current Linux implementation of MemoryPressureHandler relies on
> > /sys/fs/cgroup/memory to be properly setup. This isn't the case for all
> > desktop systems and embedded devices, so having an alternative method to
> > measure memory pressure should be desirable (especially on embedded devices
> > with limited memory).
> 
> As I rememeber, the polling way was considered but it was rejected because
> of a concern that regularly waking up cpu to read meminfo itself badly
> affected on performance. https://bugs.webkit.org/show_bug.cgi?id=123532#c2 .
> For the reason, it is intended that MemoryPressureHandler works for only
> cgroup avaiable platforms. I know there is a small difference between this
> approach and previous one, using a timer or sleep() in a separate thread. I
> am also glad to see this alernative memory handler though, I would like to
> ask if there is anything different from using timer way and this patch is
> actually useful on real embeded systems. How about performance there? Is it
> acceptable?

I agree, for embedded devices these wakeups could affect the battery life.

[Carlos Garcia Campos]

Comment on attachment 273748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273748&action=review

cq- for now while we discuss whether it's worth it supporting this or not.

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:143
> +            if (ReliefLogger::loggingEnabled())
> +                LOG(MemoryPressure, "Polled memory pressure (%s)", isUnderMemoryPressure ? "critical" : "non-critical");
> +
> +            MemoryPressureHandler::singleton().setUnderMemoryPressure(isUnderMemoryPressure);
> +            callOnMainThread([isUnderMemoryPressure] {
> +                MemoryPressureHandler::singleton().respondToMemoryPressure(isUnderMemoryPressure ? Critical::Yes : Critical::No);
> +            });

This is mostly duplicated code, we could probably move it to a function that receives the isUnderMemoryPressure value.

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:149
> +        sleep(s_pollingIntervalInSeconds);
> +    } while (true);

I'm not happy with this either, but I have no idea how to do this if meminfo is not pollable. Note that there's one memory pressure handler per process . . .

[Enrique Ocaña]

(In reply to comment #11)

> I'm not sure /proc/meminfo is pollable, the kernel probably gives the
> contents directly when reading, but I haven't actually checked, it's just a
> guess.

/proc/meminfo doesn't seem to be pollable. I've tried the script "inotifywait /proc/meminfo" and it doesn't get any event, while for a normal text file modified from another terminal it does get the events.

[Enrique Ocaña]

(In reply to comment #13)

> > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:149
> > +        sleep(s_pollingIntervalInSeconds);
> > +    } while (true);
> 
> I'm not happy with this either, but I have no idea how to do this if meminfo
> is not pollable. Note that there's one memory pressure handler per process .

We can use a larger polling interval (5 sec?) and wider memory threshold margins so that we trigger the pressure condition earlier without needing such a high monitoring frequency. Actually, I thing that the current thresholds for remaining memory of 300MB (non critical) and 100MB (critical) are wide enough.

About battery consumption, take into account the relative expense of "opening a text file, parsing its first lines and closing it". What can it be, some milliseconds? I can't believe it's so expensive compared to the huge amount of time (in terms of CPU time scale) that the thread is going to be idle.

[Enrique Ocaña]

(In reply to comment #10)

> As I rememeber, the polling way was considered but it was rejected because
> of a concern that regularly waking up cpu to read meminfo itself badly
> affected on performance. https://bugs.webkit.org/show_bug.cgi?id=123532#c2 .
> For the reason, it is intended that MemoryPressureHandler works for only
> cgroup avaiable platforms. I know there is a small difference between this
> approach and previous one, using a timer or sleep() in a separate thread. I
> am also glad to see this alernative memory handler though, I would like to
> ask if there is anything different from using timer way and this patch is
> actually useful on real embeded systems. How about performance there? Is it
> acceptable?

I understand the concerns of polling on battery-backed devices, but for our use case (a RaspberryPi2 acting as a Set Top Box) the power consumption isn't a problem. What is really worrying is not being able to control the memory usage and having our application being OOM killed. I invested several days studying our browser with valgrind in our device only to find that we weren't actually having any leak, just a memory pressure handler that wasn't doing its job.

In any case, if battery usage is more worrying than OOM for battery-backed devices, what about reading some environment variables when the object is created and enable polling on demand? We could even parameterize the memory limits (per each process) if we use env vars.

[Enrique Ocaña]

Created attachment 273833 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #15)
> (In reply to comment #13)
> 
> > > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:149
> > > +        sleep(s_pollingIntervalInSeconds);
> > > +    } while (true);
> > 
> > I'm not happy with this either, but I have no idea how to do this if meminfo
> > is not pollable. Note that there's one memory pressure handler per process .
> 
> We can use a larger polling interval (5 sec?) and wider memory threshold
> margins so that we trigger the pressure condition earlier without needing
> such a high monitoring frequency. Actually, I thing that the current
> thresholds for remaining memory of 300MB (non critical) and 100MB (critical)
> are wide enough.
> 
> About battery consumption, take into account the relative expense of
> "opening a text file, parsing its first lines and closing it". What can it
> be, some milliseconds? I can't believe it's so expensive compared to the
> huge amount of time (in terms of CPU time scale) that the thread is going to
> be idle.

The problem is precisely that the thread is never going to be idle, it's going to wake up every second. It's not only a matter of what you do when you wake up, it's just that you are waking up every second. I've seen patches in embedded devices to fix battery drain that simply removed loops like this one. If there's no other way to do this, we should figure out if it's possible to disable this while there's no activity, or pages are all hidden, etc.

[ChangSeok Oh]

(In reply to comment #15)
> (In reply to comment #13)
> 
> > > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:149
> > > +        sleep(s_pollingIntervalInSeconds);
> > > +    } while (true);
> > 
> > I'm not happy with this either, but I have no idea how to do this if meminfo
> > is not pollable. Note that there's one memory pressure handler per process .
> 
> We can use a larger polling interval (5 sec?) and wider memory threshold
> margins so that we trigger the pressure condition earlier without needing
> such a high monitoring frequency. Actually, I thing that the current
> thresholds for remaining memory of 300MB (non critical) and 100MB (critical)
> are wide enough.
Larger polling interval, less useful MemoryPressureHandler. Memory drain sometimes happen instantly. It means the hitting rate on OOM would decrease, larger interval is set. I don't know how we could figure out a proper threadhold which makes all systems happy. the thresholds you said sound very heuristic to me. I understand the necessity of your patch for RPi, but this is upstream. We need to consider not only RPi but also other systems using gtk.

> 
> About battery consumption, take into account the relative expense of
> "opening a text file, parsing its first lines and closing it". What can it
> be, some milliseconds? I can't believe it's so expensive compared to the
> huge amount of time (in terms of CPU time scale) that the thread is going to
> be idle.
As KaL explained, opening/reading a file is quite different from what this patch does. The former is a one-time usecase but the latter is something happening forever. Regularly waking a cpu up would prevents systems from going sleep mode or low-power mode. I don't know if this should be covered in application level. The best way would be that we adopt an interface provided by the linux system and that is why the current MemoryPressureHandler is based on the cgroup even though it is not perfect. I hope the cgroup memory management to be improved near time soon. :P

[Sergio Villar Senin]

(In reply to comment #18)
> (In reply to comment #15)
> > (In reply to comment #13)
> > 
> > > > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:149
> > > > +        sleep(s_pollingIntervalInSeconds);
> > > > +    } while (true);
> > > 
> > > I'm not happy with this either, but I have no idea how to do this if meminfo
> > > is not pollable. Note that there's one memory pressure handler per process .
> > 
> > We can use a larger polling interval (5 sec?) and wider memory threshold
> > margins so that we trigger the pressure condition earlier without needing
> > such a high monitoring frequency. Actually, I thing that the current
> > thresholds for remaining memory of 300MB (non critical) and 100MB (critical)
> > are wide enough.
> > 
> > About battery consumption, take into account the relative expense of
> > "opening a text file, parsing its first lines and closing it". What can it
> > be, some milliseconds? I can't believe it's so expensive compared to the
> > huge amount of time (in terms of CPU time scale) that the thread is going to
> > be idle.
> 
> The problem is precisely that the thread is never going to be idle, it's
> going to wake up every second. It's not only a matter of what you do when
> you wake up, it's just that you are waking up every second. I've seen
> patches in embedded devices to fix battery drain that simply removed loops
> like this one. If there's no other way to do this, we should figure out if
> it's possible to disable this while there's no activity, or pages are all
> hidden, etc.

Yes please, let's not land something like this unless we want embedded devices not to use webkitgtk

[Enrique Ocaña]

(In reply to comment #16)

> In any case, if battery usage is more worrying than OOM for battery-backed
> devices, what about reading some environment variables when the object is
> created and enable polling on demand? We could even parameterize the memory
> limits (per each process) if we use env vars.

Nobody commented anything about this proposal of using an environment variable to enable and parameterize the new memory measurement alternative. It would be disabled if there's no environment variable defined and enabled otherwise. An example of how this environment variable could be defined:

export MEMORY_PRESSURE_HANDLER_MEMINFO=MiniBrowser:500M,WebProcess:100M/20M,*Process:500M/100M

The pattern is: MEMORY_PRESSURE_HANDLER_MEMINFO=<process-name-pattern>:[<optional-non-critical-limit>/]<critical-limit>, ...

I think this alternative would make everybody happy: no battery drain by default (when the variable isn't defined), and the memory exhaustion problem solved "if it's important enough for your use case and you know what you're doing".

[Carlos Alberto Lopez Perez]

I think we need something like this, because WebKit don't drops caches if the memory pressure handler is not working, which causes that the memory usage continues to grow forever causing all sorts of problems.

The current implementation based on cgroups doesn't work on a "standard" GNU/Linux system, because memory cgroups don't work out of the box.

To enable memory cgroups you need to configure quite a few things, you even have to modify the kernel boot parameters (by setting at least cgroup_enable=memory on it). Also the interfaces /sys/fs/cgroup/memory/cgroup.event_control and /sys/fs/cgroup/memory/memory.pressure_level are not accessible by a normal user by default (only by root)

So I doubt our current implementation based on cgroups is actually working on any system other than those the admin took care of fine tuning this or on embedded devices running webkit as root.

So I think this patch is a good fallback for the rest of systems (mainly Desktops running a standard Linux distribution like ours).

Regarding the concerns about that the wakeups could affect the battery life I propose that we only enable this fallback memory pressure handler when the system is not running on battery.

We can reliably detect this by inspecting some values on /sys. For example, we can borrow either the systemd implementation to detect this <https://github.com/systemd/systemd/blob/master/src/basic/util.c#L498> or the e2fsprogs one <http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git/tree/e2fsck/unix.c#n284> (yes, fsck avoids running automatically if it detects you are on battery)


And I don't think we should hide this feature behind an obscure environment variable than other than we developers and power users will know about, but make it run by default when the cgroup based memory pressure handler fails and when the system is not running on battery.


Note: is a shame that Linux (in 2016!) don't has a standard/reliable way to poll for low memory conditions.

[Carlos Garcia Campos]

A better approach could be to never wake up when process is idle, and poll only when there's other activity, since it's very unlikely that the memory grows if the process is idle. I'm not sure how we can detect that, though. Maybe we can invent a run loop source that is only triggered when other run loops are dispatched, or something like that.

[Carlos Alberto Lopez Perez]

(In reply to comment #23)
> A better approach could be to never wake up when process is idle, and poll
> only when there's other activity, since it's very unlikely that the memory
> grows if the process is idle. I'm not sure how we can detect that, though.
> Maybe we can invent a run loop source that is only triggered when other run
> loops are dispatched, or something like that.

I don't think is unlikely at all to enter in a low memory scenario when webkit is idle.

Just suppose that the user minimizes the browser and loads up libreoffice or any other application.

In that situation webkit should be able to get the memory pressure notification as fast as possible and free the caches potentially keeping the system from swapping.

[Michael Catanzaro]

Comment on attachment 273833 [details]
Patch

I think this patch needs to address clopez's feedback:

"""Regarding the concerns about that the wakeups could affect the battery life I propose that we only enable this fallback memory pressure handler when the system is not running on battery.

We can reliably detect this by inspecting some values on /sys. For example, we can borrow either the systemd implementation to detect this <https://github.com/systemd/systemd/blob/master/src/basic/util.c#L498> or the e2fsprogs one <http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git/tree/e2fsck/unix.c#n284> (yes, fsck avoids running automatically if it detects you are on battery)


And I don't think we should hide this feature behind an obscure environment variable than other than we developers and power users will know about, but make it run by default when the cgroup based memory pressure handler fails and when the system is not running on battery."""

[Antonio Gomes]

(In reply to comment #25)
> Comment on attachment 273833 [details]
> Patch
> 
> I think this patch needs to address clopez's feedback:
> 
> """Regarding the concerns about that the wakeups could affect the battery
> life I propose that we only enable this fallback memory pressure handler
> when the system is not running on battery.
> 
> We can reliably detect this by inspecting some values on /sys. For example,
> we can borrow either the systemd implementation to detect this
> <https://github.com/systemd/systemd/blob/master/src/basic/util.c#L498> or
> the e2fsprogs one
> <http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git/tree/e2fsck/unix.c#n284>
> (yes, fsck avoids running automatically if it detects you are on battery)
> 
> 
> And I don't think we should hide this feature behind an obscure environment
> variable than other than we developers and power users will know about, but
> make it run by default when the cgroup based memory pressure handler fails
> and when the system is not running on battery."""

For the record, on ChromeOS, the kernel provides a file (available for reading if system is under low mem conditions):

// This is the file that will exist if low memory notification is available
// on the device.  Whenever it becomes readable, it signals a low memory
// condition.
const char kLowMemFile[] = "/dev/chromeos-low-mem";

No parsing needed of its content, just verifying its presence is enough.
The check happens every 1s, and I believe it would not be as expensive as actually reading and parsing /proc/meminfo every 1s.
If Linux would have something like this by default, it would do it, as far as I can tell.

[Carlos Garcia Campos]

Created attachment 282544 [details]
Different approach

This is a different approach to the previous patch. If there' no other way than polling an parsing /proc/meminfo, let's try to minimize the problems. There's a memory pressure handler in every secondary process, not only the web process but also in the network and plugin processes. So, this patch does the polling in a thread in the UI process to ensure it's only one process waking up. It uses eventFD to notify the other processes about memory pressure. The poll interval is calculated depending on the memory usage in every moment, between 1 and 15 seconds, to try to wake up less often when memory usage is low. Also instead of using a fixed size in MB of free memory as a threshold, it uses a percentage of the memory used. Everything can be configured just modifying some global variables in case any of the values I set is not good enough. And finally instead of using the MemFree value from /proce/meminfo, it uses the MemAvailable value, that was added to the kernel in version 3.something, with a fallback implementation based on the MemAvailable values implementation in the kernel in case of using an old kernel. This is still not perfect and it can probably be improved (use different configuration values, stop the poll in case of running on batery, etc.), but hopefully it works well enough.

[Carlos Garcia Campos]

(In reply to comment #27)
> Created attachment 282544 [details]
> Different approach
> 
> This is a different approach to the previous patch. If there' no other way
> than polling an parsing /proc/meminfo, let's try to minimize the problems.
> There's a memory pressure handler in every secondary process, not only the
> web process but also in the network and plugin processes. So, this patch
> does the polling in a thread in the UI process to ensure it's only one
> process waking up. It uses eventFD to notify the other processes about
> memory pressure. The poll interval is calculated depending on the memory
> usage in every moment, between 1 and 15 seconds,

Oops, I meant between 1 and 5.

[Sergio Villar Senin]

Comment on attachment 282544 [details]
Different approach

Looks good to me in general, but the mix of negative numbers and size_t is a bit shocking to me.

[ChangSeok Oh]

(In reply to comment #27)
> Created attachment 282544 [details]
> Different approach
> 
> This is a different approach to the previous patch. If there' no other way
> than polling an parsing /proc/meminfo, let's try to minimize the problems.
> There's a memory pressure handler in every secondary process, not only the
> web process but also in the network and plugin processes. So, this patch
> does the polling in a thread in the UI process to ensure it's only one
> process waking up. It uses eventFD to notify the other processes about
> memory pressure. The poll interval is calculated depending on the memory
> usage in every moment, between 1 and 15 seconds, to try to wake up less
> often when memory usage is low. Also instead of using a fixed size in MB of
> free memory as a threshold, it uses a percentage of the memory used.
> Everything can be configured just modifying some global variables in case
> any of the values I set is not good enough. And finally instead of using the
> MemFree value from /proce/meminfo, it uses the MemAvailable value, that was
> added to the kernel in version 3.something, with a fallback implementation
> based on the MemAvailable values implementation in the kernel in case of
> using an old kernel. This is still not perfect and it can probably be
> improved (use different configuration values, stop the poll in case of
> running on batery, etc.), but hopefully it works well enough.

Fair enough.

[Carlos Garcia Campos]

Created attachment 283524 [details]
Updated patch

Just rebased after updating patch for bug #159346

[Carlos Garcia Campos]

(In reply to comment #29)
> Comment on attachment 282544 [details]
> Different approach
> 
> Looks good to me in general, but the mix of negative numbers and size_t is a
> bit shocking to me.

We can make the code a bit better by using a macro or global variable like DFGValidate or Vector do:

#define notSet (static_cast<size_t>(-1))
const size_t notFound = static_cast<size_t>(-1);

[Carlos Garcia Campos]

Created attachment 283609 [details]
Updated patch

[WebKit Commit Bot]

Attachment 283609 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:230:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 283624 [details]
Try to fix builds

[WebKit Commit Bot]

Attachment 283624 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:231:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sergio Villar Senin]

Comment on attachment 283624 [details]
Try to fix builds

View in context: https://bugs.webkit.org/attachment.cgi?id=283624&action=review

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:196
> +bool MemoryPressureHandler::tryEnsureEventFD()

tryEnsure seems a bit weird name to me.

> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:196
> +void NetworkProcess::initializeNetworkProcess(NetworkProcessCreationParameters&& parameters)

Seems unrelated

> Source/WebKit2/PluginProcess/PluginProcess.cpp:142
> +    memoryPressureHandler.install();

I understand you move this here because you need to set the handle before the install, I hope this does not add any race condition I've seen before when moving stuff like this.

> Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:47
> +static const size_t notSet = static_cast<size_t>(-1);

Instead of notSet, consider making  lowWatermarkPages() and calculateMemoryAvailable() return an Optional<size_t>.

> Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:189
> +        // See https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34e431b0ae398fc54ea69ff85ec700722c9da773

I'd find more useful to have this link just above the function definition.

> Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:195
> +    return ((memoryTotal - memoryAvailable) * 100) / memoryTotal;

Do we have any guarantee that memoryTotal is not 0?

Do we have any guarantee that memoryTotal > memoryAvailable ?

[Carlos Garcia Campos]

(In reply to comment #37)
> Comment on attachment 283624 [details]
> Try to fix builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283624&action=review

Thanks for the review.

> > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:196
> > +bool MemoryPressureHandler::tryEnsureEventFD()
> 
> tryEnsure seems a bit weird name to me.

It tries to ensure an eventFD. The try is because it can fail, and the ensure is because it doesn't return the new FD, but updates the member, and does nothing if we already have an fd.

> > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:196
> > +void NetworkProcess::initializeNetworkProcess(NetworkProcessCreationParameters&& parameters)
> 
> Seems unrelated

It's not actually. We can't do Attachment::releaseFileDescriptor with a const reference. This change is consistent with all other initializeFooProcess methods.

> > Source/WebKit2/PluginProcess/PluginProcess.cpp:142
> > +    memoryPressureHandler.install();
> 
> I understand you move this here because you need to set the handle before
> the install, I hope this does not add any race condition I've seen before
> when moving stuff like this.

No, I don't think so, this is what all other processes do, so this is also consistent.

> > Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:47
> > +static const size_t notSet = static_cast<size_t>(-1);
> 
> Instead of notSet, consider making  lowWatermarkPages() and
> calculateMemoryAvailable() return an Optional<size_t>.

Ok, I'll check it again.

> > Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:189
> > +        // See https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34e431b0ae398fc54ea69ff85ec700722c9da773
> 
> I'd find more useful to have this link just above the function definition.
> 
> > Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:195
> > +    return ((memoryTotal - memoryAvailable) * 100) / memoryTotal;
> 
> Do we have any guarantee that memoryTotal is not 0?
> 
> Do we have any guarantee that memoryTotal > memoryAvailable ?

I'll check this

[Carlos Garcia Campos]

Committed r203342: <http://trac.webkit.org/changeset/203342>

[Antonio Gomes]

Comment on attachment 283624 [details]
Try to fix builds

View in context: https://bugs.webkit.org/attachment.cgi?id=283624&action=review

> Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:29
> +#if OS(LINUX)

is OS(LINUX) needed given that we are within Source/WebKit2/UIProcess/*linux*/...?

> Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:128
> +static size_t calculateMemoryAvailable(size_t memoryFree, size_t activeFile, size_t inactiveFile, size_t slabReclaimable)
> +{
> +    if (memoryFree == notSet || activeFile == notSet || inactiveFile == notSet || slabReclaimable == notSet)
> +        return notSet;
> +
> +    size_t lowWatermark = lowWatermarkPages();
> +    if (lowWatermark == notSet)
> +        return notSet;
> +
> +    lowWatermark *= systemPageSize() / KB;
> +
> +    // Estimate the amount of memory available for userspace allocations, without causing swapping.
> +    // Free memory cannot be taken below the low watermark, before the system starts swapping.
> +    lowWatermark *= systemPageSize() / KB;
> +    size_t memoryAvailable = memoryFree - lowWatermark;
> +
> +    // Not all the page cache can be freed, otherwise the system will start swapping. Assume at least
> +    // half of the page cache, or the low watermark worth of cache, needs to stay.
> +    size_t pageCache = activeFile + inactiveFile;
> +    pageCache -= std::min(pageCache / 2, lowWatermark);
> +    memoryAvailable += pageCache;
> +
> +    // Part of the reclaimable slab consists of items that are in use, and cannot be freed.
> +    // Cap this estimate at the low watermark.
> +    memoryAvailable += slabReclaimable - std::min(slabReclaimable / 2, lowWatermark);
> +    return memoryAvailable;

Getting late to the party here, but is it really worth to to add support for kernels older than 2014/January? this adds some complexity to the patch that I would rather avoid if possible.

> Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:186
> +    if (memoryAvailable == notSet) {

I believe most recent kernels have "MemAvailable" in /proc/meminfo.

So cant we shortcut it here by only reading "MemFree", "Active(file)", "Inactive(file)" and "SReclaimable" only if mem == notSet?

So move part of the switch above (lines 150 - 176) to with the "if" line 186.

Also, we could name it explicitly like "readProcMemInfoEntriesForAncientKernels" or so.

> Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:227
> +bool MemoryPressureMonitor::isEnabled()

const?

[Carlos Garcia Campos]

(In reply to comment #40)
> Comment on attachment 283624 [details]
> Try to fix builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283624&action=review
> 
> > Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:29
> > +#if OS(LINUX)
> 
> is OS(LINUX) needed given that we are within
> Source/WebKit2/UIProcess/*linux*/...?

Right! we don't really need that here.

> > Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:128
> > +static size_t calculateMemoryAvailable(size_t memoryFree, size_t activeFile, size_t inactiveFile, size_t slabReclaimable)
> > +{
> > +    if (memoryFree == notSet || activeFile == notSet || inactiveFile == notSet || slabReclaimable == notSet)
> > +        return notSet;
> > +
> > +    size_t lowWatermark = lowWatermarkPages();
> > +    if (lowWatermark == notSet)
> > +        return notSet;
> > +
> > +    lowWatermark *= systemPageSize() / KB;
> > +
> > +    // Estimate the amount of memory available for userspace allocations, without causing swapping.
> > +    // Free memory cannot be taken below the low watermark, before the system starts swapping.
> > +    lowWatermark *= systemPageSize() / KB;
> > +    size_t memoryAvailable = memoryFree - lowWatermark;
> > +
> > +    // Not all the page cache can be freed, otherwise the system will start swapping. Assume at least
> > +    // half of the page cache, or the low watermark worth of cache, needs to stay.
> > +    size_t pageCache = activeFile + inactiveFile;
> > +    pageCache -= std::min(pageCache / 2, lowWatermark);
> > +    memoryAvailable += pageCache;
> > +
> > +    // Part of the reclaimable slab consists of items that are in use, and cannot be freed.
> > +    // Cap this estimate at the low watermark.
> > +    memoryAvailable += slabReclaimable - std::min(slabReclaimable / 2, lowWatermark);
> > +    return memoryAvailable;
> 
> Getting late to the party here, but is it really worth to to add support for
> kernels older than 2014/January? this adds some complexity to the patch that
> I would rather avoid if possible.

We support versions of our deps released even earlier (for example GTK+ 3.6, cairo 1.10.2, etc. released in 2012), because of old distros. I haven't checked the kernel version used by old distros, though, but I assumed it could be older. We don't really have a minimum kernel version supported. This is the same discussion of the minimum versions supported by our deps. I don't think it's that much complexity in any case.

> > Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:186
> > +    if (memoryAvailable == notSet) {
> 
> I believe most recent kernels have "MemAvailable" in /proc/meminfo.

Yes.

> So cant we shortcut it here by only reading "MemFree", "Active(file)",
> "Inactive(file)" and "SReclaimable" only if mem == notSet?

There's an early break before, so as soon as we read MemAvailable we stop reading more.

> So move part of the switch above (lines 150 - 176) to with the "if" line 186.

And reading twice? I prefer to read once and break as soon as we have the indo we need as we currently do. We stop reading when we have MemAvailable or all others needed for the fallback implementation.

> Also, we could name it explicitly like
> "readProcMemInfoEntriesForAncientKernels" or so.

There's a comment explaining why we have that function, but yes we could probably rename it. It's a private function in a cpp file, so not a big deal in any case.

> > Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:227
> > +bool MemoryPressureMonitor::isEnabled()
> 
> const?

It's static, this is used to avoid creating the singleton instance in case of using cgroups.

[Carlos Garcia Campos]

(In reply to comment #41)
> (In reply to comment #40)
> > Comment on attachment 283624 [details]
> > Try to fix builds
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=283624&action=review
> > 
> > > Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:29
> > > +#if OS(LINUX)
> > 
> > is OS(LINUX) needed given that we are within
> > Source/WebKit2/UIProcess/*linux*/...?
> 
> Right! we don't really need that here.

We actually need it, because we are unconditionally adding the file to the build, even for other unix systems like freebsd or macosx.