[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200818003210.GD3265@brightrain.aerifal.cx>
Date: Mon, 17 Aug 2020 20:32:10 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: Support SIGEV_THREAD_ID
On Thu, Aug 01, 2019 at 12:15:57PM -0400, James Y Knight wrote:
> There seems to be some debate in glibc over whether this API should be
> supported, due to the long-standing debate about "pthread_t" vs
> "kernel tid" APIs. (And this API uses kernel tids, of course.)
>
> One proposal from previous glibc discussion was to add a
> SIGEV_PTHREAD_ID, which takes a pthread_t, instead of a kernel tid.
> Nobody has done this yet, and I'd note that if it is done, that is not
> at all incompatible with also continuing to support SIGEV_THREAD_ID.
> (Just like both sched_setaffinity and pthread_setaffinity_np exist
> without issue).
>
> Despite that discussion, SIGEV_THREAD_ID functionality does in fact
> work with glibc -- it provides the SIGEV_THREAD_ID define in its
> headers, and it defines the same 'struct sigevent' as the kernel does,
> including a _tid member. The only thing it's missing is the field name
> "sigev_notify_thread_id" -- so users are simply doing "#define
> sigev_notify_thread_id _sigev_un._tid" as a workaround (ugh).
I'd like to go forward with this, along with adding gettid() which I'm
doing now.
> However, it does _not_ work today with musl, as musl's timer_create
> function translates the user-facing struct to a separate kernel-facing
> structure.
>
> Given long-standing usage of this feature, and given that potential
> future directions are additive, not conflicting, ISTM reasonable to
> just add support for this to musl.
> From 0a0aef759f216444f558f427466b47f38d678052 Mon Sep 17 00:00:00 2001
> From: James Y Knight <jyknight@...gle.com>
> Date: Sun, 30 Jun 2019 21:55:20 -0400
> Subject: [PATCH] Add support for SIGEV_THREAD_ID and sigev_notify_thread_id.
>
> This is like SIGEV_SIGNAL, but targeted to a particular thread's
> tid, rather than the process.
> ---
> include/signal.h | 16 +++++++++++++---
> src/time/timer_create.c | 8 ++++++--
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/include/signal.h b/include/signal.h
> index 5c48cb83..735e0ac7 100644
> --- a/include/signal.h
> +++ b/include/signal.h
> @@ -180,14 +180,24 @@ struct sigevent {
> union sigval sigev_value;
> int sigev_signo;
> int sigev_notify;
> - void (*sigev_notify_function)(union sigval);
> - pthread_attr_t *sigev_notify_attributes;
> - char __pad[56-3*sizeof(long)];
> + union {
> + char __pad[64 - 2*sizeof(int) - sizeof(union sigval)];
> + pid_t sigev_notify_thread_id;
> + struct {
> + void (*sigev_notify_function)(union sigval);
> + pthread_attr_t *sigev_notify_attributes;
> + } __sev_thread;
> + } __sev_fields;
> };
I was concerned about the padding size computation, but it seems to
check out:
Before the patch, if you adjust to make the sigev_notify_* members
overlap with the padding, you have 56-sizeof(long) == 56-sizeof(union
sigval) bytes of padding.
After the patch, it's 64-2*sizeof(int)-sizeof(union sigval) bytes of
padding, where 64-2*sizeof(int)==56.
> +#define sigev_notify_thread_id __sev_fields.sigev_notify_thread_id
> +#define sigev_notify_function __sev_fields.__sev_thread.sigev_notify_function
> +#define sigev_notify_attributes __sev_fields.__sev_thread.sigev_notify_attributes
> +
I'm still ok with using the macros unless/until we make a global
change to anon unions and structs. (And I'm aware of the C++ problem
raised before... :/)
> #define SIGEV_SIGNAL 0
> #define SIGEV_NONE 1
> #define SIGEV_THREAD 2
> +#define SIGEV_THREAD_ID 4 /* Linux extension */
Comment not needed/wanted in header but I can drop it when merging.
> int __libc_current_sigrtmin(void);
> int __libc_current_sigrtmax(void);
> diff --git a/src/time/timer_create.c b/src/time/timer_create.c
> index c5e40a19..5a3f4d3a 100644
> --- a/src/time/timer_create.c
> +++ b/src/time/timer_create.c
> @@ -83,11 +83,15 @@ int timer_create(clockid_t clk, struct sigevent *restrict evp, timer_t *restrict
> switch (evp ? evp->sigev_notify : SIGEV_SIGNAL) {
> case SIGEV_NONE:
> case SIGEV_SIGNAL:
> + case SIGEV_THREAD_ID:
> if (evp) {
> ksev.sigev_value = evp->sigev_value;
> ksev.sigev_signo = evp->sigev_signo;
> ksev.sigev_notify = evp->sigev_notify;
> - ksev.sigev_tid = 0;
> + if (evp->sigev_notify == SIGEV_THREAD_ID)
> + ksev.sigev_tid = evp->sigev_notify_thread_id;
> + else
> + ksev.sigev_tid = 0;
> ksevp = &ksev;
> }
> if (syscall(SYS_timer_create, clk, ksevp, &timerid) < 0)
> @@ -115,7 +119,7 @@ int timer_create(clockid_t clk, struct sigevent *restrict evp, timer_t *restrict
>
> ksev.sigev_value.sival_ptr = 0;
> ksev.sigev_signo = SIGTIMER;
> - ksev.sigev_notify = 4; /* SIGEV_THREAD_ID */
> + ksev.sigev_notify = SIGEV_THREAD_ID;
> ksev.sigev_tid = td->tid;
> if (syscall(SYS_timer_create, clk, &ksev, &timerid) < 0)
> timerid = -1;
> --
> 2.22.0.709.g102302147b-goog
This looks ok, but I think the same SIGEV_THREAD_ID support should be
added to aio. That can be done as a separate patch.
So I think this is OK in its current form.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
