785171 - Update GonkHal.cpp to use oom_score_adj rather than oom_adj

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Dave Hylands [:dhylands]]

/proc/<pid>/oom_adj was deprecated in the 2.6.37 kernel and is scheduled to be removed entirely aroudn the August 2012 timeframe (so around kernel version 3.5).

We should update GonkHal.cpp to use the newer oom_score_adj (introduced in 2.6.37) instead.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Proposed patch for supporting /proc/<pid>/oom_score_adj

Comment 2

[Dave Hylands [:dhylands]]

I can review this for you.

I see you've done 3 separate patches in the same attachment. You should split them into 3 separate attachments so that Splinter Review works properly.

Having the whitespace changes in a separate patch is a plus.

It's also customary to use -U8 for patches. You may want to look at git://github.com/jlebar/moz-git-tools.git for git bz which is one way to associate patches with bugzilla bugs.

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Dave Hylands [:dhylands] from comment #2)

Sure, thanks! I thought about asking you in the first place, but then choose to look for a reviewer in the recent commit history of that file.

I'll rework the patch set according to your suggestions and submit it again.

Comment 4

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

Comment on attachment 660640 [details] [diff] [review]
Proposed patch for supporting /proc/<pid>/oom_score_adj

Sounds great, dhylands.

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: 01 - Whitespace fixes

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: 02 - Return success from WriteToFile

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: 03 - Support /proc/<pid>/oom_score_adj

Comment 8

[Dave Hylands [:dhylands]]

Comment on attachment 660922 [details] [diff] [review]
01 - Whitespace fixes

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

This doesn't look like white space changes. The patch looks like its adding the entire file.

Comment 9

[Dave Hylands [:dhylands]]

Comment on attachment 660924 [details] [diff] [review]
02 - Return success from WriteToFile

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

Looks good.

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Dave Hylands [:dhylands] from comment #8)
> Comment on attachment 660922 [details] [diff] [review]
> 01 - Whitespace fixes
> 
> Review of attachment 660922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't look like white space changes. The patch looks like its adding
> the entire file.

This just made me realize that I attached the wrong set of patches. m( I'll add the correct ones in a minute. Sorry.

Comment 11

[Dave Hylands [:dhylands]]

Comment on attachment 660925 [details] [diff] [review]
03 - Support /proc/<pid>/oom_score_adj

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

::: hal/gonk/GonkHal.cpp
@@ +828,5 @@
>    return true;
>  }
>  
> +static int
> +oomScoreAdj(int oomAdj)

nit: I see that this file has a mishmash of coding styles, but we might as well get you started in the right direction. Gecko code uses the "a" prefix to mean argument, so for new stuff, and since this file does it both ways, I would use aOomAdj.

