sched: Only setting CPUSchedulingPriority=rr doesn't work

A service that only sets the scheduling policy to round-robin
fails to be started. This is because the cpu_sched_priority is
initialized to 0 and is not adjusted when the policy is changed.

Clamp the cpu_sched_priority when the scheduler policy is set. Use
the current policy to validate the new priority.

Change the manual page to state that the given range only applies
to the real-time scheduling policies.

Add a testcase that verifies this change:

$ make test-sched-prio; ./test-sched-prio
[test/sched_idle_bad.service:6] CPU scheduling priority is out of range, ignoring: 1
[test/sched_rr_bad.service:7] CPU scheduling priority is out of range, ignoring: 0
[test/sched_rr_bad.service:8] CPU scheduling priority is out of range, ignoring: 100

diff --git a/.gitignore b/.gitignore
index 18e4e23c507fdfe6f065cc1fa9f08312dd6eebec..3fbd83e0e606acc1f04ac5f9d299872dd52a0ae7 100644 (file)
--- a/.gitignore
+++ b/.gitignore
@@ -90,6 +90,7 @@
 /systemd
 /test-engine
 /test-job-type
 /systemd
 /test-engine
 /test-job-type

+/test-sched-prio

 /systemctl
 /systemadm
 .dirstamp
 /systemctl
 /systemadm
 .dirstamp

diff --git a/Makefile.am b/Makefile.am
index da3885dbe7c3001a0aed1ec93d58075c8405f96b..42fed59641e26550d3b88c22d7982c2c3382169a 100644 (file)
--- a/Makefile.am
+++ b/Makefile.am
@@ -1185,7 +1185,8 @@ noinst_PROGRAMS += \
        test-unit-file \
        test-date \
        test-sleep \
        test-unit-file \
        test-date \
        test-sleep \

-       test-replace-var
+       test-replace-var \
+       test-sched-prio

 
 TESTS += \
        test-job-type \
 
 TESTS += \
        test-job-type \


@@ -1195,7 +1196,15 @@ TESTS += \
        test-unit-file \
        test-date \
        test-sleep \
        test-unit-file \
        test-date \
        test-sleep \

-       test-replace-var
+       test-replace-var \
+       test-sched-prio
+
+EXTRA_DIST += \
+       test/sched_idle_bad.service \
+       test/sched_idle_ok.service \
+       test/sched_rr_bad.service \
+       test/sched_rr_ok.service \
+       test/sched_rr_change.service

 
 test_engine_SOURCES = \
        src/test/test-engine.c
 
 test_engine_SOURCES = \
        src/test/test-engine.c


@@ -1323,6 +1332,18 @@ test_watchdog_SOURCES = \
 test_watchdog_LDADD = \
        libsystemd-shared.la
 
 test_watchdog_LDADD = \
        libsystemd-shared.la
 

+test_sched_prio_SOURCES = \
+       src/test/test-sched-prio.c
+
+test_sched_prio_CFLAGS = \
+       $(AM_CFLAGS) \
+       $(DBUS_CFLAGS) \
+       -D"STR(s)=\#s" -D"TEST_DIR=STR($(abs_top_srcdir)/test/)"
+
+test_sched_prio_LDADD = \
+       libsystemd-core.la \
+       libsystemd-daemon.la
+

 # ------------------------------------------------------------------------------
 systemd_initctl_SOURCES = \
        src/initctl/initctl.c
 # ------------------------------------------------------------------------------
 systemd_initctl_SOURCES = \
        src/initctl/initctl.c

diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml
index 7b6514375d23ad188643e99eb10309c968d35332..b684bfbe5c06e156445fd9a27df9bc66ff648f61 100644 (file)
--- a/man/systemd.exec.xml
+++ b/man/systemd.exec.xml
@@ -214,13 +214,15 @@
 
                                 <listitem><para>Sets the CPU
                                 scheduling priority for executed
 
                                 <listitem><para>Sets the CPU
                                 scheduling priority for executed

-                                processes. Takes an integer between 1
-                                (lowest priority) and 99 (highest
-                                priority). The available priority
+                                processes. The available priority

                                 range depends on the selected CPU
                                 range depends on the selected CPU

-                                scheduling policy (see above). See
-                                <citerefentry><refentrytitle>sched_setscheduler</refentrytitle><manvolnum>2</manvolnum></citerefentry>
-                                for details.</para></listitem>
+                                scheduling policy (see above). For
+                                real-time scheduling policies an
+                                integer between 1 (lowest priority)
+                                and 99 (highest priority) can be used.
+                                See <citerefentry><refentrytitle>sched_setscheduler</refentrytitle><manvolnum>2</manvolnum></citerefentry>
+                                for details.
+                                </para></listitem>

                         </varlistentry>
 
                         <varlistentry>
                         </varlistentry>
 
                         <varlistentry>

diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c
index b99e70e05a144de8b77f6e72345a83862a1c3e4f..6759255984288fe350101f0552bc46f2add75d2a 100644 (file)
--- a/src/core/load-fragment.c
+++ b/src/core/load-fragment.c
@@ -4,6 +4,7 @@
   This file is part of systemd.
 
   Copyright 2010 Lennart Poettering
   This file is part of systemd.
 
   Copyright 2010 Lennart Poettering

+  Copyright 2012 Holger Hans Peter Freyther

 
   systemd is free software; you can redistribute it and/or modify it
   under the terms of the GNU Lesser General Public License as published by
 
   systemd is free software; you can redistribute it and/or modify it
   under the terms of the GNU Lesser General Public License as published by


@@ -689,6 +690,8 @@ int config_parse_exec_cpu_sched_policy(
         }
 
         c->cpu_sched_policy = x;
         }
 
         c->cpu_sched_policy = x;

+        /* Moving to or from real-time policy? We need to adjust the priority */
+        c->cpu_sched_priority = CLAMP(c->cpu_sched_priority, sched_get_priority_min(x), sched_get_priority_max(x));

         c->cpu_sched_set = true;
 
         return 0;
         c->cpu_sched_set = true;
 
         return 0;


@@ -705,19 +708,28 @@ int config_parse_exec_cpu_sched_prio(
                 void *userdata) {
 
         ExecContext *c = data;
                 void *userdata) {
 
         ExecContext *c = data;

-        int i;
+        int i, min, max;

 
         assert(filename);
         assert(lvalue);
         assert(rvalue);
         assert(data);
 
 
         assert(filename);
         assert(lvalue);
         assert(rvalue);
         assert(data);
 

-        /* On Linux RR/FIFO have the same range */
-        if (safe_atoi(rvalue, &i) < 0 || i < sched_get_priority_min(SCHED_RR) || i > sched_get_priority_max(SCHED_RR)) {
+        if (safe_atoi(rvalue, &i) < 0) {

                 log_error("[%s:%u] Failed to parse CPU scheduling priority, ignoring: %s", filename, line, rvalue);
                 return 0;
         }
 
                 log_error("[%s:%u] Failed to parse CPU scheduling priority, ignoring: %s", filename, line, rvalue);
                 return 0;
         }
 

+
+        /* On Linux RR/FIFO range from 1 to 99 and OTHER/BATCH may only be 0 */
+        min = sched_get_priority_min(c->cpu_sched_policy);
+        max = sched_get_priority_max(c->cpu_sched_policy);
+
+        if (i < min || i > max) {
+                log_error("[%s:%u] CPU scheduling priority is out of range, ignoring: %s", filename, line, rvalue);
+                return 0;
+        }
+

         c->cpu_sched_priority = i;
         c->cpu_sched_set = true;
 
         c->cpu_sched_priority = i;
         c->cpu_sched_set = true;
 

diff --git a/src/test/test-sched-prio.c b/src/test/test-sched-prio.c
new file mode 100644 (file)
index 0000000..29235e8
--- /dev/null
+++ b/src/test/test-sched-prio.c
@@ -0,0 +1,86 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+/***
+  This file is part of systemd.
+
+  Copyright 2012 Holger Hans Peter Freyther
+
+  systemd is free software; you can redistribute it and/or modify it
+  under the terms of the GNU Lesser General Public License as published by
+  the Free Software Foundation; either version 2.1 of the License, or
+  (at your option) any later version.
+
+  systemd is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+  Lesser General Public License for more details.
+
+  You should have received a copy of the GNU Lesser General Public License
+  along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <sched.h>
+
+#include "manager.h"
+
+
+int main(int argc, char *argv[]) {
+        Manager *m;
+        Unit *idle_ok, *idle_bad, *rr_ok, *rr_bad, *rr_sched;
+        Service *ser;
+        FILE *serial = NULL;
+        FDSet *fdset = NULL;
+
+        /* prepare the test */
+        assert_se(set_unit_path(TEST_DIR) >= 0);
+        assert_se(manager_new(SYSTEMD_SYSTEM, &m) >= 0);
+        assert_se(manager_startup(m, serial, fdset) >= 0);
+
+        /* load idle ok */
+        assert_se(manager_load_unit(m, "sched_idle_ok.service", NULL, NULL, &idle_ok) >= 0);
+        assert_se(idle_ok->load_state == UNIT_LOADED);
+        ser = SERVICE(idle_ok);
+        assert_se(ser->exec_context.cpu_sched_policy == SCHED_OTHER);
+        assert_se(ser->exec_context.cpu_sched_priority == 0);
+
+        /*
+         * load idle bad. This should print a warning but we have no way to look at it.
+         */
+        assert_se(manager_load_unit(m, "sched_idle_bad.service", NULL, NULL, &idle_bad) >= 0);
+        assert_se(idle_bad->load_state == UNIT_LOADED);
+        ser = SERVICE(idle_ok);
+        assert_se(ser->exec_context.cpu_sched_policy == SCHED_OTHER);
+        assert_se(ser->exec_context.cpu_sched_priority == 0);
+
+        /*
+         * load rr ok.
+         * Test that the default priority is moving from 0 to 1.
+         */
+        assert_se(manager_load_unit(m, "sched_rr_ok.service", NULL, NULL, &rr_ok) >= 0);
+        assert_se(rr_ok->load_state == UNIT_LOADED);
+        ser = SERVICE(rr_ok);
+        assert_se(ser->exec_context.cpu_sched_policy == SCHED_RR);
+        assert_se(ser->exec_context.cpu_sched_priority == 1);
+
+        /*
+         * load rr bad.
+         * Test that the value of 0 and 100 is ignored.
+         */
+        assert_se(manager_load_unit(m, "sched_rr_bad.service", NULL, NULL, &rr_bad) >= 0);
+        assert_se(rr_bad->load_state == UNIT_LOADED);
+        ser = SERVICE(rr_bad);
+        assert_se(ser->exec_context.cpu_sched_policy == SCHED_RR);
+        assert_se(ser->exec_context.cpu_sched_priority == 1);
+
+        /*
+         * load rr change.
+         * Test that anything between 1 and 99 can be set.
+         */
+        assert_se(manager_load_unit(m, "sched_rr_change.service", NULL, NULL, &rr_sched) >= 0);
+        assert_se(rr_sched->load_state == UNIT_LOADED);
+        ser = SERVICE(rr_sched);
+        assert_se(ser->exec_context.cpu_sched_policy == SCHED_RR);
+        assert_se(ser->exec_context.cpu_sched_priority == 99);
+
+        return 0;
+}

