Re: [PATCH v6] coresched: Manage core scheduling cookies for tasks

[Phil Auld <pauld@redhat.com>]

On Tue, Apr 09, 2024 at 03:44:30PM +0200 Karel Zak wrote:
> 
>  Hi,
> 
> sorry for delay in my review ...
> 
> On Tue, Apr 09, 2024 at 01:55:32PM +0200, Thijs Raymakers wrote:
> > +== OPTIONS
> > +*-p*, *--pid* _PID_::
> > +Operate on an existing PID and do not launch a new task.
> 
> "Nnot launch a new task" does not seem to be true, as you later
> provide an example:
> 
>   Copy the cookie from a task with pid _123_ to a new task _command_{colon}::
>   *{command} --copy -p* _123_ \-- _command_ [_argument_...]
> 
> > +*-d*, *--dest* _PID_::
> > +When using *--copy*, specify a destination PID where you want to copy the cookie to.
> 
> I find it a little confusing that the --pid is sometimes used as a
> source and in other cases as a destination.
> 
>      coresched --new --pid 123              # pid is destination
>      coresched --copy --pid 123 --dest 456  # pid is source
> 
> It seems --copy always requires a source PID (according to the man
> page), why not require a PID as argument for --copy:
> 
>     coresched --copy 123 --pid 456
> 
> in this way --pid will be always destination (for 'copy' and 'new'
> functions) and you will not need extra --dest option at all.
>

Having copy take the dest pid itself could work for me.

As an aside, I just noticed we lost the ability to push the current cookie
to the new task or pid. I suppose you can just do "--copy $$" and get that
though so probably that's good enough. 


> If you want to keep the basic functions (e.g. --copy) without
> arguments, it would be better to have --source, --dest, and
> --dest-type instead of using the ambiguous --pid.

At one point we had three arguments that took a pid.

> 
> I can also imagine the basic "functions" without "--".
> 
>     coresched [get] [--dest] 123

This is is really --source since we are getting the cookie from it.


>     coresched copy [--source] 123 [--dest] 456
>     coresched new [--dest] 456
>

And using --source for "get" you'd have to change it to --dest
after you check for a cookie before you do new.  

> In my opinion, we do not have to strictly adhere to old taskset or
> similar commands.

We went around about that.  My take is that the users of coresched
will be the same people who use taskset. The idea was to keep it
familiar.

> 
> It should be noted that command line arguments are crucial, as they
> are difficult to modify after release.

Indeed. That's why we went around and around...


Cheers,
Phil

> 
> > +#include <getopt.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <sys/prctl.h>
> > +#include <unistd.h>
> > +
> > +#include "c.h"
> > +#include "closestream.h"
> > +#include "nls.h"
> > +#include "optutils.h"
> > +#include "strutils.h"
> > +
> > +// These definitions might not be defined in the header files, even if the
> > +// prctl interface in the kernel accepts them as valid.
> > +#ifndef PR_SCHED_CORE
> > +#define PR_SCHED_CORE 62
> > +#endif
> 
> pedantic note, use extra space within ifdef
> 
> #ifndef PR_SCHED_CORE
> # define PR_SCHED_CORE 62
> #endif
> 
> please :-)
> 
> > +static void core_sched_push_cookie(pid_t to, sched_core_scope type)
> > +{
> > +	if (prctl(PR_SCHED_CORE, PR_SCHED_CORE_SHARE_TO, to, type, 0))
> > +		err(EXIT_FAILURE, _("Failed to push cookie to PID %d"), to);
> > +}
> > +
> > +static void core_sched_copy_cookie(pid_t from, pid_t to,
> > +				   sched_core_scope to_type)
> > +{
> > +	core_sched_pull_cookie(from);
> > +	core_sched_push_cookie(to, to_type);
> > +
> > +	if (sched_core_verbose) {
> > +		sched_core_cookie before = core_sched_get_cookie(from);
> > +		fprintf(stderr,
> > +			_("%s: copied cookie 0x%lx from PID %d to PID %d\n"),
> > +			program_invocation_short_name, before, from, to);
> 
>  Don't use fprintf(), use:
> 
>         warnx(_(copied cookie 0x%lx from PID %d to PID %d\n"), before, from, to);
> 
> > +static void core_sched_get_and_print_cookie(pid_t pid)
> > +{
> > +	if (sched_core_verbose) {
> > +		sched_core_cookie after = core_sched_get_cookie(pid);
> > +		fprintf(stderr, _("%s: set cookie of PID %d to 0x%lx\n"),
> > +			program_invocation_short_name, pid, after);
> > +	}
> 
>  warnx()
> 
> > +int main(int argc, char **argv)
> > +{
> > +	struct args args = { 0 };
> > +	args.cmd = SCHED_CORE_CMD_GET;
> > +	args.type = PR_SCHED_CORE_SCOPE_THREAD_GROUP;
> 
>  struct args args = {
>         .cmd = SCHED_CORE_CMD_GET,
>         .type = PR_SCHED_CORE_SCOPE_THREAD_GROUP
>  };
> 
>  (the rest is set to zero by compiler).
> 
> > +ts_check_test_command "$TS_CMD_CORESCHED"
> > +ts_check_test_command "tee"
> > +ts_check_test_command "sed"
> 
> ts_check_test_command is for in-tree stuff, use
> 
>     ts_check_prog "tee"
>     ts_check_prog "sed"
> 
> for things in $PATH.
> 
> > +ts_init_subtest "get-own-pid-with-cookie"
> > +$TS_CMD_CORESCHED -p $$ 2>> "$TS_ERRLOG" | sed "s/$$/OWN_PID/g" | sed "s/$CORESCHED_COOKIE/COOKIE/g" >> "$TS_OUTPUT"
> 
>  Let's save some forks, execute sed(1) only once:
> 
>     | sed -e "s/$$/OWN_PID/g" \
>           -e "s/$CORESCHED_COOKIE/COOKIE/g" >> "$TS_OUTPUT"
> 
> > +ts_finalize_subtest
> > +
> > +ts_init_subtest "spawn-child-with-new-cookie"
> > +$TS_CMD_CORESCHED -n -- "$TS_CMD_CORESCHED" 2>> "$TS_ERRLOG" \
> > +  | sed 's/^.*\(0x.*$\)/\1/g' \
> > +  | sed "s/$CORESCHED_COOKIE/SAME_COOKIE/g" \
> > +  | sed "s/0x.*$/DIFFERENT_COOKIE/g" >> "$TS_OUTPUT"
> > +ts_finalize_subtest
> 
>   | sed -e ... -e ... -e 
> 
> > +ts_init_subtest "change-cookie-of-parent"
> > +$TS_CMD_CORESCHED -n -- /bin/bash -c "$TS_CMD_CORESCHED -c -p \$\$ -d $$"
> > +$TS_CMD_CORESCHED -p $$ 2>> "$TS_ERRLOG" \
> > +  | sed "s/$$/OWN_PID/g" \
> > +  | sed "s/$CORESCHED_COOKIE/COOKIE/g" \
> > +  | sed "s/0x.*$/DIFFERENT_COOKIE/g" >> "$TS_OUTPUT"
> > +ts_finalize_subtest
> 
>   | sed -e ... -e ... -e 
> 
> 
> Looks good, thanks for all the work!
> 
>     Karel
> 
> -- 
>  Karel Zak  <kzak@redhat.com>
>  http://karelzak.blogspot.com
> 

--