Merge lp:~vorlon/upstart/default-priority-not-always-zero into lp:upstart

[Steve Langasek]

Upstart doesn't cope well when it can't set the nice level to 0, which may
be the case if the process is non-root and has been niced. So instead of
using 0 as the default, use the current nice level as the default; this
gives identical results in the common case, and there's really no reason a
non-root upstart should fail if niced.

[James Hunt (jamesodhunt)]

Hi Steve,

I think really we should be querying the priority when jobs are started since otherwise we effectively cache the priority such that if it is changed _after_ a Session Init is started, any newly-created user job will be run with the original priority.

I think the correct fix therefore is to modify job_process_spawn() to call getpriority() just prior to the setpriority() to deterine the current priority. We can retain the original behaviour if running as root *and* as PID 1, but not if only running as root (to allow for root running a Session Init).

review: Needs Fixing

[James Hunt (jamesodhunt)]

Strike that first comment. What we actually need to do is:

- retain the original behaviour and set class->nice = 0.
- in job_process_spawn(), only call setpriority() if:
  - running as root and with PID 1.
  - not running as root and where 'class->nice > getpriority()'.

That way, Session Init job processes will by default just get the priority of their parent (whatever that has recently been set to). But if a user job attempts to lower the job priority explictly using the nice stanza, that will be honoured).

[Steve Langasek (vorlon)]

Thanks for the review, James. You're right that my proposed solution falls a bit short. I think there are still some issues with your counterproposal as well, though:

 - a non-root session init process may have CAP_SYS_NICE set, in which case 'nice' settings in the job which raise the priority should be honored.
 - For root, if getpriority() != 0 and class->nice == 0, it's impossible to distinguish between a default and an explicitly-configured 'nice 0'. For the first, we should not be resetting the priority; for the second, we should be setting it.

and a lower-priority issue:

 - If the user session has a positive getpriority() value n, and a job foo has a 'nice' option m such that 0 < m < n, foo will be started with the same priority as all the other jobs, whereas it's likely the *intent* was to start it with a nice level m *higher* than the session (i.e., base+m where base is normally 0).

I'll put together a fix addressing the first two points (as well as the other points you brought up), then take the last to upstart-devel for discussion as it requires a syntax change.

[James Hunt (jamesodhunt)]

LGTM - merged.

review: Approve

=== modified file 'ChangeLog'
--- ChangeLog	2012-12-11 13:59:01 +0000
+++ ChangeLog	2012-12-14 21:14:25 +0000
@@ -1,3 +1,12 @@
[phone]  Steve Langasek  <[e-mail]>
+	* init/job_class.[ch]: instead of assuming a fixed value (0) as the
+	  default nice value for job processes, use whatever the nice value
+	  of the current process is.  This will be important later for user
+	  sessions where an entire session may be started with a higher nice
+	  value; and it fixes running the test suite as part of a nice'd
+	  build.
+	* init/tests/test_job_class.c: update test suite to match.
+
 2012-12-11  James Hunt  <[e-mail]>
 
 	* init/Makefile.am: Add explicit -lrt for tests (LP: #1088863)

=== modified file 'init/job_class.c'
--- init/job_class.c	2012-11-22 16:32:36 +0000
+++ init/job_class.c	2012-12-14 21:14:25 +0000
@@ -27,6 +27,8 @@
 #include <errno.h>
 #include <string.h>
 #include <signal.h>
+#include <sys/time.h>
+#include <sys/resource.h>
 
 #include <nih/macros.h>
 #include <nih/alloc.h>
@@ -219,7 +221,7 @@
 	class->console = default_console >= 0 ? default_console : CONSOLE_LOG;
 
 	class->umask = JOB_DEFAULT_UMASK;
-	class->nice = JOB_DEFAULT_NICE;
+	class->nice = JOB_NICE_INVALID;
 	class->oom_score_adj = JOB_DEFAULT_OOM_SCORE_ADJ;
 
 	for (i = 0; i < RLIMIT_NLIMITS; i++)

=== modified file 'init/job_class.h'
--- init/job_class.h	2012-09-24 09:10:05 +0000
+++ init/job_class.h	2012-12-14 21:14:25 +0000
@@ -102,11 +102,11 @@
 #define JOB_DEFAULT_UMASK 022
 
 /**
- * JOB_DEFAULT_NICE:
+ * JOB_NICE_INVALID:
  *
- * The default nice level for processes.
+ * The nice level for processes when no nice level is set.
  **/
-#define JOB_DEFAULT_NICE 0
+#define JOB_NICE_INVALID -21
 
 /**
  * JOB_DEFAULT_OOM_SCORE_ADJ:

=== modified file 'init/job_process.c'
--- init/job_process.c	2012-12-03 20:27:09 +0000
+++ init/job_process.c	2012-12-14 21:14:25 +0000
@@ -780,7 +780,8 @@
 
 	/* Adjust the process priority ("nice level").
 	 */
-	if (setpriority (PRIO_PROCESS, 0, class->nice) < 0) {
+	if (class->nice != JOB_NICE_INVALID &&
+	    setpriority (PRIO_PROCESS, 0, class->nice) < 0) {
 		nih_error_raise_system ();
 		job_process_error_abort (fds[1],
 					 JOB_PROCESS_ERROR_PRIORITY, 0);

=== modified file 'init/tests/test_job_class.c'
--- init/tests/test_job_class.c	2011-12-09 15:27:57 +0000
+++ init/tests/test_job_class.c	2012-12-14 21:14:25 +0000
@@ -27,6 +27,8 @@
 #include <sys/wait.h>
 #include <sys/ptrace.h>
 #include <sys/select.h>
+#include <sys/time.h>
+#include <sys/resource.h>
 
 #include <time.h>
 #include <stdio.h>
@@ -131,7 +133,7 @@
 		TEST_EQ (class->console, CONSOLE_LOG);
 
 		TEST_EQ (class->umask, 022);
-		TEST_EQ (class->nice, 0);
+		TEST_EQ (class->nice, JOB_NICE_INVALID);
 		TEST_EQ (class->oom_score_adj, 0);
 
 		for (i = 0; i < RLIMIT_NLIMITS; i++)