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@xxxxxxxxxx>
> http://karelzak.blogspot.com
>
--
