Updater needs to set oom_adj and nice appropriately

RESOLVED FIXED in Firefox 21

Status

()

P4
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: cjones, Assigned: dhylands)

Tracking

({dogfood})

unspecified
mozilla21
ARM
Gonk (Firefox OS)
dogfood
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, blocking-basecamp:-, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18+ fixed, b2g18-v1.0.0 fixed)

Details

(Whiteboard: [LOE:S] [Triaged:1/17])

Attachments

(1 attachment)

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.
Whiteboard: [LOE:S]
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
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.
Based on recent discussions this may not make be enabled, so can't block on that.
blocking-basecamp: + → ---
Renoming. Even if gecko/gaia updates aren't enabled, the extraction process for FOTA updates shouldn't cause the system to stutter..
blocking-basecamp: --- → ?
We discussed during triage that we probably can't block on stutter during (hopefully) infrequent updates.  Please re-nom if you disagree.
blocking-basecamp: ? → -
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.
blocking-basecamp: - → ?
Priority: -- → P4
Sorry, don't know how the bb- was reset.
blocking-basecamp: ? → -
(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> Suggest soft-blocking though.

Sorry, missed this.  Setting it now.
tracking-b2g18: --- → +
(Assignee)

Updated

6 years ago
Assignee: nobody → dhylands
(Assignee)

Updated

6 years ago
Duplicate of this bug: 821129
(Assignee)

Comment 10

6 years ago
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.
I think it was changing oom_adj to make the updater even less killable.

don't forget comment 1 if possible too :-)
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.
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Comment 14

6 years ago
I could also combine the 4 prefs into a single env var.
(Assignee)

Comment 15

6 years ago
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
Blocks: 828731

Updated

6 years ago
blocking-b2g: --- → tef?

Updated

6 years ago
blocking-b2g: tef? → tef+
(Assignee)

Comment 16

6 years ago
Created attachment 703071 [details] [diff] [review]
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.
Attachment #703071 - Flags: review?(robert.bugzilla)
Whiteboard: [LOE:S] → [LOE:S] [Triaged:1/17]

Comment 18

6 years ago
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.  :-)
Attachment #703071 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 19

6 years ago
(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.
(Assignee)

Updated

6 years ago
Blocks: 833063

Comment 21

6 years ago
https://hg.mozilla.org/mozilla-central/rev/a3313ce63be0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1ea93193e006
status-b2g18: --- → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
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
Landed on mozilla-b2g18 prior to the 1/25 branching to mozilla-b2g18_v1_0_0, updating status-b2g-v1.0.0
status-b2g18-v1.0.0: --- → fixed
You need to log in before you can comment on or make changes to this bug.