From: Phil Auld <pauld@redhat.com>
To: Karel Zak <kzak@redhat.com>
Cc: Thijs Raymakers <thijs@raymakers.nl>,
thomas@t-8ch.de, util-linux@vger.kernel.org
Subject: Re: [PATCH v6] coresched: Manage core scheduling cookies for tasks
Date: Tue, 9 Apr 2024 10:12:51 -0400 [thread overview]
Message-ID: <20240409141251.GC319320@lorien.usersys.redhat.com> (raw)
In-Reply-To: <20240409134430.twndleg7rxnfsf4m@ws.net.home>
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
>
--
next prev parent reply other threads:[~2024-04-09 14:13 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-16 17:10 [PATCH RFC] coresched: Manage core scheduling cookies for tasks Thijs Raymakers
2024-03-24 20:07 ` Karel Zak
2024-03-26 14:41 ` Phil Auld
2024-03-26 14:45 ` Phil Auld
2024-03-26 17:49 ` [PATCH 0/1] " Thijs Raymakers
2024-03-26 18:13 ` Phil Auld
2024-03-27 12:43 ` [PATCH v2 " Thijs Raymakers
2024-03-27 13:21 ` Phil Auld
2024-03-27 14:18 ` Phil Auld
2024-03-27 15:30 ` [PATCH v3] " Thijs Raymakers
2024-04-01 17:18 ` Thomas Weißschuh
2024-04-04 22:03 ` [PATCH v4] " Thijs Raymakers
2024-04-05 6:26 ` Thomas Weißschuh
2024-04-05 14:14 ` Phil Auld
2024-04-08 21:16 ` [PATCH v5] " Thijs Raymakers
2024-04-09 6:12 ` Thomas Weißschuh
2024-04-09 11:55 ` [PATCH v6] " Thijs Raymakers
2024-04-09 13:44 ` Karel Zak
2024-04-09 14:12 ` Phil Auld [this message]
2024-04-10 21:11 ` [PATCH v7] " Thijs Raymakers
2024-04-10 22:15 ` Phil Auld
2024-04-11 11:02 ` [PATCH v8] " Thijs Raymakers
2024-04-11 13:46 ` Phil Auld
2024-04-17 10:31 ` Karel Zak
2024-04-17 11:39 ` [PATCH v9] " Thijs Raymakers
2024-04-23 9:27 ` Karel Zak
2024-04-23 10:19 ` Thomas Weißschuh
2024-04-23 11:12 ` [PATCH v10] " Thijs Raymakers
2024-04-25 15:36 ` [PATCH v11] " Thijs Raymakers
2024-04-25 15:51 ` Thomas Weißschuh
2024-04-25 16:22 ` [PATCH v12] " Thijs Raymakers
2024-04-26 7:59 ` Karel Zak
2024-04-26 18:03 ` Thomas Weißschuh
2024-04-29 8:28 ` Karel Zak
2024-04-17 12:27 ` [PATCH v8] " Phil Auld
2024-03-27 12:43 ` [PATCH v2 1/1] " Thijs Raymakers
2024-03-27 14:09 ` Phil Auld
2024-03-26 19:09 ` [PATCH 0/1] " Phil Auld
2024-03-26 19:26 ` Thijs Raymakers
2024-03-26 20:16 ` Phil Auld
2024-03-26 20:17 ` Phil Auld
2024-03-26 17:49 ` [PATCH 1/1] " Thijs Raymakers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240409141251.GC319320@lorien.usersys.redhat.com \
--to=pauld@redhat.com \
--cc=kzak@redhat.com \
--cc=thijs@raymakers.nl \
--cc=thomas@t-8ch.de \
--cc=util-linux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.