@@ +833,5 @@
> +{
> +  // Convert the value of the OOM adjustment from the domain of
> +  // /proc/<pid>/oom_adj [-17,15] to the domain of
> +  // /proc/<pid>/oom_adj_score [-1000,1000]. The special value of
> +  // never-OOM kill is converted to to 1000.

Actually, the never-OOM is supposed to be -1000 (see linux/oom.h)

Since it's available, you should #include <linux/oom.h>. You'll find the userspace version of the header over here:

./out/target/product/otoro/obj/KERNEL_OBJ/usr/include/linux/oom.h

So I think that if you just do #include <linux/oom.h> you'll get that header file. Since OOM_SCORE_ADJ_xxx will only exist on newer kernels and OOM_ADJUST_xxx will go away, you should probably define your own local ones if the ones you want don't exist.

So use the constants and not magic numbers.

@@ +836,5 @@
> +  // /proc/<pid>/oom_adj_score [-1000,1000]. The special value of
> +  // never-OOM kill is converted to to 1000.
> +
> +  if (oomAdj <= -17) {
> +    return 1000; // never OOM kill

This should be OOM_SCORE_ADJ_MIN (-1000).

You can verify this by doing:

echo -1000 > /proc/self/oom_score_adj
cat /proc/self/oom_adj

or

echo -17 > /proc/self/oom_adj
cat /proc/self/oom_score_adj

@@ +837,5 @@
> +  // never-OOM kill is converted to to 1000.
> +
> +  if (oomAdj <= -17) {
> +    return 1000; // never OOM kill
> +  } else if (oomAdj < 0) {

nit: The else isn't required since you're returning above

@@ +869,5 @@
>    // hal.processPriorityManager{foreground,background,master}{OomAdjust,Nice}.
>  
>    int32_t oomAdj = 0;
>    nsresult rv = Preferences::GetInt(nsPrintfCString(
>      "hal.processPriorityManager.gonk.%sOomAdjust", priorityStr).get(), &oomAdj);

Since the oom_adj stuff is going to be deprecated, we should probably change our preferences to be based on the oom_score_adj ranges and look for: 

hal.processPriorityManager.gonk.%sOomScoreAdjust

and if that doesn't exist, look for OomAdjust.

and also fix the defaults to use the new ranges.

You'll find the defaults over here:
https://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#516

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks. BTW, shouldn't there be a test for the validity of the configured values?

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Dave Hylands [:dhylands] from comment #11)
 
> Since the oom_adj stuff is going to be deprecated, we should probably change
> our preferences to be based on the oom_score_adj ranges and look for: 
> 
> hal.processPriorityManager.gonk.%sOomScoreAdjust
> 
> and if that doesn't exist, look for OomAdjust.

IMHO we should not change a user interface without necessity.

- It's just cumbersome for users.
- If both values disagree, we might have a config that does different things on different systems.
- The old interface covers all cases.

Comment 14

[Dave Hylands [:dhylands]]

(In reply to Thomas Zimmermann from comment #12)
> Thanks. BTW, shouldn't there be a test for the validity of the configured
> values?

That seems like a reasonable thing to do. The kernel will just ignore out of range requests:
http://lxr.linux.no/#linux+v3.0.8/fs/proc/base.c#L1082
http://lxr.linux.no/#linux+v3.0.8/fs/proc/base.c#L1190

So you could also clamp the value within an acceptable range.

Comment 15

[Dave Hylands [:dhylands]]

(In reply to Thomas Zimmermann from comment #13)
> (In reply to Dave Hylands [:dhylands] from comment #11)
>  
> > Since the oom_adj stuff is going to be deprecated, we should probably change
> > our preferences to be based on the oom_score_adj ranges and look for: 
> > 
> > hal.processPriorityManager.gonk.%sOomScoreAdjust
> > 
> > and if that doesn't exist, look for OomAdjust.
> 
> IMHO we should not change a user interface without necessity.
> 
> - It's just cumbersome for users.
> - If both values disagree, we might have a config that does different things
> on different systems.
> - The old interface covers all cases.

This isn't something that's exposed to users. Since we haven't done a release yet, I'd change it now, and then we don't need to worry about it.

I also think it would be fine to just use OomScoreAdjust and drop OomAdjust altogether.

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Whitespace cleanups

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Return success from WriteToFile

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Adopt semantics of /proc/<pid>/oom_score_adjust in preferences

Comment 19

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Adopt semantics of /proc/<pid>/oom_score_adjust in preferences

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Clamp OOM adjustment settings to valid range

Comment 21

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Support OOM adjustment via /proc/<pid>/oom_score_adj

Comment 22

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Some comments regarding the third version of the patch set:

- Bionic does not include <linux/oom.h> and file in out/target... is not within the include search paths of the C preprocessor. I redefined the OOM constants within GonkHal.cpp.

- As suggested, I replaced the old preference strings by the new ones.

Comment 23

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Whitespace cleanups

Comment 24

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Return success from WriteToFile

Comment 25

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Adopt semantics of /proc/<pid>/oom_score_adjust in preferences

Comment 26

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Clamp OOM adjustment settings to valid range

Comment 27

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Support OOM adjustment via /proc/<pid>/oom_score_adj

Comment 28

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Ok, I checked this twice and these _are_ now the patches for the third round of the review.

Comment 29

[Dave Hylands [:dhylands]]

Comment on attachment 661071 [details] [diff] [review]
Adopt semantics of /proc/<pid>/oom_score_adjust in preferences

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

I'm not sure why it doesn't work, but splinter review doesn't work properly with this patch.

The only thing I noticed is that oomAdjOfOomScoreAdj wasn't declared static. Otherwise, it looks good.

After reading the rest of the patches, it looks like oomAdjOfOomScoreAdj should be removed since it's no longer used, so don't bother making it static here.

Comment 30

[Dave Hylands [:dhylands]]

Comment on attachment 661074 [details] [diff] [review]
Support OOM adjustment via /proc/<pid>/oom_score_adj

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

nit: You should remove oomAdjOfOomScoreAdj since it's no longer needed.

r=me with this change.

Personally, I would probably have done this with 2 patches, one for the white space change and one for the rest, but there is no need to change the way it is.

Comment 31

[Dave Hylands [:dhylands]]

Bah - I see oomAdjOfOomScoreAdj is still used, so it should be declared static (so ignore my comment about removing it)

Comment 32

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Adopt semantics of /proc/<pid>/oom_score_adjust in preferences

The only difference to the previous patch is the addition of the missing static keyword. Dave, please commit the patch set if you're ok with it.

Comment 33

[Dave Hylands [:dhylands]]

Submitted to try:
https://tbpl.mozilla.org/?tree=Try&rev=0a15614fda19

Comment 34

[Dave Hylands [:dhylands]]

Try looks good. Pushing to mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/4de50fce24ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e5fe9ad7950
https://hg.mozilla.org/integration/mozilla-inbound/rev/f746375dc95c
https://hg.mozilla.org/integration/mozilla-inbound/rev/54f940cf6d65
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4ce18b32241

Comment 35

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks! :)

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4de50fce24ea
https://hg.mozilla.org/mozilla-central/rev/8e5fe9ad7950
https://hg.mozilla.org/mozilla-central/rev/f746375dc95c
https://hg.mozilla.org/mozilla-central/rev/54f940cf6d65
https://hg.mozilla.org/mozilla-central/rev/b4ce18b32241

Comment 37

[Justin Lebar (not reading bugmail)]

FWIW, we didn't make this change originally because mvines explained that the old interface wasn't going away in Android 4.1.