[-- Attachment #1: Type: text/plain, Size: 1271 bytes --] 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). 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. [-- Attachment #2: 0001-Add-support-for-SIGEV_THREAD_ID-and-sigev_notify_thr.patch --] [-- Type: text/x-patch, Size: 2647 bytes --] 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; }; +#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 + #define SIGEV_SIGNAL 0 #define SIGEV_NONE 1 #define SIGEV_THREAD 2 +#define SIGEV_THREAD_ID 4 /* Linux extension */ 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
* James Y. Knight:
> 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.)
The debate is over and has been decided in favor of supporting TIDs. We
just have a backlog of interfaces for which we need to add support.
Fortunately, C++ is standardizing stackless coroutines, so this has not
been proven to be a mistake.
Thanks,
Florian
On Thu, Aug 01, 2019 at 06:24:31PM +0200, Florian Weimer wrote:
> * James Y. Knight:
>
> > 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.)
>
> The debate is over and has been decided in favor of supporting TIDs. We
> just have a backlog of interfaces for which we need to add support.
>
> Fortunately, C++ is standardizing stackless coroutines, so this has not
> been proven to be a mistake.
Good to know. I think we can go forward with doing the same then,
providing the interfaces to get and use tids.
Rich
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). > > 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; > }; > > +#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 really hate these macro hacks, and have been looking at getting rid of the ones we have (using anon unions). We don't mandate C11 to use the public headers, but it might make sense to mandate C11 || __GNUC__ and stick __extension__ on the struct if __GNUC__ is defined. Unfortunately, cparser/firm prior to latest git master does not support designated initializers right with anon unions, and GCC 3.x doesn't either (yes, there are users of GCC 3 with musl, both full-time and just as a bootstrapping-from-plain-C stage for distros). So I'm not sure if we can fix this yet or just keep doing the same nasty macro hack for now... Alternatively we could just put the new member in the padding since we're translating the struct anyway, but that sacrifices glibc ABI-compat for the newly-added feature. Rich
[-- Attachment #1: Type: text/plain, Size: 9208 bytes --] On Thu, Aug 1, 2019 at 12:50 PM Rich Felker <dalias@libc.org> wrote: > > 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). > > > > 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; > > }; > > > > +#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 really hate these macro hacks, and have been looking at getting rid > of the ones we have (using anon unions). We don't mandate C11 to use > the public headers, but it might make sense to mandate C11 || __GNUC__ > and stick __extension__ on the struct if __GNUC__ is defined. > Unfortunately, cparser/firm prior to latest git master does not > support designated initializers right with anon unions, and GCC 3.x > doesn't either (yes, there are users of GCC 3 with musl, both > full-time and just as a bootstrapping-from-plain-C stage for distros). > So I'm not sure if we can fix this yet or just keep doing the same > nasty macro hack for now... 100% agreed! I had actually drafted the email below proposing to get rid of this in favor of C11 anonymous struct/union. However, after writing the below text, I then abandoned that idea, because _no_ version of the C++ standard supports anonymous structs -- although every version has supported anonymous unions. And I figured that because the musl headers must be parseable in C++, and given musl's goals of standards adherence, the use of a widely-supported-yet-nonstandard compiler extension would not be acceptable, even if effectively every C++ compiler in use supports it. If I was wrong about that, I'd be quite happy to use anonymous structs and unions here (and to convert the other cases as well). Unnamed struct/unions have been standardized as of C11, and was a > very-long-standing compiler extension in at least GCC, Clang, and MSVC > before that. (Looks to have been first introduced in GCC in version 3.0). > Docs https://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Unnamed-Fields.html The feature makes things a bit cleaner than having to introduce #defines to > name the inner-fields, as is generally done now. Both gcc and clang accept > these unnamed, even in "-std=c89". They even accept such structs without > warning in musl headers with "-std=c89 -pedantic", since pedantic warnings > are ignored in system headers (otherwise all sorts of things like "long > long" would also warn!). Given that this feature is standard now for 8 years, and was widely > supported for decades before that, it seems that it may be a nice > simplification. On Thu, Aug 1, 2019 at 12:50 PM Rich Felker <dalias@libc.org> wrote: > 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). > > > > 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; > > }; > > > > +#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 really hate these macro hacks, and have been looking at getting rid > of the ones we have (using anon unions). We don't mandate C11 to use > the public headers, but it might make sense to mandate C11 || __GNUC__ > and stick __extension__ on the struct if __GNUC__ is defined. > Unfortunately, cparser/firm prior to latest git master does not > support designated initializers right with anon unions, and GCC 3.x > doesn't either (yes, there are users of GCC 3 with musl, both > full-time and just as a bootstrapping-from-plain-C stage for distros). > So I'm not sure if we can fix this yet or just keep doing the same > nasty macro hack for now... > > Alternatively we could just put the new member in the padding since > we're translating the struct anyway, but that sacrifices glibc > ABI-compat for the newly-added feature. > > Rich > [-- Attachment #2: Type: text/html, Size: 11876 bytes --]
On Thu, Aug 01, 2019 at 02:00:44PM -0400, James Y Knight wrote:
> On Thu, Aug 1, 2019 at 12:50 PM Rich Felker <dalias@libc.org> wrote:
> >
> > 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).
> > >
> > > 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;
> > > };
> > >
> > > +#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 really hate these macro hacks, and have been looking at getting rid
> > of the ones we have (using anon unions). We don't mandate C11 to use
> > the public headers, but it might make sense to mandate C11 || __GNUC__
> > and stick __extension__ on the struct if __GNUC__ is defined.
> > Unfortunately, cparser/firm prior to latest git master does not
> > support designated initializers right with anon unions, and GCC 3.x
> > doesn't either (yes, there are users of GCC 3 with musl, both
> > full-time and just as a bootstrapping-from-plain-C stage for distros).
> > So I'm not sure if we can fix this yet or just keep doing the same
> > nasty macro hack for now...
>
> 100% agreed!
>
> I had actually drafted the email below proposing to get rid of this in
> favor of C11 anonymous struct/union. However, after writing the below text,
> I then abandoned that idea, because _no_ version of the C++ standard
> supports anonymous structs -- although every version has supported
> anonymous unions. And I figured that because the musl headers must be
> parseable in C++, and given musl's goals of standards adherence, the use of
> a widely-supported-yet-nonstandard compiler extension would not be
> acceptable, even if effectively every C++ compiler in use supports it.
The C++ part is really disappointing, since poor interaction with C++
is one of the main reasons I wanted to eliminate the #defines... :(
I think we could probably get by with only anon unions, not anon
structs, but I haven't done any detailed analysis. We should probably
just put this aside for now, though, since it looks less productive
relative to the painfulness than I expected...
Rich
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
On 2019-08-01, Florian Weimer <fweimer@redhat.com> wrote:
> * James Y. Knight:
>
>> 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.)
>
> The debate is over and has been decided in favor of supporting TIDs. We
> just have a backlog of interfaces for which we need to add support.
Hi Florian,
Am I interpreting you correctly that glibc intends to add a
sigev_notify_thread_id define to access this field in struct sigevent?
I plan to send a patch to qemu to use sigev_notify_thread_id over
_sigev_un.tid if it is available, and I just want to confirm that
glibc intends to follow suit before I mention that in the commit
message.
Thanks
[-- Attachment #1: Type: text/plain, Size: 958 bytes --] On 10/30/20 2:22 PM, Michael Forney wrote: > On 2019-08-01, Florian Weimer <fweimer@redhat.com> wrote: >> * James Y. Knight: >> >>> 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.) >> >> The debate is over and has been decided in favor of supporting TIDs. We >> just have a backlog of interfaces for which we need to add support. > > Hi Florian, > > Am I interpreting you correctly that glibc intends to add a > sigev_notify_thread_id define to access this field in struct sigevent? > I plan to send a patch to qemu to use sigev_notify_thread_id over > _sigev_un.tid if it is available, and I just want to confirm that > glibc intends to follow suit before I mention that in the commit > message. we will need something similar for gperftools as well. > > Thanks > [-- Attachment #2: pEpkey.asc --] [-- Type: application/pgp-keys, Size: 2373 bytes --]
* Michael Forney: > On 2019-08-01, Florian Weimer <fweimer@redhat.com> wrote: >> * James Y. Knight: >> >>> 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.) >> >> The debate is over and has been decided in favor of supporting TIDs. We >> just have a backlog of interfaces for which we need to add support. > > Hi Florian, > > Am I interpreting you correctly that glibc intends to add a > sigev_notify_thread_id define to access this field in struct sigevent? > I plan to send a patch to qemu to use sigev_notify_thread_id over > _sigev_un.tid if it is available, and I just want to confirm that > glibc intends to follow suit before I mention that in the commit > message. I do not expect that we are going to add a sigev_notify_thread_id member once glibc implements the struct member. On common configurations, it's likely that we are going to use a different approach, using anonymous structs and unions. I think you will have to use a different mechanism for detecting the presence of the struct member. Thanks, Florian -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill