Re: [musl] Support SIGEV_THREAD_ID

[Rich Felker <dalias@libc.org>]

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@google.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