Looks good to me on a first pass <div class="gmail_quote">On Mon, May 2, 2011 at 2:05 PM, Marc - A. Dahlhaus <<a href="mailto:mad@wol.de">mad@wol.de</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> <div class="im">Am 17.03.2011 21:48, schrieb Scott James Remnant: </div><div><div></div><div class="h5">> (Replying to the new patch separately) > > On Thu, Mar 17, 2011 at 11:58 AM, Marc - A. Dahlhaus <<a href="mailto:mad@wol.de">mad@wol.de</a>> wrote: > >> The never case was no problem as i set booth values on never. >> But i get your point. Rewrote it per your comments. Result attached. >> > This looks perfect, could you add some tests for "oom score" to the > parsing code, I see you've already updated the existing ones. It also > needs a man page update (I suggest just dropping "oom NN" out of the > man page and replacing it with "score") > > Then it looks ready to apply! > > Thankyou, > Scott </div></div>This should be it... Running for more weeks without any problem here. 3 Test-Boxes (x86_64 and i686) did several boots, shutdowns and reboots no problems detected. thanks, Marc <div class="im"> --- a/init/errors.h +++ b/init/errors.h @@ -62,7 +62,8 @@ enum { #define PARSE_ILLEGAL_EXIT_STR N_("Illegal exit status, expected integer") #define PARSE_ILLEGAL_UMASK_STR N_("Illegal file creation mask, expected octal integer") #define PARSE_ILLEGAL_NICE_STR N_("Illegal nice value, expected -20 to 19") -#define PARSE_ILLEGAL_OOM_STR N_("Illegal oom adjustment, expected -16 to 15 or never") +#define PARSE_ILLEGAL_OOM_STR N_("Illegal oom adjustment, expected -16 to 15 or 'never'") +#define PARSE_ILLEGAL_OOM_SCORE_STR N_("Illegal oom score adjustment, expected -999 to 1000 or 'never'") #define PARSE_ILLEGAL_LIMIT_STR N_("Illegal limit, expected 'unlimited' or integer") #define PARSE_EXPECTED_EVENT_STR N_("Expected event") #define PARSE_EXPECTED_OPERATOR_STR N_("Expected operator") --- a/init/job_class.c +++ b/init/job_class.c </div>@@ -211,7 +211,7 @@ job_class_new (const void *parent, <div class="im"> class->umask = JOB_DEFAULT_UMASK; class->nice = 0; - class->oom_adj = 0; + class->oom_score_adj = 0; for (i = 0; i < RLIMIT_NLIMITS; i++) class->limits[i] = NULL; </div><div class="im">--- a/init/job_class.h +++ b/init/job_class.h </div>@@ -93,7 +93,7 @@ typedef enum console_type { * @console: how to arrange processes' stdin/out/err file descriptors, <div class="im"> * @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, </div>@@ -141,7 +141,7 @@ typedef struct job_class { <div class="im"> mode_t umask; int nice; - int oom_adj; + int oom_score_adj; struct rlimit *limits[RLIMIT_NLIMITS]; char *chroot; char *chdir; </div>@@ -150,6 +150,8 @@ typedef struct job_class { int debug; } JobClass; +#define SCORE_TO_ADJ(x) ((x * ((x < 0) ? 17 : 15)) / 1000) +#define ADJ_TO_SCORE(x) ((x * 1000) / ((x < 0) ? 17 : 15)) NIH_BEGIN_EXTERN <div class="im"> --- a/init/job_process.c +++ b/init/job_process.c </div>@@ -371,7 +371,7 @@ job_process_spawn (JobClass *class, <div class="im"> { sigset_t child_set, orig_set; pid_t pid; - int i, fds[2]; + int i, fds[2], oom_value; char filename[PATH_MAX]; FILE *fd; </div>@@ -493,16 +493,22 @@ job_process_spawn (JobClass *class, /* Adjust the process OOM killer priority. */ - if (class->oom_adj) { + if (class->oom_score_adj) { <div class="im"> snprintf (filename, sizeof (filename), - "/proc/%d/oom_adj", getpid ()); </div>- + "/proc/%d/oom_score_adj", getpid ()); + oom_value = class->oom_score_adj; fd = fopen (filename, "w"); + if ((! fd) && (errno == EACCES)) { + snprintf (filename, sizeof (filename), <div class="im">+ "/proc/%d/oom_adj", getpid ()); </div>+ oom_value = SCORE_TO_ADJ(class->oom_score_adj); <div class="im">+ fd = fopen (filename, "w"); + } if (! fd) { </div><div class="im"> nih_error_raise_system (); job_process_error_abort (fds[1], JOB_PROCESS_ERROR_OOM_ADJ, 0); } else { - fprintf (fd, "%d\n", class->oom_adj); + fprintf (fd, "%d\n", oom_value); if (fclose (fd)) { nih_error_raise_system (); </div>--- a/init/man/init.5 +++ b/init/man/init.5 @@ -500,15 +500,15 @@ see for more details. .\" .TP -.B oom \fIADJUSTMENT\fR|\fBnever +.B oom score \fIADJUSTMENT\fR|\fBnever Normally the OOM killer regards all processes equally, this stanza advises the kernel to treat this job differently. .I ADJUSTMENT may be an integer value from -.I -16 +.I -999 (very unlikely to be killed by the OOM killer) up to -.I 14 +.I 1000 (very likely to be killed by the OOM killer). It may also be the special value .B never <div class="im">--- a/init/parse_job.c +++ b/init/parse_job.c </div>@@ -2233,6 +2233,7 @@ stanza_oom (JobClass *class, nih_local char *arg = NULL; char *endptr; size_t a_pos, a_lineno; + int oom_adj; int ret = -1; nih_assert (class != NULL); @@ -2247,12 +2248,37 @@ stanza_oom (JobClass *class, if (! arg) goto finish; - if (! strcmp (arg, "never")) { - class->oom_adj = -17; <div class="im">+ if (! strcmp (arg, "score")) { + nih_local char *scorearg = NULL; + </div>+ /* Update error position to the score value */ + *pos = a_pos; + if (lineno) + *lineno = a_lineno; <div class="im">+ + scorearg = nih_config_next_arg (NULL, file, len, + &a_pos, &a_lineno); + if (! scorearg) + goto finish; + + if (! strcmp (scorearg, "never")) { + class->oom_score_adj = -1000; </div><div class="im">+ } else { + errno = 0; + class->oom_score_adj = (int)strtol (scorearg, &endptr, 10); </div><div class="im">+ if (errno || *endptr || + (class->oom_score_adj < -1000) || + (class->oom_score_adj > 1000)) + nih_return_error (-1, PARSE_ILLEGAL_OOM, + _(PARSE_ILLEGAL_OOM_SCORE_STR)); + } </div>+ } else if (! strcmp (arg, "never")) { <div class="im">+ class->oom_score_adj = -1000; } else { errno = 0; - class->oom_adj = (int)strtol (arg, &endptr, 10); - if (errno || *endptr || (class->oom_adj < -17) || (class->oom_adj > 15)) </div>+ oom_adj = (int)strtol (arg, &endptr, 10); + class->oom_score_adj = ADJ_TO_SCORE(oom_adj); + if (errno || *endptr || (oom_adj < -17) || (oom_adj > 15)) <div class="im"> nih_return_error (-1, PARSE_ILLEGAL_OOM, _(PARSE_ILLEGAL_OOM_STR)); } </div>--- a/init/tests/test_job_class.c +++ b/init/tests/test_job_class.c @@ -133,7 +133,7 @@ test_new (void) TEST_EQ (class->umask, 022); TEST_EQ (class->nice, 0); - TEST_EQ (class->oom_adj, 0); + TEST_EQ (class->oom_score_adj, 0); <div class="im"> for (i = 0; i < RLIMIT_NLIMITS; i++) </div> TEST_EQ_P (class->limits[i], NULL); --- a/init/tests/test_parse_job.c +++ b/init/tests/test_parse_job.c @@ -6198,11 +6198,39 @@ test_stanza_oom (void) TEST_ALLOC_SIZE (job, sizeof (JobClass)); - TEST_EQ (job->oom_adj, 10); + TEST_EQ (job->oom_score_adj, ADJ_TO_SCORE(10)); nih_free (job); } + TEST_FEATURE ("with positive score argument"); + strcpy (buf, "oom score 100\n"); + + TEST_ALLOC_FAIL { + pos = 0; + lineno = 1; + job = parse_job (NULL, "test", buf, strlen (buf), + &pos, &lineno); + + if (test_alloc_failed) { + TEST_EQ_P (job, NULL); + + err = nih_error_get (); + TEST_EQ (err->number, ENOMEM); + nih_free (err); + + continue; + } + + TEST_EQ (pos, strlen (buf)); + TEST_EQ (lineno, 2); + + TEST_ALLOC_SIZE (job, sizeof (JobClass)); + + TEST_EQ (job->oom_score_adj, 100); + + nih_free (job); + } /* Check that an oom stanza with a negative timeout results * in it being stored in the job. @@ -6231,7 +6259,36 @@ test_stanza_oom (void) TEST_ALLOC_SIZE (job, sizeof (JobClass)); - TEST_EQ (job->oom_adj, -10); + TEST_EQ (job->oom_score_adj, ADJ_TO_SCORE(-10)); + + nih_free (job); + } + + TEST_FEATURE ("with negative score argument"); + strcpy (buf, "oom score -100\n"); + + TEST_ALLOC_FAIL { + pos = 0; + lineno = 1; + job = parse_job (NULL, "test", buf, strlen (buf), + &pos, &lineno); + + if (test_alloc_failed) { + TEST_EQ_P (job, NULL); + + err = nih_error_get (); + TEST_EQ (err->number, ENOMEM); + nih_free (err); + + continue; + } + + TEST_EQ (pos, strlen (buf)); + TEST_EQ (lineno, 2); + + TEST_ALLOC_SIZE (job, sizeof (JobClass)); + + TEST_EQ (job->oom_score_adj, -100); nih_free (job); } @@ -6264,7 +6321,40 @@ test_stanza_oom (void) TEST_ALLOC_SIZE (job, sizeof (JobClass)); - TEST_EQ (job->oom_adj, -17); + TEST_EQ (job->oom_score_adj, ADJ_TO_SCORE(-17)); + + nih_free (job); + } + + + /* Check that an oom score stanza may have the special never + * argument which stores -1000 in the job. + */ + TEST_FEATURE ("with never score argument"); + strcpy (buf, "oom score never\n"); + + TEST_ALLOC_FAIL { + pos = 0; + lineno = 1; + job = parse_job (NULL, "test", buf, strlen (buf), + &pos, &lineno); + + if (test_alloc_failed) { + TEST_EQ_P (job, NULL); + + err = nih_error_get (); + TEST_EQ (err->number, ENOMEM); + nih_free (err); + + continue; + } + + TEST_EQ (pos, strlen (buf)); + TEST_EQ (lineno, 2); + + TEST_ALLOC_SIZE (job, sizeof (JobClass)); + + TEST_EQ (job->oom_score_adj, -1000); nih_free (job); } @@ -6297,7 +6387,100 @@ test_stanza_oom (void) TEST_ALLOC_SIZE (job, sizeof (JobClass)); - TEST_EQ (job->oom_adj, 10); + TEST_EQ (job->oom_score_adj, ADJ_TO_SCORE(10)); + + nih_free (job); + } + + TEST_FEATURE ("with multiple score stanzas"); + strcpy (buf, "oom score -500\n"); + strcat (buf, "oom score 500\n"); + + TEST_ALLOC_FAIL { + pos = 0; + lineno = 1; + job = parse_job (NULL, "test", buf, strlen (buf), + &pos, &lineno); + + if (test_alloc_failed) { + TEST_EQ_P (job, NULL); + + err = nih_error_get (); + TEST_EQ (err->number, ENOMEM); + nih_free (err); + + continue; + } + + TEST_EQ (pos, strlen (buf)); + TEST_EQ (lineno, 3); + + TEST_ALLOC_SIZE (job, sizeof (JobClass)); + + TEST_EQ (job->oom_score_adj, 500); + + nih_free (job); + } + + /* Check that the last of multiple distinct oom stanzas is + * used. + */ + TEST_FEATURE ("with an oom overriding an oom score stanza"); + strcpy (buf, "oom score -10\n"); + strcat (buf, "oom 10\n"); + + TEST_ALLOC_FAIL { + pos = 0; + lineno = 1; + job = parse_job (NULL, "test", buf, strlen (buf), + &pos, &lineno); + + if (test_alloc_failed) { + TEST_EQ_P (job, NULL); + + err = nih_error_get (); + TEST_EQ (err->number, ENOMEM); + nih_free (err); + + continue; + } + + TEST_EQ (pos, strlen (buf)); + TEST_EQ (lineno, 3); + + TEST_ALLOC_SIZE (job, sizeof (JobClass)); + + TEST_EQ (job->oom_score_adj, ADJ_TO_SCORE(10)); + + nih_free (job); + } + + TEST_FEATURE ("with an oom score overriding an oom stanza"); + strcpy (buf, "oom -10\n"); + strcat (buf, "oom score 10\n"); + + TEST_ALLOC_FAIL { + pos = 0; + lineno = 1; + job = parse_job (NULL, "test", buf, strlen (buf), + &pos, &lineno); + + if (test_alloc_failed) { + TEST_EQ_P (job, NULL); + + err = nih_error_get (); + TEST_EQ (err->number, ENOMEM); + nih_free (err); + + continue; + } + + TEST_EQ (pos, strlen (buf)); + TEST_EQ (lineno, 3); + + TEST_ALLOC_SIZE (job, sizeof (JobClass)); + + TEST_EQ (job->oom_score_adj, 10); nih_free (job); } @@ -6322,6 +6505,25 @@ test_stanza_oom (void) nih_free (err); + /* Check that an oom score stanza without an argument results in a + * syntax error. + */ + TEST_FEATURE ("with missing score argument"); + strcpy (buf, "oom score\n"); + + pos = 0; + lineno = 1; + job = parse_job (NULL, "test", buf, strlen (buf), &pos, &lineno); + + TEST_EQ_P (job, NULL); + + err = nih_error_get (); + TEST_EQ (err->number, NIH_CONFIG_EXPECTED_TOKEN); + TEST_EQ (pos, 9); + TEST_EQ (lineno, 1); + nih_free (err); + + /* Check that an oom stanza with an overly large argument results * in a syntax error. */ @@ -6340,6 +6542,21 @@ test_stanza_oom (void) TEST_EQ (lineno, 1); nih_free (err); + TEST_FEATURE ("with overly large score argument"); + strcpy (buf, "oom score 1200\n"); + + pos = 0; + lineno = 1; + job = parse_job (NULL, "test", buf, strlen (buf), &pos, &lineno); + + TEST_EQ_P (job, NULL); + + err = nih_error_get (); + TEST_EQ (err->number, PARSE_ILLEGAL_OOM); + TEST_EQ (pos, 10); + TEST_EQ (lineno, 1); + nih_free (err); + /* Check that an oom stanza with an overly small argument results * in a syntax error. @@ -6359,6 +6576,21 @@ test_stanza_oom (void) TEST_EQ (lineno, 1); nih_free (err); + TEST_FEATURE ("with overly small score argument"); + strcpy (buf, "oom score -1200\n"); + + pos = 0; + lineno = 1; + job = parse_job (NULL, "test", buf, strlen (buf), &pos, &lineno); + + TEST_EQ_P (job, NULL); + + err = nih_error_get (); + TEST_EQ (err->number, PARSE_ILLEGAL_OOM); + TEST_EQ (pos, 10); + TEST_EQ (lineno, 1); + nih_free (err); + /* Check that an oom stanza with a non-integer argument results * in a syntax error. @@ -6378,6 +6610,21 @@ test_stanza_oom (void) TEST_EQ (lineno, 1); nih_free (err); + TEST_FEATURE ("with non-integer score argument"); + strcpy (buf, "oom score foo\n"); + + pos = 0; + lineno = 1; + job = parse_job (NULL, "test", buf, strlen (buf), &pos, &lineno); + + TEST_EQ_P (job, NULL); + + err = nih_error_get (); + TEST_EQ (err->number, PARSE_ILLEGAL_OOM); + TEST_EQ (pos, 10); + TEST_EQ (lineno, 1); + nih_free (err); + /* Check that an oom stanza with a partially numeric argument * results in a syntax error. @@ -6397,6 +6644,21 @@ test_stanza_oom (void) TEST_EQ (lineno, 1); nih_free (err); + TEST_FEATURE ("with alphanumeric score argument"); + strcpy (buf, "oom score 12foo\n"); + + pos = 0; + lineno = 1; + job = parse_job (NULL, "test", buf, strlen (buf), &pos, &lineno); + + TEST_EQ_P (job, NULL); + + err = nih_error_get (); + TEST_EQ (err->number, PARSE_ILLEGAL_OOM); + TEST_EQ (pos, 10); + TEST_EQ (lineno, 1); + nih_free (err); + /* Check that an oom stanza with a priority but with an extra * argument afterwards results in a syntax error. @@ -6415,6 +6677,21 @@ test_stanza_oom (void) TEST_EQ (pos, 7); TEST_EQ (lineno, 1); nih_free (err); + + TEST_FEATURE ("with extra score argument"); + strcpy (buf, "oom score 500 foo\n"); + + pos = 0; + lineno = 1; + job = parse_job (NULL, "test", buf, strlen (buf), &pos, &lineno); + + TEST_EQ_P (job, NULL); + + err = nih_error_get (); + TEST_EQ (err->number, NIH_CONFIG_UNEXPECTED_TOKEN); + TEST_EQ (pos, 14); + TEST_EQ (lineno, 1); + nih_free (err); } void </blockquote></div>