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.
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.
I can also imagine the basic "functions" without "--".
coresched [get] [--dest] 123
coresched copy [--source] 123 [--dest] 456
coresched new [--dest] 456
In my opinion, we do not have to strictly adhere to old taskset or
similar commands.
It should be noted that command line arguments are crucial, as they
are difficult to modify after release.
> +#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
