From: enh <enh@google.com>
To: Kir Kolyshkin <kolyshkin@gmail.com>
Cc: linux-kernel@vger.kernel.org, libc-alpha@sourceware.org,
musl@lists.openwall.com, linux-api@vger.kernel.org,
Ingo Molnar <mingo@redhat.com>,
Peter Zijlstra <peterz@infradead.org>,
Joel Fernandes <joel@joelfernandes.org>,
Christian Brauner <brauner@kernel.org>
Subject: Re: [PATCH] sched/headers: move struct sched_param out of uapi
Date: Mon, 2 Oct 2023 12:12:48 -0700 [thread overview]
Message-ID: <CAJgzZooaa7m01KMnL+jHPLCX-Gqtps0ZqxhU7q6ai3OXdNzAgA@mail.gmail.com> (raw)
In-Reply-To: <20230808030357.1213829-1-kolyshkin@gmail.com>
On Mon, Aug 7, 2023 at 8:04 PM Kir Kolyshkin via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> Both glibc and musl define struct sched_param in sched.h, while kernel
> has it in uapi/linux/sched/types.h, making it cumbersome to use
> sched_getattr(2) or sched_setattr(2) from userspace.
>
> For example, something like this:
>
> #include <sched.h>
> #include <linux/sched/types.h>
>
> struct sched_attr sa;
>
> will result in "error: redefinition of ‘struct sched_param’" (note the
> code doesn't need sched_param at all -- it needs struct sched_attr
> plus some stuff from sched.h).
note that `struct sched_attr` is still pretty problematic for
userspace because it keeps changing. i (Android's bionic libc
maintainer) get pretty frequent complaints about the lack of a wrapper
for this in libc, but that doesn't seem plausible if it keeps
changing. worse than that, we do find projects copy & pasting `struct
sched_attr` (ltp, for example), which causes problems when bionic --
which uses the latest released uapi headers directly -- and those
projects' duplicates don't match.
it would be helpful (or at least "less problematic") if each new
variant was called sched_attr_v1, sched_attr_v2 etc.
ironically, the end result of the requests for `struct sched_attr` to
be in <sched.h> and to have wrappers for the syscalls is that i'm
seriously considering _removing_ `struct sched_attr` from our uapi
headers [because when i said we use them "directly", they do actually
go through a python script] on the assumption that "everyone just
carries around the specific version they want, and no-one has to worry
about ABI differences" is less problematic than the current situation.
(to be clear: personally i've only seen source incompatibility.
although one _could_ pass `struct sched_attr` across library
boundaries and have ABI incompatibility, i haven't yet seen that.
unless you count "that's the reason why there's no libc wrapper for
this syscall, despite it being by far my most-demanded syscall
wrapper".)
> The situation is, glibc is not going to provide a wrapper for
> sched_{get,set}attr, thus the need to include linux/sched_types.h
> directly, which leads to the above problem.
>
> Thus, the userspace is left with a few sub-par choices when it wants to
> use e.g. sched_setattr(2), such as maintaining a copy of struct
> sched_attr definition, or using some other ugly tricks.
>
> OTOH, struct sched_param is well known, defined in POSIX, and it won't
> be ever changed (as that would break backward compatibility).
>
> So, while struct sched_param is indeed part of the kernel uapi,
> exposing it the way it's done now creates an issue, and hiding it
> (like this patch does) fixes that issue, hopefully without creating
> another one: common userspace software rely on libc headers, and as
> for "special" software (like libc), it looks like glibc and musl
> do not rely on kernel headers for struct sched_param definition
> (but let's Cc their mailing lists in case it's otherwise).
getting back to your actual point about `struct sched_param`, yes,
this sgtm too. bionic has the exact same <sched.h> vs
<linux/sched/types.h> duplication.
> The alternative to this patch would be to move struct sched_attr to,
> say, linux/sched.h, or linux/sched/attr.h (the new file).
as long as everyone promises never to change `struct sched_param`,
that would be my personal preference --- my _ideal_ is that i never
define anything that's uapi and get it "direct" from the [very lightly
modified] upstream uapi headers instead.
> Oh, and here is the previous attempt to fix the issue:
> https://lore.kernel.org/all/20200528135552.GA87103@google.com/
> While I support Linus arguments, the issue is still here
> and needs to be fixed.
>
> Cc: libc-alpha@sourceware.org
> Cc: musl@lists.openwall.com
> Cc: linux-api@vger.kernel.org
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Cc: Christian Brauner <brauner@kernel.org>
> Fixes: e2d1e2aec572 ("sched/headers: Move various ABI definitions to <uapi/linux/sched/types.h>")
> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
> ---
> include/linux/sched.h | 5 ++++-
> include/uapi/linux/sched/types.h | 4 ----
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 609bde814cb0..3167e97a6b04 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -63,7 +63,6 @@ struct robust_list_head;
> struct root_domain;
> struct rq;
> struct sched_attr;
> -struct sched_param;
> struct seq_file;
> struct sighand_struct;
> struct signal_struct;
> @@ -370,6 +369,10 @@ extern struct root_domain def_root_domain;
> extern struct mutex sched_domains_mutex;
> #endif
>
> +struct sched_param {
> + int sched_priority;
> +};
> +
> struct sched_info {
> #ifdef CONFIG_SCHED_INFO
> /* Cumulative counters: */
> diff --git a/include/uapi/linux/sched/types.h b/include/uapi/linux/sched/types.h
> index f2c4589d4dbf..90662385689b 100644
> --- a/include/uapi/linux/sched/types.h
> +++ b/include/uapi/linux/sched/types.h
> @@ -4,10 +4,6 @@
>
> #include <linux/types.h>
>
> -struct sched_param {
> - int sched_priority;
> -};
> -
> #define SCHED_ATTR_SIZE_VER0 48 /* sizeof first published struct */
> #define SCHED_ATTR_SIZE_VER1 56 /* add: util_{min,max} */
>
> --
> 2.41.0
>
prev parent reply other threads:[~2023-10-02 19:13 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-08 3:03 [PATCH] sched/headers: move struct sched_param out of uapi Kir Kolyshkin
2023-10-02 18:57 ` Ingo Molnar
2023-10-02 19:12 ` enh [this message]
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=CAJgzZooaa7m01KMnL+jHPLCX-Gqtps0ZqxhU7q6ai3OXdNzAgA@mail.gmail.com \
--to=enh@google.com \
--cc=brauner@kernel.org \
--cc=joel@joelfernandes.org \
--cc=kolyshkin@gmail.com \
--cc=libc-alpha@sourceware.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=musl@lists.openwall.com \
--cc=peterz@infradead.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).