From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL, SUBJ_OBFU_PUNCT_FEW autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 20952 invoked from network); 18 Aug 2020 00:32:28 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 18 Aug 2020 00:32:28 -0000 Received: (qmail 3570 invoked by uid 550); 18 Aug 2020 00:32:23 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 3551 invoked from network); 18 Aug 2020 00:32:22 -0000 Date: Mon, 17 Aug 2020 20:32:10 -0400 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20200818003210.GD3265@brightrain.aerifal.cx> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] 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 > 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