Hi!
On Tue, 2009-01-13 at 22:46:06 +0000, Chris Coulson wrote:
> I have re-written the patch again now. It now accepts both IO scheduler
> class and IO scheduler data with a single flag, as you said you would
> prefer earlier (in the format "-I <class>:<data>"). Also, on non-Linux
> platforms or Linux systems that don't support ioprio_set, passing the
> '-I' flag will have no effect, as the new code won't be compiled in (it
> will just silently ignore it).
>
> There is error-checking on the scheduler class string. To convert the
> scheduler data string in to an integer, I used strtol(), as that allows
> a certain level of error checking in the string to be converted, so it
> detects if you pass non-integer characters as the scheduler data (as
> opposed to just converting it to zero, as atoi() does, which is valid
> anyway).
Thanks for improving the patch. Could you also document the new option
in the man page?
I had a patch around to add support for process scheduling policies,
that I've cleaned up and committed today so that it could be useful
as a reference (commit id a5908b7ddb65d84650f0dab407097bdbca6a2cc2).
You can download the current development code with:
git clone git://git.debian.org/git/dpkg/dpkg.git
Added some comments inline:
> === modified file 'utils/start-stop-daemon.c'
> --- utils/start-stop-daemon.c 2009-01-13 17:28:50 +0000
> +++ utils/start-stop-daemon.c 2009-01-13 22:01:36 +0000
> @@ -103,6 +103,26 @@
> #include <error.h>
> #endif
>
> +#include <sys/syscall.h>
> +#if defined(SYS_ioprio_set) && defined(linux)
> +# define SupportIOPrioSet
> +#endif
Could you add a check in configure.ac for the 'sys/syscall.h' header
as it's not present in all systems?
> +#ifdef SupportIOPrioSet
No camel case plase. :)
> +enum {
> + IOPRIO_WHO_PROCESS = 1,
> + IOPRIO_WHO_PGRP,
> + IOPRIO_WHO_USER,
> +};
> +
> +enum {
> + IOPRIO_CLASS_NONE,
> + IOPRIO_CLASS_RT,
> + IOPRIO_CLASS_BE,
> + IOPRIO_CLASS_IDLE,
> +};
> +#endif
> +
> static int testmode = 0;
> static int quietmode = 0;
> static int exitnodo = 1;
> @@ -127,6 +147,18 @@
> static const char *progname = "";
> static int nicelevel = 0;
> static int umask_value = -1;
> +#ifdef SupportIOPrioSet
> +static int ioprio_mask = 0;
> +static char *ioprio_class_in = NULL;
> +static char *ioprio_data_in = NULL;
> +static int ioprio_class = IOPRIO_CLASS_BE;
> +static long int ioprio_data = 4;
> +static long int ioprio_data_temp = 0;
> +static char *temp = NULL;
The less globals used the better, you should be able to use the new
'struct res_schedule' to hold some of this data, the rest should be
moved to an inner scope.
> +#define IOPRIO_CLASS_SHIFT 13
> +#define IOPRIO_PRIO_VALUE(class, data) (((class) <<
> IOPRIO_CLASS_SHIFT) | data)
> +#endif
>
> static struct stat exec_stat;
> #if defined(OSHURD)
> @@ -169,6 +201,9 @@
> static int pid_is_exec(pid_t pid, const struct stat *esb);
> #endif
>
> +#ifdef SupportIOPrioSet
> +static inline int ioprio_set(int which, int who, int ioprio);
> +#endif
Try to define the function early on so that we don't need a prototype
for a static function.
> static void fatal(const char *format, ...)
> NONRETURNING PRINTFFORMAT(1, 2);
> @@ -308,6 +343,9 @@
> " -r|--chroot <directory> chroot to <directory> before starting\n"
> " -d|--chdir <directory> change to <directory> (default is /)\n"
> " -N|--nicelevel <incr> add incr to the process's nice level\n"
> +#ifdef SupportIOPrioSet
> +" -I|--ioprio <class>:<data> set the process's IO scheduler class and
> class data\n"
> +#endif
This should not be conditional, we always want the help available.
Also this would cause problems in case we wanted to add i18n support
here later on. Also it should be less than 80 chars wide.
> " -k|--umask <mask> change the umask to <mask> before
> starting\n"
> " -b|--background force the process to detach\n"
> " -m|--make-pidfile create the pidfile before starting\n"
> @@ -520,6 +558,7 @@
> { "exec", 1, NULL, 'x'},
> { "chuid", 1, NULL, 'c'},
> { "nicelevel", 1, NULL, 'N'},
> + { "ioprio", 1, NULL, 'I'},
I think probably better name this 'iosched', in line with 'procsched'.
> { "umask", 1, NULL, 'k'},
> { "background", 0, NULL, 'b'},
> { "make-pidfile", 0, NULL, 'm'},
> @@ -533,9 +572,9 @@
> int c;
>
> for (;;) {
> - c = getopt_long(argc, argv,
> - "HKSVa:n:op:qr:s:tu:vx:c:N:k:bmR:g:d:",
> - longopts, NULL);
> + c = getopt_long(argc, argv,
> + "HKSVa:n:op:qr:s:tu:vx:c:N:I:k:bmR:g:d:",
> + longopts, NULL);
> if (c == -1)
> break;
> switch (c) {
> @@ -597,6 +636,13 @@
> case 'N': /* --nice */
> nicelevel = atoi(optarg);
> break;
> + case 'I': /* --ioprio */
> +#ifdef SupportIOPrioSet
> + ioprio_class_in = strdup(optarg);
> + ioprio_class_in = strtok(ioprio_class_in, ":");
> + ioprio_data_in = strtok(NULL, ":");
> +#endif
> + break;
Here only save optarg, and do the parsing and validation later on in
this same function. And you can use now xstrdup.
> case 'k': /* --umask <mask> */
> umask_str = optarg;
> break;
> @@ -1221,6 +1267,14 @@
> }
> }
>
> +#ifdef SupportIOPrioSet
> +static inline int
> +ioprio_set (int which, int who, int ioprio)
> +{
> + return syscall(SYS_ioprio_set, which, who, ioprio);
> +}
> +#endif
> +
> int
> main(int argc, char **argv)
> {
> @@ -1334,6 +1388,40 @@
> fatal("Unable to alter nice level by %i: %s",
> nicelevel, strerror(errno));
> }
> +#ifdef SupportIOPrioSet
> + if (ioprio_class_in != NULL) {
> + if (strcmp(ioprio_class_in, "real-time") == 0) {
> + ioprio_class = IOPRIO_CLASS_RT;
> + } else if (strcmp(ioprio_class_in, "best-effort") == 0) {
> + ioprio_class = IOPRIO_CLASS_BE;
> + } else if (strcmp(ioprio_class_in, "idle") == 0) {
> + ioprio_class = IOPRIO_CLASS_IDLE;
> + } else {
> + fatal("Unrecognized IO scheduler class %s",
> ioprio_class_in);
> + }
> + switch (ioprio_class) {
> + case (IOPRIO_CLASS_IDLE):
Indent the case at the same level as the switch, and no need for the
parenthesis in the case.
> + ioprio_data = 7;
> + break;
> + default:
> + if (ioprio_data_in != NULL) {
> + ioprio_data_temp =
> strtol(ioprio_data_in, &temp, 10);
> + if (ioprio_data_in != temp) {
> + ioprio_data = ioprio_data_temp;
> + } else {
> + fatal("Unrecognized IO
> scheduler class data %s", ioprio_data_in);
> + }
> + }
You can use parse_integer instead.
> + }
We want as much validation as possible to happen always, so that a
packager trying this on an unsupported system would get a meaningful
error then, instead of having to wait for someone to run it and get it
to fail on a supported system.
> + ioprio_mask = IOPRIO_PRIO_VALUE(ioprio_class, ioprio_data);
> + if (quietmode < 0)
> + printf("IO priority mask %i", ioprio_mask);
The values to be printed should be on the code dealing with --test.
> + errno = 0;
> + if ((ioprio_set(IOPRIO_WHO_PROCESS, getpid(), ioprio_mask) ==
> -1) && (errno != 0))
> + fatal("Unable to alter IO priority to mask %i: %s",
> + ioprio_mask, strerror(errno));
> + }
Better to move this part into its own function.
> +#endif
> if (umask_value >= 0)
> umask(umask_value);
> if (mpidfile && pidfile != NULL) {
thanks,
guillem
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
