[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