877222 - The low memory notification threshold is set too close to the homescreen OOM killer threshold

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Gabriele Svelto [:gsvelto]]

When free memory is running low on a device the kernel should dispatch low-memory events to the /sys/kernel/mm/lowmemkiller/notify_trigger_active pseudo-files. On recent versions of B2G this is not happening anymore, instead background applications are being killed by the lowmemkiller driver. Memory pressure notifications seem to be finally triggered only when no background processes remain.

The issue was caused by this change which raised the lowmemkiller minfree parameter for background processes to 20MiB:

https://hg.mozilla.org/mozilla-central/rev/2450d0d56213

The notify_trigger parameter is currently set at 10MiB and is thus never triggered as the OOM killer will start getting rid of the background processes instead. See this commit for more info on this behavior:

https://www.codeaurora.org/cgit/quic/la//kernel/msm/commit/?id=b3f986cba580b14438b77b42070ebbc77b69d4c4

"The threshold should be typically set to be higher than the largest free pages value in /sys/module/lowmemorykiller/parameters/minfree, so that notification happens before any processes get killed."

I've tested the issue on two devices (Otoro and Inari) and setting the notify_trigger to a higher value (e.g. 32MiB) solves the issue. We should thus find a proper value for both this trigger and the OOM killer highest threshold (and add an assertion to prevent this from happening again).

Comment 1

[Justin Lebar (not reading bugmail)]

> The notify_trigger parameter is currently set at 10MiB and is thus never triggered as 
> the OOM killer will start getting rid of the background processes instead.

I'm confused.

Is memory pressure "never triggered", or is it triggered correctly, but only after bg processes are killed?

The discussion about tuning the low-mem notification takes us back to the general difficulty of these notifications.  We will throw out all images in the fg app whenever we are under memory pressure (as defined by this threshold).  So basically images don't work on the phone if you're under the threshold.  Therefore it could be argued that the right thing to do is not to fire a memory-pressure event until after we've killed all of the bg processes.

I'm happy to discuss tuning the low-mem notification, but I suspect there are no good answers.  At least it would be helpful to separate the issue of "memory-pressure notifications don't work correctly" from "memory-pressure events aren't dispatched as early as they might be."

Comment 2

[Gabriele Svelto [:gsvelto]]

(In reply to Justin Lebar [:jlebar] from comment #1)
> I'm confused.
> 
> Is memory pressure "never triggered", or is it triggered correctly, but only
> after bg processes are killed?

It's triggered only after all bg processes have been killed.

> The discussion about tuning the low-mem notification takes us back to the
> general difficulty of these notifications.  We will throw out all images in
> the fg app whenever we are under memory pressure (as defined by this
> threshold).  So basically images don't work on the phone if you're under the
> threshold.  Therefore it could be argued that the right thing to do is not
> to fire a memory-pressure event until after we've killed all of the bg
> processes.

In the light of this then the current behavior is what we want. However this means that we're happy with killing bg apps while one of the reasons we had for putting this logic in place was to keep them alive for improved responsiveness. A middle-ground could be to change the patch for bug 873284 to never send memory-pressure events to a foreground app if it makes it essentially unusable (and I assume that not having images will make pretty much *any* app unusable). Alternatively did anybody try making a memory-pressure-but-dont-throw-away-images event that would be reserved for foreground apps? I could try that too and see if it helps. Yet another approach might be to vary the threshold dynamically, pushing it below the bg-gets-killed threshold after the first notification and resetting it only once a new app gets in the foreground.

> I'm happy to discuss tuning the low-mem notification, but I suspect there
> are no good answers.  At least it would be helpful to separate the issue of
> "memory-pressure notifications don't work correctly" from "memory-pressure
> events aren't dispatched as early as they might be."

I think we're in the latter situation here and yes, there's probably no perfect solution. The current behavior however makes low-memory notifications so unlikely to happen that IMHO we might as well get rid of them (and their added complexity) altogether.

Comment 3

[Justin Lebar (not reading bugmail)]

We can't get rid of low-memory notifications; we rely on them, for example, to know when to discard images in the fg app.  Andreas made some changes so we never discard in the foreground except when we get a low-memory notification.

I don't understand why notifications are unlikely to occur.  Once we've killed all of the bg apps, is there some additional problem that keeps them from happening?

Comment 4

[Gabriele Svelto [:gsvelto]]

(In reply to Justin Lebar [:jlebar] from comment #3)
> We can't get rid of low-memory notifications; we rely on them, for example,
> to know when to discard images in the fg app.  Andreas made some changes so
> we never discard in the foreground except when we get a low-memory
> notification.

OK.

> I don't understand why notifications are unlikely to occur.  Once we've
> killed all of the bg apps, is there some additional problem that keeps them
> from happening?

The kill-the-homescreen is set at 8MiB, that leaves a 2MiB range for the low-memory notification to be triggered before the homescreen gets killed which is pretty slim. Once the homescreen has been killed the kill-the-foreground-app threshold is set at 6MiB which again leaves only a 4MiB range for the low-memory notification to be triggered; it's going to be a bit more likely but still pretty slim.

I was thinking of experimenting with a dual-level approach with a higher threshold (above the kill-the-bg-apps level) that sends a low-memory notification to background apps only and then lowers the threshold below the kill-the-bg-apps threshold. Once there it would send low-memory notifications to all apps. This would be reset every time the foreground app changes... Does this sound a reasonable approach to you to retain the existing behavior but giving us more leeway before killing the bg apps?

Comment 5

[Justin Lebar (not reading bugmail)]

> Once the homescreen has been killed the kill-the-foreground-app threshold is set at 6MiB 
> which again leaves only a 4MiB range for the low-memory notification to be triggered; 
> it's going to be a bit more likely but still pretty slim.

Aha, I see.  Okay.

When an app goes into the bg, we run a minimize memory usage on it.  Doing so again at memory pressure might be nice, but I don't think it's critical, since we probably already squeezed most of the memory out of the process.  So if there's a simpler thing you think we can do, maybe we should do that.

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to Justin Lebar [:jlebar] from comment #5)
> When an app goes into the bg, we run a minimize memory usage on it.  Doing
> so again at memory pressure might be nice, but I don't think it's critical,
> since we probably already squeezed most of the memory out of the process. 
> So if there's a simpler thing you think we can do, maybe we should do that.

One quick solution might be to raise the notify_trigger threshold somewhere in-between the bg and homescreen thresholds say to 14MiB. This will ensure that:

- bg apps get killed first

- then we get memory-pressure events (which should be more likely)

- then the homescreen gets killed

This would be a one-line change in the b2g.js file and once we add the patch to prevent memory-pressure events from being delivered to FOREGROUND_HIGH applications should make the whole system fairly well behaved. What do you think about this?

Comment 7

[Justin Lebar (not reading bugmail)]

Sounds worth a shot to me!

Comment 8

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Increase the low memory notification threshold to increase the chance of triggering it

This patch moves the low memory threshold (hal.processPriorityManager.gonk.notifyLowMemUnderMB in b2g.js) to 14MiB. This puts the threshold exactly in between the OOM killer thresholds for the BACKGROUND_HOMESCREEN and the BACKGROUND priority classes hopefully increasing the chance of it triggering before apps above the BACKGROUND priority get killed. I'll test this patch in a few common scenarios to see how it behaves.

Comment 9

[Gabriele Svelto [:gsvelto]]

Comment on attachment 758593 [details] [diff] [review]
[PATCH] Increase the low memory notification threshold to increase the chance of triggering it

I've tested the following scenarios on my Otoro (the only 256MiB device I have where the memory trigger seems to work properly):

1) Membuster (with various increments)
2) Process buster
3) Opening up a bunch of apps

