Hi Thijs,
On Tue, Mar 26, 2024 at 06:49:07PM +0100 Thijs Raymakers wrote:
> Hi Phil,
>
> Thank you for your comments. I've attached an interdiff to this email
> with all the changes that I've made in response.
>
I tried this version out and it doesn't work for me. One of the
basic use cases of a wrapper like this is to run a command with
a new cookie (say starting a container or something).
Coresched requires a pid to do that:
# ./coresched -n ls
coresched: Failed to parse PID for -n/--new: 'ls'
With my coreset utility it does work and gives some information about what
it did:
# coreset -n ls
pid 20860's current cookie: 0x0
pid 20860's new cookie: 0xa9fcfbf1
ABOUT-NLS chrt configure ionice libsmartcols Makefile.in
...
Did I miss something? I think this will be one of the primary use cases for
this utility.
Cheers,
Phil
> Phil Auld wrote on 26-03-2024 at 15:41:
> > The exec should not fork first.
>
> You're right, this is not necessary. I've removed it.
>
> > And the error handling I think is just not right. I believe prctl()
> > returns -1 on error and sets errno. This code will report EPERM for all
> > prtcl errors. While EPERM is likely I don't think it's the only error.
>
> Good catch! I assumed that the return values of the `prctl()` call
> matched the return values of `sched_core_share_pid` in
> `kernel/sched/core_sched.c`. I've updated the error handling code to
> just use errno instead.
>
> > On Sat, Mar 16, 2024 at 06:10:03PM +0100, Thijs Raymakers wrote:
> >> - How should the program behave if the prctl core scheduling API is not
> >> available? It has been in Linus' tree since november 2021
> >> (commit a41b74451b35f7a6529689760eb8c05241feecbc) but it can be
> >> disabled with CONFIG_SCHED_CORE=n
> >
> > This one is tricky. If I recall the error you get is ambiguous in this case
> > (EINVAL, which can be returned for other reasons). It would be nice to be
> > able to report that specifically.
>
> In general, `prctl` does indeed return EINVAL if the operation is not
> recognized, or not supported on the system. The `PR_SCHED_CORE`
> operation itself only returns EINVAL if it is called with
> - an invalid operation
> - an invalid type
> - a negative PID
> - an invalid cookie store address (for PR_SCHED_CORE_GET)
> Assuming that all these cases are prevented by the util, we could
> interpret a EINVAL as a sign that PR_SCHED_CORE is not supported on
> the system.
>
> >> - I've currently licensed it under the EUPL v1.2, which is easier to
> >> enforce in my jurisdiction than the GPL. It is GPL compatible so it
> >> shouldn't be an issue, but if anybody has any remarks on this, please
> >> let me know.
> >
> > The util-linux package is GPL so personally I think it would be good to just
> > make any new addition to that package also GPL.
>
> As far as I understand it, util-linux is a collection of different works
> with different licenses (see the README.licensing file). Not every
> program included in util-linux has to be GPL.
>
> >> +typedef enum {
> >> + SCHED_CORE_SCOPE_PID = PR_SCHED_CORE_SCOPE_THREAD,
> >> + SCHED_CORE_SCOPE_TGID = PR_SCHED_CORE_SCOPE_THREAD_GROUP,
> >> + SCHED_CORE_SCOPE_PGID = PR_SCHED_CORE_SCOPE_PROCESS_GROUP,
> >> +} core_sched_type_t;
> >> +
> >
> > Is this needed? You've made the names a little shorter I guess.
> > Also some versions of prctl.h don't have the PR_SCHED_CORE_SCOPE* defines
> > even when they have the base PR_SCHED_CORE macros.
>
> I've changed to to just use the PR_SCHED_CORE_SCOPE* definitions
> instead.
>
> >> +void core_sched_create_cookie(struct args *args)
> >> +{
> >> + int prctl_errno = prctl(PR_SCHED_CORE, PR_SCHED_CORE_CREATE,
> >> + args->from_pid, args->type, 0);
> >
> > We are creating a new cookie on from_pid, seems odd.
>
> I agree, I changed to to `args->to_pid` because that is more logical.
>
> >> + fprintf(stdout, _(" %s --get <PID>\n"), program_invocation_short_name);
> >> + fprintf(stdout, _(" %s --new <PID> [-t <TYPE>]\n"),
> >> + program_invocation_short_name);
> >> + fprintf(stdout, _(" %s --copy -s <PID> -d <PID> [-t <TYPE>]\n"),
> >> + program_invocation_short_name);
> >> + fprintf(stdout, _(" %s --exec [-s <PID>] -- PROGRAM ARGS... \n"),
> >> + program_invocation_short_name);
> >
> > There are 4 different arguments that take a pid when there are really
> > only at most 2 needed.
>
> I chose this to keep the amount of options that you have to provide as
> small possible. I thought that `coresched -n 123` would be easier to use
> than `coresched -n -p 123`. I guess the amount of different arguments
> that can take PIDs can be reduced to two, but it would make the commands
> a bit longer.
>
> > "Get" could just be the default with no arguments given.
>
> I've removed the explicit --exec and made it the default when a program
> name is provided. If no program name is provided, it defaults to --get.
>
> >> + enum {
> >> + OPT_GET = 'g',
> >> + OPT_NEW = 'n',
> >> + OPT_COPY = 'c',
> >> + OPT_EXEC = 'e',
> >> + OPT_SRC = 's',
> >> + OPT_DEST = 'd',
> >> + OPT_TYPE = 't',
> >> + OPT_VERSION = 'V',
> >> + OPT_HELP = 'h'
> >> + };
> >
> > This seems over-engineered. You have to specificy the characters
> > in the getopt_long call anyway so this just complicates the code for
> > no reason it seems to me.
>
> Fair point, I've removed this. I initially added this to make it easier
> to experiment a bit with different values for the options, but that is
> no longer necessary.
>
> Thanks!
>
> - Thijs
>
> Thijs Raymakers (1):
> coresched: Manage core scheduling cookies for tasks
>
> .gitignore | 1 +
> bash-completion/coresched | 0
> configure.ac | 12 +-
> meson.build | 16 +-
> meson_options.txt | 2 +-
> schedutils/Makemodule.am | 8 +
> schedutils/coresched.1.adoc | 16 ++
> schedutils/coresched.c | 353 ++++++++++++++++++++++++++++++++++++
> 8 files changed, 402 insertions(+), 6 deletions(-)
> create mode 100644 bash-completion/coresched
> create mode 100644 schedutils/coresched.1.adoc
> create mode 100644 schedutils/coresched.c
>
> Interdiff:
> diff --git a/schedutils/coresched.c b/schedutils/coresched.c
> index 4be8f9fda..17d775f2d 100644
> --- a/schedutils/coresched.c
> +++ b/schedutils/coresched.c
> @@ -18,18 +18,24 @@
> #include "nls.h"
> #include "strutils.h"
>
> +// These definitions might not be defined, even if the
> +// prctl interface accepts them.
> +#ifndef PR_SCHED_CORE_SCOPE_THREAD
> +#define PR_SCHED_CORE_SCOPE_THREAD 0
> +#endif
> +#ifndef PR_SCHED_CORE_SCOPE_THREAD_GROUP
> +#define PR_SCHED_CORE_SCOPE_THREAD_GROUP 1
> +#endif
> +#ifndef PR_SCHED_CORE_SCOPE_PROCESS_GROUP
> +#define PR_SCHED_CORE_SCOPE_PROCESS_GROUP 2
> +#endif
> +
> +typedef int core_sched_type_t;
> typedef enum {
> - SCHED_CORE_SCOPE_PID = PR_SCHED_CORE_SCOPE_THREAD,
> - SCHED_CORE_SCOPE_TGID = PR_SCHED_CORE_SCOPE_THREAD_GROUP,
> - SCHED_CORE_SCOPE_PGID = PR_SCHED_CORE_SCOPE_PROCESS_GROUP,
> -} core_sched_type_t;
> -
> -typedef enum {
> - SCHED_CORE_CMD_NONE = 0,
> + SCHED_CORE_CMD_EXEC = 0,
> SCHED_CORE_CMD_GET = 1,
> SCHED_CORE_CMD_CREATE = 2,
> SCHED_CORE_CMD_COPY = 4,
> - SCHED_CORE_CMD_EXEC = 8,
> } core_sched_cmd_t;
>
> struct args {
> @@ -50,44 +56,40 @@ void core_sched_exec_with_cookie(struct args *args, char **argv);
> core_sched_type_t parse_core_sched_type(char *str);
> bool verify_arguments(struct args *args);
> void parse_arguments(int argc, char **argv, struct args *args);
> +void set_pid_or_err(pid_t *dest, pid_t src, const char *err_msg);
> +static void __attribute__((__noreturn__)) usage(void);
>
> unsigned long core_sched_get_cookie(struct args *args)
> {
> unsigned long cookie = 0;
> - int prctl_errno = prctl(PR_SCHED_CORE, PR_SCHED_CORE_GET,
> - args->from_pid, SCHED_CORE_SCOPE_PID, &cookie);
> - if (prctl_errno) {
> - errx(-prctl_errno, "Failed to get cookie from PID %d",
> - args->from_pid);
> + if (prctl(PR_SCHED_CORE, PR_SCHED_CORE_GET, args->from_pid,
> + PR_SCHED_CORE_SCOPE_THREAD, &cookie)) {
> + err(errno, "Failed to get cookie from PID %d", args->from_pid);
> }
> return cookie;
> }
>
> void core_sched_create_cookie(struct args *args)
> {
> - int prctl_errno = prctl(PR_SCHED_CORE, PR_SCHED_CORE_CREATE,
> - args->from_pid, args->type, 0);
> - if (prctl_errno) {
> - errx(-prctl_errno, "Failed to create cookie for PID %d",
> - args->from_pid);
> + if (prctl(PR_SCHED_CORE, PR_SCHED_CORE_CREATE, args->to_pid, args->type,
> + 0)) {
> + err(errno, "Failed to create cookie for PID %d",
> + args->from_pid);
> }
> }
>
> void core_sched_pull_cookie(pid_t from)
> {
> - int prctl_errno = prctl(PR_SCHED_CORE, PR_SCHED_CORE_SHARE_FROM, from,
> - SCHED_CORE_SCOPE_PID, 0);
> - if (prctl_errno) {
> - errx(-prctl_errno, "Failed to pull cookie from PID %d", from);
> + if (prctl(PR_SCHED_CORE, PR_SCHED_CORE_SHARE_FROM, from,
> + PR_SCHED_CORE_SCOPE_THREAD, 0)) {
> + err(errno, "Failed to pull cookie from PID %d", from);
> }
> }
>
> void core_sched_push_cookie(pid_t to, core_sched_type_t type)
> {
> - int prctl_errno =
> - prctl(PR_SCHED_CORE, PR_SCHED_CORE_SHARE_TO, to, type, 0);
> - if (prctl_errno) {
> - errx(-prctl_errno, "Failed to push cookie to PID %d", to);
> + if (prctl(PR_SCHED_CORE, PR_SCHED_CORE_SHARE_TO, to, type, 0)) {
> + err(errno, "Failed to push cookie to PID %d", to);
> }
> }
>
> @@ -100,46 +102,35 @@ void core_sched_copy_cookie(struct args *args)
> void core_sched_exec_with_cookie(struct args *args, char **argv)
> {
> if (!args->exec_argv_offset) {
> - errx(EINVAL, "when --exec is provided, a program name "
> - "has to be given.");
> + usage();
> }
>
> // Move the argument list to the first argument of the program
> argv = &argv[args->exec_argv_offset];
>
> - pid_t pid = fork();
> - if (pid == -1) {
> - errx(errno, "Failed to spawn new process");
> + // If a source PID is provided, try to copy the cookie from
> + // that PID. Otherwise, create a brand new cookie with the
> + // provided type.
> + if (args->from_pid) {
> + core_sched_pull_cookie(args->from_pid);
> + } else {
> + args->to_pid = getpid();
> + core_sched_create_cookie(args);
> }
>
> - if (!pid) {
> - // If a source PID is provided, try to copy the cookie from
> - // that PID. Otherwise, create a brand new cookie with the
> - // provided type.
> - if (args->from_pid) {
> - core_sched_pull_cookie(args->from_pid);
> - } else {
> - args->from_pid = getpid();
> - core_sched_create_cookie(args);
> - }
> - if (execvp(argv[0], argv)) {
> - errexec(argv[0]);
> - }
> - } else {
> - int status = 0;
> - waitpid(pid, &status, 0);
> - exit(status);
> + if (execvp(argv[0], argv)) {
> + errexec(argv[0]);
> }
> }
>
> core_sched_type_t parse_core_sched_type(char *str)
> {
> if (!strncmp(str, "pid\0", 4)) {
> - return SCHED_CORE_SCOPE_PID;
> + return PR_SCHED_CORE_SCOPE_THREAD;
> } else if (!strncmp(str, "tgid\0", 5)) {
> - return SCHED_CORE_SCOPE_TGID;
> + return PR_SCHED_CORE_SCOPE_THREAD_GROUP;
> } else if (!strncmp(str, "pgid\0", 5)) {
> - return SCHED_CORE_SCOPE_PGID;
> + return PR_SCHED_CORE_SCOPE_PROCESS_GROUP;
> }
>
> errx(EINVAL, "'%s' is an invalid option. Must be one of pid/tgid/pgid",
> @@ -155,7 +146,7 @@ static void __attribute__((__noreturn__)) usage(void)
> program_invocation_short_name);
> fprintf(stdout, _(" %s --copy -s <PID> -d <PID> [-t <TYPE>]\n"),
> program_invocation_short_name);
> - fprintf(stdout, _(" %s --exec [-s <PID>] -- PROGRAM ARGS... \n"),
> + fprintf(stdout, _(" %s [-s <PID>] -- PROGRAM ARGS... \n"),
> program_invocation_short_name);
>
> fputs(USAGE_SEPARATOR, stdout);
> @@ -169,9 +160,6 @@ static void __attribute__((__noreturn__)) usage(void)
> fputsln(_(" -c, --copy copy the core scheduling cookie from PID to\n"
> " another PID, requires the --source and --dest option"),
> stdout);
> - fputsln(_(" -e, --exec execute a program with a new core scheduling\n"
> - " cookie."),
> - stdout);
>
> fputs(USAGE_OPTIONS, stdout);
> fputsln(_(" -s, --source <PID> where to copy the core scheduling cookie from."),
> @@ -193,116 +181,141 @@ static void __attribute__((__noreturn__)) usage(void)
>
> bool verify_arguments(struct args *args)
> {
> - if (args->cmd == SCHED_CORE_CMD_NONE) {
> - usage();
> - }
> -
> // Check if the value of args->cmd is a power of 2
> // In that case, only a single function option was set.
> - if (!(args->cmd && !(args->cmd & (args->cmd - 1)))) {
> - errx(EINVAL, "Cannot do more than one function at a time.");
> - }
> -
> - if (args->from_pid < 0) {
> - errx(EINVAL, "source PID cannot be negative");
> + if (args->cmd & (args->cmd - 1)) {
> + errx(EINVAL,
> + "Cannot do more than one function at a time. See %s --help",
> + program_invocation_short_name);
> }
>
> - if (args->to_pid < 0) {
> - errx(EINVAL, "destination PID cannot be negative");
> - }
> -
> - if (args->from_pid == 0 && args->cmd == SCHED_CORE_CMD_COPY) {
> - errx(EINVAL, "valid argument to --source is required");
> - }
> -
> - if (args->to_pid == 0 && args->cmd == SCHED_CORE_CMD_COPY) {
> - errx(EINVAL, "valid argument to --dest is required");
> + switch (args->cmd) {
> + case SCHED_CORE_CMD_GET:
> + if (args->to_pid) {
> + errx(EINVAL,
> + "Cannot use -d/--dest with this -g/--get. See %s --help",
> + program_invocation_short_name);
> + }
> + break;
> + case SCHED_CORE_CMD_CREATE:
> + if (args->from_pid) {
> + errx(EINVAL,
> + "Cannot use -s/--source with this -n/--new. See %s --help",
> + program_invocation_short_name);
> + }
> + break;
> + case SCHED_CORE_CMD_COPY:
> + if (!args->from_pid) {
> + errx(EINVAL,
> + "-s/--source PID is required when copying");
> + }
> + if (!args->to_pid) {
> + errx(EINVAL, "-d/--dest PID is required when copying");
> + }
> + break;
> + case SCHED_CORE_CMD_EXEC:
> + if (args->to_pid) {
> + errx(EINVAL,
> + "Cannot use -d/--dest when spawning a program. See %s --help",
> + program_invocation_short_name);
> + }
> + break;
> }
> + return true;
> +}
>
> - if (args->from_pid == 0 && args->cmd != SCHED_CORE_CMD_EXEC) {
> - errx(EINVAL, "PID cannot be zero");
> +void set_pid_or_err(pid_t *dest, pid_t src, const char *err_msg)
> +{
> + if (*dest) {
> + errx(EINVAL, "Ambigious usage: %s", err_msg);
> + } else {
> + *dest = src;
> }
> -
> - return true;
> }
>
> +static const char *ERR_MSG_MULTIPLE_SOURCE_PIDS =
> + "Multiple source PIDs defined";
> void parse_arguments(int argc, char **argv, struct args *args)
> {
> int c;
> -
> - enum {
> - OPT_GET = 'g',
> - OPT_NEW = 'n',
> - OPT_COPY = 'c',
> - OPT_EXEC = 'e',
> - OPT_SRC = 's',
> - OPT_DEST = 'd',
> - OPT_TYPE = 't',
> - OPT_VERSION = 'V',
> - OPT_HELP = 'h'
> - };
> + pid_t tmp;
>
> static const struct option longopts[] = {
> - { "get", required_argument, NULL, OPT_GET },
> - { "new", required_argument, NULL, OPT_NEW },
> - { "copy", no_argument, NULL, OPT_COPY },
> - { "exec", no_argument, NULL, OPT_EXEC },
> - { "source", required_argument, NULL, OPT_SRC },
> - { "destination", required_argument, NULL, OPT_DEST },
> - { "type", required_argument, NULL, OPT_TYPE },
> - { "version", no_argument, NULL, OPT_VERSION },
> - { "help", no_argument, NULL, OPT_HELP },
> + { "get", required_argument, NULL, 'g' },
> + { "new", required_argument, NULL, 'n' },
> + { "copy", no_argument, NULL, 'c' },
> + { "source", required_argument, NULL, 's' },
> + { "destination", required_argument, NULL, 'd' },
> + { "type", required_argument, NULL, 't' },
> + { "version", no_argument, NULL, 'V' },
> + { "help", no_argument, NULL, 'h' },
> { NULL, 0, NULL, 0 }
> };
>
> - while ((c = getopt_long(argc, argv, "g:n:ces:d:t:Vh", longopts,
> - NULL)) != -1)
> + while ((c = getopt_long(argc, argv, "g:n:cs:d:t:Vh", longopts, NULL)) !=
> + -1)
> switch (c) {
> - case OPT_GET:
> + case 'g':
> args->cmd |= SCHED_CORE_CMD_GET;
> - args->from_pid = strtos32_or_err(
> - optarg, "Failed to parse PID for --get");
> + tmp = strtopid_or_err(
> + optarg, "Failed to parse PID for -g/--get");
> + set_pid_or_err(&args->from_pid, tmp,
> + ERR_MSG_MULTIPLE_SOURCE_PIDS);
> break;
> - case OPT_NEW:
> + case 'n':
> args->cmd |= SCHED_CORE_CMD_CREATE;
> - args->from_pid = strtos32_or_err(
> - optarg, "Failed to parse PID for --new");
> + tmp = strtopid_or_err(
> + optarg, "Failed to parse PID for -n/--new");
> + set_pid_or_err(&args->to_pid, tmp,
> + ERR_MSG_MULTIPLE_SOURCE_PIDS);
> break;
> - case OPT_COPY:
> + case 'c':
> args->cmd |= SCHED_CORE_CMD_COPY;
> break;
> - case OPT_EXEC:
> - args->cmd |= SCHED_CORE_CMD_EXEC;
> - break;
> - case OPT_SRC:
> - args->from_pid = strtos32_or_err(
> - optarg, "Failed to parse PID for --source");
> + case 's':
> + tmp = strtopid_or_err(
> + optarg, "Failed to parse PID for -s/--source");
> + set_pid_or_err(&args->from_pid, tmp,
> + ERR_MSG_MULTIPLE_SOURCE_PIDS);
> break;
> - case OPT_DEST:
> - args->to_pid = strtos32_or_err(
> - optarg, "Failed to parse PID for --dest");
> + case 'd':
> + tmp = strtopid_or_err(
> + optarg, "Failed to parse PID for -d/--dest");
> + set_pid_or_err(&args->to_pid, tmp,
> + "Multiple destination PIDs defined");
> break;
> - case OPT_TYPE:
> + case 't':
> args->type = parse_core_sched_type(optarg);
> break;
> - case OPT_VERSION:
> + case 'V':
> print_version(EXIT_SUCCESS);
> - case OPT_HELP:
> + case 'h':
> usage();
> default:
> errtryhelp(EXIT_FAILURE);
> }
>
> if (argc > optind) {
> - args->exec_argv_offset = optind;
> + if (args->cmd == SCHED_CORE_CMD_EXEC) {
> + args->exec_argv_offset = optind;
> + } else {
> + // -g, -n or -c AND a program to run is provided
> + errx(EINVAL, "bad usage, see %s --help",
> + program_invocation_short_name);
> + }
> + } else if (argc == optind && args->from_pid) {
> + // Neither a function (-g, -n, or -c), nor a program to
> + // run is given
> + args->cmd = SCHED_CORE_CMD_GET;
> }
> +
> verify_arguments(args);
> }
>
> int main(int argc, char **argv)
> {
> struct args arguments = { 0 };
> - arguments.type = SCHED_CORE_SCOPE_TGID;
> + arguments.type = PR_SCHED_CORE_SCOPE_THREAD_GROUP;
>
> setlocale(LC_ALL, "");
> bindtextdomain(PACKAGE, LOCALEDIR);
> --
> 2.44.0
>
--
