[RFC][PATCH] Add support for oom_score_adj

Scott James Remnant scott at netsplit.com
Tue Mar 15 00:28:47 UTC 2011


On Sun, Mar 13, 2011 at 1:10 PM, Marc - A. Dahlhaus <mad at wol.de> wrote:

> Am 13.03.2011 19:56, schrieb Scott James Remnant:
>
>> Thanks for the patch, a few comments inline below...
>>
>> On Sun, Mar 13, 2011 at 11:29 AM, Marc - A. Dahlhaus <mad at wol.de> wrote:
>>> --- a/init/job_class.h
>>> +++ b/init/job_class.h
>>> @@ -94,6 +94,7 @@ typedef enum console_type {
>>>  * @umask: file mode creation mask,
>>>  * @nice: process priority,
>>>  * @oom_adj: OOM killer adjustment,
>>> + * @oom_score_adj: OOM killer score adjustment,
>>>  * @limits: resource limits indexed by resource,
>>>  * @chroot: root directory of process (implies @chdir if not set),
>>>  * @chdir: working directory of process,
>>> @@ -142,6 +143,7 @@ typedef struct job_class {
>>>        mode_t          umask;
>>>        int             nice;
>>>        int             oom_adj;
>>> +       int             oom_score_adj;
>>>        struct rlimit  *limits[RLIMIT_NLIMITS];
>>>        char           *chroot;
>>>        char           *chdir;
>>
>> Why keep both oom_adj and oom_score_adj in the struct? If you only
>> kept the oom_score_adj in the struct, you could multiple the oom_adj
>> values to it, and divide later.
>
> with both values in the struct it was possible to add exact values for each interface by specifying
>
> oom value
> oom score value
>
> in a conf file.
>
> Also this way the calculation of values is in the configuration parsing only which looked cleaner IMO.
>
I don't see why this is cleaner?  It adds two members to the struct
which have the same meaning, and with a non-standard method of
overriding - "oom score" always overrides "oom" which isn't consistent
with "last one wins"

e.g. if a config has

  oom score -150

and you do

  echo oom never >> /etc/init/foo.override

The score is still -150, the new value doesn't take effect.


Having "oom" simply rescale the -17...+15 value would fix this.

>>> --- a/init/job_process.c
>>> +++ b/init/job_process.c
>>> @@ -487,14 +487,20 @@ job_process_spawn (JobClass     *class,
>>>                fd = fopen (filename, "w");
>>>                if (! fd) {
>>> +                       snprintf (filename, sizeof (filename),
>>> +                                 "/proc/%d/oom_adj", getpid ());
>>> +                       oom_value = class->oom_adj;
>>> +                       fd = fopen (filename, "w");
>>> +               }
>>> +               if (! fd) {
>>>
>> There's no errno checking here - the file could fail to open for any
>> number of reasons, it should only fall back to the old interface in
>> the one reason that the new interface doesn't exist.
>
> if (errno == EACCES) {
> instead of the first
> if (! fd) {
> should be ok, right?
>
if ((! fd) && (errno == EACCES)) I guess

Scott



More information about the upstart-devel mailing list