Without the patch applied I get the following behavior

1) Membuster triggers the notification correctly with 1MiB increments, 2MiB and above causes homescreen to be killed first
2) Process buster never triggers a notification, homescreen gets killed first
3) Opening apps never triggers a notification, homescreen gets killed first if killing bg apps is not enough

With the patch applied I get slightly better behavior

1) Membuster triggers the notification correctly when allocating in 1- or 2MiB increments, sometimes when doing 3, anything above that causes homescreen to die
2) Process buster triggers the low-memory notification pretty much always
3) Opening apps causes the notification to trigger from time to time but not very often; in general it depends on the starting amount of free memory and how fast the new app allocates memory to itself

I'm open for doing more tests if there's more scenarios where we want low-mem notifications. All in all this is a pretty safe change and I don't think it will have any downsides.

Comment 10

[Justin Lebar (not reading bugmail)]

I agree this is probably fine.

But to be clear, there is a downside.  We continuously fire a special type of low-memory notification that doesn't trigger GC when we're below this threshold.  That notification causes us to, among other things, discard all images, even ones onscreen.

So increasing the threshold makes it more likely that we'll get into this continuous memory-pressure scenario, where images simply don't work.

So it's a trade-off.  If we increased the threshold to 50mb or something, that would be on the wrong side of the trade-off.  I agree that this is probably fine.

Comment 11

[Gabriele Svelto [:gsvelto]]

(In reply to Justin Lebar [:jlebar] from comment #10)
> But to be clear, there is a downside.  We continuously fire a special type
> of low-memory notification that doesn't trigger GC when we're below this
> threshold.  That notification causes us to, among other things, discard all
> images, even ones onscreen.

You mean low-memory-ongoing* right? Once I'm done with this and bug 873284 I intended to add a page to MDN's Firefox OS internals describing our memory management policy because it's becoming complex enough that most people wouldn't understand all aspects of it (including me) without doing some serious bug-rcheology to figure out all the decisions, rationales and interactions.

> So it's a trade-off.  If we increased the threshold to 50mb or something,
> that would be on the wrong side of the trade-off.  I agree that this is
> probably fine.

OK, sounds good. I'm changing the title to reflect the actual issue and adding checkin-needed right away; since this is a one line change to a configuration file I don't think it's worth wasting CPU time on the try servers.

For the sheriffs, the patch applies to mozilla-b2g18 and I think we probably want it there too so I'm asking for leo?. I'm not sure what to do about v1.0.1 as I don't know if patches are still being accepted for it or not.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Can you please state the user impact here?

Comment 13

[Gabriele Svelto [:gsvelto]]

(In reply to [e-mail] [:lsblakk] from comment #12)
> Can you please state the user impact here?

Without the patch applied the homescreen and background perceivable apps (e.g. music player, radio) are more likely to be killed under low memory conditions. So for example without the patch a heavy browsing session is more likely to kill the homescreen making the phone slightly less responsive as it would need to be started up again when the user presses the home button.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/786e8d981ae6

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/786e8d981ae6

Comment 16

[Alex Keybl [:akeybl]]

Based upon the minimal user impact  in comment 13 and the fact that this is not a new regression, this is not a blocker.