802423 - Updater needs to set oom_adj and nice appropriately

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P4)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Currently, while an update is being applied in the background, the device becomes nigh unusable.  We need to nice the updater *way* up so it doesn't affect UX.

Relatedly, it would be disastrous for the updater binary to crash while staging an update.  It uses relatively little memory, maxed out at about 10MB in a quick test I just did, so pinning it to the "system service" OOM class is totally fine.

Comment 1

[Marshall Culpepper [:marshall_law]]

We might also want to set the I/O priority, since the stager does mostly extracting and copying:

http://linux.die.net/man/2/ioprio_set

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

That shouldn't affect user-perceived perf much (unless we're hitting some of the badness sync disk IO in gecko), but it's a good idea to guard against degenerate cases.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Based on recent discussions this may not make be enabled, so can't block on that.

Comment 4

[Marshall Culpepper [:marshall_law]]

Renoming. Even if gecko/gaia updates aren't enabled, the extraction process for FOTA updates shouldn't cause the system to stutter..

Comment 5

[Andrew Overholt [:overholt]]

We discussed during triage that we probably can't block on stutter during (hopefully) infrequent updates.  Please re-nom if you disagree.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Tentatively agreed because while the experience is extremely bad, other OS processes are getting CPU time slices fairly reliably so the user can make progress.  (Unlike bug 813765 where the system completely freezes for seconds at a time.)

Suggest soft-blocking though.  The patch for this should be about 3 lines and is safe.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, don't know how the bb- was reset.

Comment 8

[Andrew Overholt [:overholt]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> Suggest soft-blocking though.

Sorry, missed this.  Setting it now.

Comment 10

[Dave Hylands [:dhylands]]

I'm going to assume that we only want to change the nice and not the oom_adj.

Changing oom_adj will make it more likely that the updater gets killed.
Changing nice make the updater play better with the other apps.

Comment 11

[Julien Wajsberg [:julienw]]

I think it was changing oom_adj to make the updater even less killable.

don't forget comment 1 if possible too :-)

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes, we should set oom_adj to -16.  The b2g process runs with oom_adj 0 which the updater most likely will inherit.

Be nicer about CPU but meaner about memory.

Comment 13

[Dave Hylands [:dhylands]]

OK - my current plan of action then is to add 4 prefs (ioprio needs both a class and a level), which nsUpdateDriver.cpp will use to set env vars which updater.cpp will detect and set.

Comment 14

[Dave Hylands [:dhylands]]

I could also combine the 4 prefs into a single env var.

Comment 15

[Dave Hylands [:dhylands]]

I'm going to propose the following 4 preference names:

app.update.updater.nice
app.update.updater.oom_score_adj
app.update.updater.ioprio.class
app.update.updater.ioprio.level

Comment 16

[Dave Hylands [:dhylands]]

Attachment: Make the updater run with reduced priority under B2G.

With this patch, I don't notice any UI degradation while the updater is applying in the background.

Comment 17

[Dave Hylands [:dhylands]]

https://tbpl.mozilla.org/?tree=Try&rev=78d7a7b8f981

Comment 18

[(no longer active)]

Comment on attachment 703071 [details] [diff] [review]
Make the updater run with reduced priority under B2G.

Review of attachment 703071 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the pref defaults issue either fixed or explained.

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2417,5 @@
> +    int32_t ioprioClass;
> +    int32_t ioprioLevel;
> +    if (sscanf(prioEnv, "%d/%d/%d/%d",
> +               &prioVal, &oomScoreAdj, &ioprioClass, &ioprioLevel) == 4) {
> +      LOG(("MOZ_UPDATER_PRIO=%s", prioEnv));

Please move this to before the if block and remove the needless else block below.

::: toolkit/xre/nsUpdateDriver.cpp
@@ +99,5 @@
>  #if defined(MOZ_WIDGET_GONK)
> +#include <linux/ioprio.h>
> +#include "mozilla/Preferences.h"
> +
> +using namespace mozilla;

Hiding this using namespace statement behind an #ifdef is just asking for trouble.  Please either move it outside of the #ifdef or fully quality Preferences further down.

@@ +112,5 @@
> +
> +static const int  kAppUpdaterPrioDefault        = 19;     // -20..19 where 19 = lowest priority
> +static const int  kAppUpdaterOomScoreAdjDefault = -1000;  // -1000 = Never kill
> +static const int  kAppUpdaterIOPrioClassDefault = IOPRIO_CLASS_IDLE;
> +static const int  kAppUpdaterIOPrioLevelDefault = 0;      // Doesn't matter for CLASS IDLE

I'm not sure why you're repeating the default values here...  With these, why do we need to set the defaults in prefs.js?

@@ +599,1 @@
>                          getter_AddRefs(osApplyToDir));

Nit: indentation.

@@ +836,5 @@
>    }
> +#if defined(MOZ_WIDGET_GONK)
> +  // We want the updater to be CPU friendly and not subject to being killed by
> +  // the low memory killer, so we pass in some preferences to allow it to
> +  // adjust it's priority.

Nit: its.  :-)

Comment 19

[Dave Hylands [:dhylands]]

(In reply to :Ehsan Akhgari from comment #18)
> Comment on attachment 703071 [details] [diff] [review]
> Make the updater run with reduced priority under B2G.
> 
> Review of attachment 703071 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the pref defaults issue either fixed or explained.
> 
> ::: toolkit/mozapps/update/updater/updater.cpp
> @@ +2417,5 @@
> > +    int32_t ioprioClass;
> > +    int32_t ioprioLevel;
> > +    if (sscanf(prioEnv, "%d/%d/%d/%d",
> > +               &prioVal, &oomScoreAdj, &ioprioClass, &ioprioLevel) == 4) {
> > +      LOG(("MOZ_UPDATER_PRIO=%s", prioEnv));
> 
> Please move this to before the if block and remove the needless else block
> below.

My intention was to have the log if the sscanf passed. The log in the else was added for debugging and I forgot to remove it.

> ::: toolkit/xre/nsUpdateDriver.cpp
> @@ +99,5 @@
> >  #if defined(MOZ_WIDGET_GONK)
> > +#include <linux/ioprio.h>
> > +#include "mozilla/Preferences.h"
> > +
> > +using namespace mozilla;
> 
> Hiding this using namespace statement behind an #ifdef is just asking for
> trouble.  Please either move it outside of the #ifdef or fully quality
> Preferences further down.

I'll pull out the #include and using from the #ifdef

> @@ +112,5 @@
> > +
> > +static const int  kAppUpdaterPrioDefault        = 19;     // -20..19 where 19 = lowest priority
> > +static const int  kAppUpdaterOomScoreAdjDefault = -1000;  // -1000 = Never kill
> > +static const int  kAppUpdaterIOPrioClassDefault = IOPRIO_CLASS_IDLE;
> > +static const int  kAppUpdaterIOPrioLevelDefault = 0;      // Doesn't matter for CLASS IDLE
> 
> I'm not sure why you're repeating the default values here...  With these,
> why do we need to set the defaults in prefs.js?

I'll remove the defaults from b2g.js. I added defaults here because I didn't want this code to rely on the prefs being present.

Thanks for the review.

Comment 20

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3313ce63be0

Comment 21

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/a3313ce63be0

Comment 22

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/a3313ce63be0

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1ea93193e006

Comment 24

[Ryan VanderMeulen [:RyanVM]]

And a follow-up bustage fix since DebugOnly.h doesn't exist on the b2g18 branch.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6a4087b8286e

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Landed on mozilla-b2g18 prior to the 1/25 branching to mozilla-b2g18_v1_0_0, updating status-b2g-v1.0.0