If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Update GonkHal.cpp to use oom_score_adj rather than oom_adj

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dhylands, Assigned: tzimmermann)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 11 obsolete attachments)

5.28 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
1.40 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
1.57 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
1.70 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
4.67 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
/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.
Assignee: nobody → tdz
Created attachment 660640 [details] [diff] [review]
Proposed patch for supporting /proc/<pid>/oom_score_adj
Attachment #660640 - Flags: review?(jones.chris.g)
Status: NEW → ASSIGNED
(Reporter)

Comment 2

5 years ago
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.
(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 on attachment 660640 [details] [diff] [review]
Proposed patch for supporting /proc/<pid>/oom_score_adj

Sounds great, dhylands.
Attachment #660640 - Flags: review?(jones.chris.g) → review?(dhylands)
Created attachment 660922 [details] [diff] [review]
01 - Whitespace fixes
Attachment #660640 - Attachment is obsolete: true
Attachment #660640 - Flags: review?(dhylands)
Attachment #660922 - Flags: review?(dhylands)
Created attachment 660924 [details] [diff] [review]
02 - Return success from WriteToFile
Attachment #660924 - Flags: review?(dhylands)
Created attachment 660925 [details] [diff] [review]
03 - Support /proc/<pid>/oom_score_adj
Attachment #660925 - Flags: review?(dhylands)
(Reporter)

Comment 8

5 years ago
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.
Attachment #660922 - Flags: review?(dhylands) → review-
(Reporter)

Comment 9

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

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

Looks good.
Attachment #660924 - Flags: review?(dhylands) → review+
(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.
(Reporter)

Comment 11

5 years ago
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
Attachment #660925 - Flags: review?(dhylands) → review-
Thanks. BTW, shouldn't there be a test for the validity of the configured values?
(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.
(Reporter)

Comment 14

5 years ago
(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.
(Reporter)

Comment 15

5 years ago
(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.
Created attachment 661059 [details] [diff] [review]
Whitespace cleanups
Attachment #660922 - Attachment is obsolete: true
Attachment #661059 - Flags: review?(dhylands)
Created attachment 661061 [details] [diff] [review]
Return success from WriteToFile
Attachment #660924 - Attachment is obsolete: true
Attachment #661061 - Flags: review?(dhylands)
Created attachment 661062 [details] [diff] [review]
Adopt semantics of /proc/<pid>/oom_score_adjust in preferences
Attachment #660925 - Attachment is obsolete: true
Attachment #661062 - Flags: review?
Attachment #661062 - Flags: review? → review?(dhylands)
Created attachment 661064 [details] [diff] [review]
Adopt semantics of /proc/<pid>/oom_score_adjust in preferences
Attachment #661062 - Attachment is obsolete: true
Attachment #661062 - Flags: review?(dhylands)
Attachment #661064 - Flags: review?(dhylands)
Created attachment 661065 [details] [diff] [review]
Clamp OOM adjustment settings to valid range
Attachment #661065 - Flags: review?(dhylands)
Created attachment 661066 [details] [diff] [review]
Support OOM adjustment via /proc/<pid>/oom_score_adj
Attachment #661066 - Flags: review?(dhylands)
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.
Created attachment 661069 [details] [diff] [review]
Whitespace cleanups
Attachment #661059 - Attachment is obsolete: true
Attachment #661059 - Flags: review?(dhylands)
Attachment #661069 - Flags: review?(dhylands)
Created attachment 661070 [details] [diff] [review]
Return success from WriteToFile
Attachment #661061 - Attachment is obsolete: true
Attachment #661061 - Flags: review?(dhylands)
Attachment #661070 - Flags: review?(dhylands)
Created attachment 661071 [details] [diff] [review]
Adopt semantics of /proc/<pid>/oom_score_adjust in preferences
Attachment #661064 - Attachment is obsolete: true
Attachment #661064 - Flags: review?(dhylands)
Attachment #661071 - Flags: review?(dhylands)
Created attachment 661073 [details] [diff] [review]
Clamp OOM adjustment settings to valid range
Attachment #661065 - Attachment is obsolete: true
Attachment #661065 - Flags: review?(dhylands)
Attachment #661073 - Flags: review?(dhylands)
Created attachment 661074 [details] [diff] [review]
Support OOM adjustment via /proc/<pid>/oom_score_adj
Attachment #661066 - Attachment is obsolete: true
Attachment #661066 - Flags: review?(dhylands)
Attachment #661074 - Flags: review?(dhylands)
Attachment #661071 - Attachment description: Adapt semantics of /proc/<pid>/oom_score_adjust in preferences → Adopt semantics of /proc/<pid>/oom_score_adjust in preferences
Ok, I checked this twice and these _are_ now the patches for the third round of the review.
(Reporter)

Updated

5 years ago
Attachment #661069 - Flags: review?(dhylands) → review+
(Reporter)

Updated

5 years ago
Attachment #661070 - Flags: review?(dhylands) → review+
(Reporter)

Comment 29

5 years ago
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.
Attachment #661071 - Flags: review?(dhylands) → review+
(Reporter)

Updated

5 years ago
Attachment #661073 - Flags: review?(dhylands) → review+
(Reporter)

Comment 30

5 years ago
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.
Attachment #661074 - Flags: review?(dhylands) → review+
(Reporter)

Comment 31

5 years ago
Bah - I see oomAdjOfOomScoreAdj is still used, so it should be declared static (so ignore my comment about removing it)
Created attachment 661255 [details] [diff] [review]
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.
Attachment #661071 - Attachment is obsolete: true
Attachment #661255 - Flags: review?(dhylands)
(Reporter)

Updated

5 years ago
Attachment #661255 - Flags: review?(dhylands) → review+
(Reporter)

Comment 33

5 years ago
Submitted to try:
https://tbpl.mozilla.org/?tree=Try&rev=0a15614fda19
(Reporter)

Comment 34

5 years ago
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
Thanks! :)
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
FWIW, we didn't make this change originally because mvines explained that the old interface wasn't going away in Android 4.1.
Depends on: 768832
You need to log in before you can comment on or make changes to this bug.