diff --git a/test/sched_idle_bad.service b/test/sched_idle_bad.service
new file mode 100644 (file)
index 0000000..589a87c
--- /dev/null
+++ b/test/sched_idle_bad.service
@@ -0,0 +1,6 @@
+[Unit]
+Description=Bad sched priority for Idle
+
+[Service]
+ExecStart=/bin/true
+CPUSchedulingPriority=1

diff --git a/test/sched_idle_ok.service b/test/sched_idle_ok.service
new file mode 100644 (file)
index 0000000..262ef3e
--- /dev/null
+++ b/test/sched_idle_ok.service
@@ -0,0 +1,6 @@
+[Unit]
+Description=Sched idle with prio 0
+
+[Service]
+ExecStart=/bin/true
+CPUSchedulingPriority=0

diff --git a/test/sched_rr_bad.service b/test/sched_rr_bad.service
new file mode 100644 (file)
index 0000000..0be534a
--- /dev/null
+++ b/test/sched_rr_bad.service
@@ -0,0 +1,8 @@
+[Unit]
+Description=Bad sched priority for RR
+
+[Service]
+ExecStart=/bin/true
+CPUSchedulingPolicy=rr
+CPUSchedulingPriority=0
+CPUSchedulingPriority=100

diff --git a/test/sched_rr_change.service b/test/sched_rr_change.service
new file mode 100644 (file)
index 0000000..b3e3a00
--- /dev/null
+++ b/test/sched_rr_change.service
@@ -0,0 +1,9 @@
+[Unit]
+Description=Change prio
+
+[Service]
+ExecStart=/bin/true
+CPUSchedulingPolicy=rr
+CPUSchedulingPriority=1
+CPUSchedulingPriority=2
+CPUSchedulingPriority=99

diff --git a/test/sched_rr_ok.service b/test/sched_rr_ok.service
new file mode 100644 (file)
index 0000000..b88adc5
--- /dev/null
+++ b/test/sched_rr_ok.service
@@ -0,0 +1,6 @@
+[Unit]
+Description=Default prio for RR
+
+[Service]
+ExecStart=/bin/true
+CPUSchedulingPolicy=rr