914541 - When OOM the keyboard App should have lower priority than the foreground App

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

I think the role=keyboard app should have lower priority than the foreground app that triggers it but higher priority than the rest of other apps. So we try to keep the keyboard open as hard as possible but don't let it kill the foreground app.

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

Controlled by dom/ipc/ProcessPriorityManager.cpp

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

Hmm.. should we reuse the BACKGROUND_PERCEIVABLE class or create a new BACKGROUND_KEYBOARD (or FOREGROUND?)

Probably FOREGROUND_KEYBOARD when the keyboard is activated and BACKGROUND when it hides.

Comment 3

[Alan Huang]

1. We can only WRITE 6 pairs of KillUnderMB/OomScoreAdjust to sys/module/lowmemorykiller/parameters/. We are currently running out of these pairs.

Since OOM_ADJUST_MAX = 15 (or OOM_SCORE_ADJ_MAX = 1000 in later kernel),
We can enlarge these pairs' OomScoreAdjust. But in my experience, I suggest we keep hal.processPriorityManager.gonk.BACKGROUND.OomScoreAdjust no larger than 400+267. We still need spaces for background LRU, which are bug 822325 and bug 913893. I will open another bug for enlarging OomScoreAdjusts later.

2. We can still define more LMK levels, write to each process but not write to kernel config by GonkHal. 

Take Unagi as an example, we now write 0,1,2,3,4,6 to /sys/module/lowmemorykiller/parameters/adj. We cannot write 0,1,2,3,4,5,6 to it, but we can still set 5 to a process' /proc/[pid]/oom_adj.

If we enlarge the series 0,1,2,3,4,6 to 0,1,2,6,8,10 or something else, we can have more benefit.

Comment 4

[Alan Huang]

Attachment: an example to add new ProcessPriority

Hi Kan-ru,

If you need to add new ProcessPriority, the attachment is for what we should do now. I think you may need to wait after bug 914728 land.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Alan Huang [:ahuang] from comment #4)
> Created attachment 802824 [details] [diff] [review]
> an example to add new ProcessPriority
> 
> Hi Kan-ru,
> 
> If you need to add new ProcessPriority, the attachment is for what we should
> do now. I think you may need to wait after bug 914728 land.

Thanks, Alan. This example is really helpful.

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP check role=keyboard for initial priority

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Assign higher priority for mozapptype=inputmethod processes

keyboard_manager.js will have to set mozapptype=keyboard for the keyboard iframe.

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 809074 [details] [diff] [review]
Assign higher priority for mozapptype=inputmethod processes

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

::: b2g/app/b2g.js
@@ +624,5 @@
>  pref("hal.processPriorityManager.gonk.FOREGROUND.KillUnderMB", 6);
>  pref("hal.processPriorityManager.gonk.FOREGROUND.Nice", 1);
>  
> +pref("hal.processPriorityManager.gonk.FOREGROUND_KEYBOARD.OomScoreAdjust", 200);
> +pref("hal.processPriorityManager.gonk.FOREGROUND_KEYBOARD.Nice", 1);

do we have a default value for the KillUnderMB pref?

Comment 9

[Alan Huang]

(In reply to Fabrice Desré [:fabrice] from comment #8)
> > +pref("hal.processPriorityManager.gonk.FOREGROUND_KEYBOARD.OomScoreAdjust", 200);
> > +pref("hal.processPriorityManager.gonk.FOREGROUND_KEYBOARD.Nice", 1);
> 
> do we have a default value for the KillUnderMB pref?
No, we don't and we cannot assign any more KillUnderMBs. The android kernel can only accept 6 (OomScoreAdjust, KillUnderMB) pairs. 

But it is okay, kernel will still kill processes with larger OomScoreAdjust first, even its OomScoreAdjust don't have corresponding KillUnderMB. The (OomScoreAdjust, KillUnderMB) pairs are raising critiria for LMK.

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Alan Huang [:ahuang] from comment #9)
> (In reply to Fabrice Desré [:fabrice] from comment #8)
> > > +pref("hal.processPriorityManager.gonk.FOREGROUND_KEYBOARD.OomScoreAdjust", 200);
> > > +pref("hal.processPriorityManager.gonk.FOREGROUND_KEYBOARD.Nice", 1);
> > 
> > do we have a default value for the KillUnderMB pref?
> No, we don't and we cannot assign any more KillUnderMBs. The android kernel
> can only accept 6 (OomScoreAdjust, KillUnderMB) pairs. 
> 
> But it is okay, kernel will still kill processes with larger OomScoreAdjust
> first, even its OomScoreAdjust don't have corresponding KillUnderMB. The
> (OomScoreAdjust, KillUnderMB) pairs are raising critiria for LMK.

Thanks for explanation. I'll add them to the comments in b2g.js

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/b2g-inbound/rev/b8f0eace02bc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ivan Tsay (:ITsay)]

This is the blocker for OOP which is needed for new keyboard framework. So nominate it to koi+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/00c23047048a