[0/7] Implement proposed POSIX _clockwait variants of existing _timedwait functions
mbox series

Message ID cover.b0c66849a87ca79889a49f2f1f2563b1a8a15d8b.1551291557.git-series.mac@mcrowe.com
Headers show
Series
  • Implement proposed POSIX _clockwait variants of existing _timedwait functions
Related show

Message

Mike Crowe Feb. 27, 2019, 6:23 p.m. UTC
My attempts[1] to add a variant of pthread_cond_timedwait that would accept
a clockid_t parameter led me to propose[2] to The Austin Group the addition
of an entire family of functions that accept a clockid_t parameter to
indicate the clock that the timespec absolute timeout parameter should be
measured against. They responded positively to the request but an
implementation is required before the proposal can proceed.

This patch series is the first part of that implementation in glibc, it
contains four new functions:

int pthread_cond_clockwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
                           clockid_t clock, const struct timespec *abstime)

int pthread_rwlock_clockrdlock(pthread_rwlock_t *rwlock, clockid_t clock,
                               const struct timespec *abstime)

int pthread_rwlock_clockwrlock(pthread_rwlock_t *rwlock, clockid_t clock,
                               const struct timespec *abstime)

int sem_clockwait(sem_t *restrict, clockid_t clock_id, const struct
                  timespec *restrict)

These are implemented by replacing the underlying equivalent _timed
functions with ones that accept a clockid_t parameter, and then
implementing the existing _timed functions by passing CLOCK_REALTIME to the
new implementation. This requires clockid_t parameters to be added to the
underlying futex-internal and lowlevellock-futex functions.

pthread_mutex_clocklock is not yet implemented because doing so requires a
version of __lll_timedlock_wait that supports both CLOCK_MONOTONIC and
CLOCK_REALTIME. This function is currently architecture specific. I plan to
work on that next, if these changes are considered acceptable.

The mq_clockreceive and mq_clocksend functions corresponding to
mq_timedreceive and mq_timedsend require kernel changes before they can be
implemented in glibc.

As implemented, passing an unsupported or invalid clock to these functions
yields EINVAL. I considered returning ENOTSUP for valid-but-unsupported
clocks, but I was worried that there was a risk that the list of valid
clocks would not be updated when a new clock was added to glibc.

A number of tests have been updated to use struct timespec rather than
struct timeval so that they can call clock_gettime(CLOCK_MONOTONIC) rather
then gettimeofday. To make this easier I created the support/timespec.h
header file to contain functions to add and subtract timespec structures
borrowed from sysdeps/pthread/posix-timer.h. There's probably a better
place for these, but I don't know where that might be.

Rather than duplicating tests, I've parameterised them so that the same
tests can be run on the existing timedwait functions and the new clockwait
functions.

The changes have been tested with "make check" on x86_64 and arm64. Earlier
versions were tested on arm. I haven't updated the ChangeLog, NEWS or
architecture abilists yet (so some tests fail on arm64), but will do so if
the rest of the changes are acceptable.

Thanks to everyone that commented on previous versions of this patch (when
the single new function was called pthread_cond_timedwaitonclock_np.) They
have been a great help, and I hope that I have incorporated their feedback
correctly.

[1] https://sourceware.org/ml/libc-alpha/2015-07/msg00193.html
[2] http://austingroupbugs.net/view.php?id=1216

Mike Crowe (7):
  nptl: Add clockid parameter to futex timed wait calls
  nptl: Add POSIX-proposed sem_clockwait
  nptl: Add POSIX-proposed pthread_cond_clockwait
  nptl: pthread_rwlock: Move timeout validation into _full functions
  nptl/tst-rwlock14: Test pthread/rwlock_timedwrlock correctly
  nptl/tst-rwlock: Use clock_gettime/timespec rather than gettimeofday/timeval
  nptl: Add POSIX-proposed pthread_rwlock_clockrdlock & pthread_rwlock_clockwrlock

 conform/data/semaphore.h-data                        |   1 +-
 manual/threads.texi                                  |  58 ++++-
 nptl/Makefile                                        |   8 +-
 nptl/Versions                                        |   5 +-
 nptl/forward.c                                       |   5 +-
 nptl/nptl-init.c                                     |   1 +-
 nptl/pthreadP.h                                      |   4 +-
 nptl/pthread_cond_wait.c                             |  45 ++-
 nptl/pthread_mutex_timedlock.c                       |   8 +-
 nptl/pthread_rwlock_clockrdlock.c                    |  27 ++-
 nptl/pthread_rwlock_clockwrlock.c                    |  27 ++-
 nptl/pthread_rwlock_common.c                         |  32 +-
 nptl/pthread_rwlock_rdlock.c                         |   2 +-
 nptl/pthread_rwlock_timedrdlock.c                    |  12 +-
 nptl/pthread_rwlock_timedwrlock.c                    |  12 +-
 nptl/pthread_rwlock_wrlock.c                         |   2 +-
 nptl/sem_clockwait.c                                 |  45 +++-
 nptl/sem_timedwait.c                                 |   3 +-
 nptl/sem_wait.c                                      |   3 +-
 nptl/sem_waitcommon.c                                |  15 +-
 nptl/tst-abstime.c                                   |  28 ++-
 nptl/tst-cond11.c                                    |  30 +-
 nptl/tst-cond26.c                                    |  91 +++++++-
 nptl/tst-cond27.c                                    | 113 +++++++++-
 nptl/tst-rwlock14.c                                  | 159 +++++++++++-
 nptl/tst-rwlock6.c                                   | 132 ++++++----
 nptl/tst-rwlock7.c                                   | 108 +++++---
 nptl/tst-rwlock9.c                                   |  90 +++++--
 nptl/tst-sem13.c                                     |  50 +++-
 nptl/tst-sem17.c                                     |  57 ++++-
 nptl/tst-sem5.c                                      |  30 +-
 support/timespec.h                                   |  27 ++-
 sysdeps/nptl/futex-internal.h                        |   7 +-
 sysdeps/nptl/lowlevellock-futex.h                    |  14 +-
 sysdeps/nptl/pthread-functions.h                     |   4 +-
 sysdeps/nptl/pthread.h                               |  21 ++-
 sysdeps/pthread/semaphore.h                          |   4 +-
 sysdeps/unix/sysv/linux/futex-internal.h             |  26 +-
 sysdeps/unix/sysv/linux/lowlevellock-futex.h         |  29 +-
 sysdeps/unix/sysv/linux/x86_64/64/libpthread.abilist |   4 +-
 40 files changed, 1124 insertions(+), 215 deletions(-)
 create mode 100644 nptl/pthread_rwlock_clockrdlock.c
 create mode 100644 nptl/pthread_rwlock_clockwrlock.c
 create mode 100644 nptl/sem_clockwait.c
 create mode 100644 nptl/tst-cond26.c
 create mode 100644 nptl/tst-cond27.c
 create mode 100644 nptl/tst-sem17.c
 create mode 100644 support/timespec.h

base-commit: a04549c19407a29a271779598a9518f9baf959e0

Comments

Adhemerval Zanella March 5, 2019, 12:35 p.m. UTC | #1
On 27/02/2019 15:23, Mike Crowe wrote:
> My attempts[1] to add a variant of pthread_cond_timedwait that would accept
> a clockid_t parameter led me to propose[2] to The Austin Group the addition
> of an entire family of functions that accept a clockid_t parameter to
> indicate the clock that the timespec absolute timeout parameter should be
> measured against. They responded positively to the request but an
> implementation is required before the proposal can proceed.

Thanks for the work on sorting this out and bring to Austin Group discussion.
The discussion links are quite elucidative and I agree with the rationale to 
add the new API.

> 
> This patch series is the first part of that implementation in glibc, it
> contains four new functions:
> 
> int pthread_cond_clockwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
>                            clockid_t clock, const struct timespec *abstime)
> 
> int pthread_rwlock_clockrdlock(pthread_rwlock_t *rwlock, clockid_t clock,
>                                const struct timespec *abstime)
> 
> int pthread_rwlock_clockwrlock(pthread_rwlock_t *rwlock, clockid_t clock,
>                                const struct timespec *abstime)
> 
> int sem_clockwait(sem_t *restrict, clockid_t clock_id, const struct
>                   timespec *restrict)
> 
> These are implemented by replacing the underlying equivalent _timed
> functions with ones that accept a clockid_t parameter, and then
> implementing the existing _timed functions by passing CLOCK_REALTIME to the
> new implementation. This requires clockid_t parameters to be added to the
> underlying futex-internal and lowlevellock-futex functions.

I am not sure about how to name a new API in this specific case where
the idea is to use as a reference one for POSIX inclusion.  MY understanding 
is we still should use a non-portable name scheme with _np suffix and whence
it is de factor accepted by POSIX we can just alias to the standard
names. Also if it is done before the release I think we can avoid exporting 
the _np naming scheme.

> 
> pthread_mutex_clocklock is not yet implemented because doing so requires a
> version of __lll_timedlock_wait that supports both CLOCK_MONOTONIC and
> CLOCK_REALTIME. This function is currently architecture specific. I plan to
> work on that next, if these changes are considered acceptable.

About that I think we can work towards once my lowlevellock.S cleanup
is accepted [4]. Once __lll_timedlock_wait is implemented in generic
way it would be much more easier to add such extension.

[1] https://sourceware.org/ml/libc-alpha/2019-02/msg00569.html

> 
> The mq_clockreceive and mq_clocksend functions corresponding to
> mq_timedreceive and mq_timedsend require kernel changes before they can be
> implemented in glibc.

Should we add them as ENOSYS as generic stubs in this version?
Because it seems the kernel work required would be just add a new
syscall on ipc/mqueue.c to receive a clock_id since 
schedule_hrtimeout_range_clock already handle CLOCK_MONOTONIC
(and it luckily fits on 6 argument syscall limit).

> 
> As implemented, passing an unsupported or invalid clock to these functions
> yields EINVAL. I considered returning ENOTSUP for valid-but-unsupported
> clocks, but I was worried that there was a risk that the list of valid
> clocks would not be updated when a new clock was added to glibc.

I think either error signalling will have the same outcome: without glibc
updating internally its supported clock ids when issuing futex syscalls 
internally programs won't be able to actually use new clocks in recent kernels.

The only possible way to avoid this issue is if we get a guarantee from kernel
that at N-bits on the futex_op argument will be used for clock identification
so we can just pass the clock_id directly on syscall.  It assumes we don't need
to actually change its representation when calling the syscall and that kernel
will fail for unsupported clocks.

> 
> A number of tests have been updated to use struct timespec rather than
> struct timeval so that they can call clock_gettime(CLOCK_MONOTONIC) rather
> then gettimeofday. To make this easier I created the support/timespec.h
> header file to contain functions to add and subtract timespec structures
> borrowed from sysdeps/pthread/posix-timer.h. There's probably a better
> place for these, but I don't know where that might be.

I think you forgot to update this part since you seemed to had chose
to use support/timespec.h instead.

> 
> Rather than duplicating tests, I've parameterised them so that the same
> tests can be run on the existing timedwait functions and the new clockwait
> functions.
> 
> The changes have been tested with "make check" on x86_64 and arm64. Earlier
> versions were tested on arm. I haven't updated the ChangeLog, NEWS or
> architecture abilists yet (so some tests fail on arm64), but will do so if
> the rest of the changes are acceptable.

Are arm64 failures related to the patchset itself?

> 
> Thanks to everyone that commented on previous versions of this patch (when
> the single new function was called pthread_cond_timedwaitonclock_np.) They
> have been a great help, and I hope that I have incorporated their feedback
> correctly.
> 
> [1] https://sourceware.org/ml/libc-alpha/2015-07/msg00193.html
> [2] http://austingroupbugs.net/view.php?id=1216> 
> Mike Crowe (7):
>   nptl: Add clockid parameter to futex timed wait calls
>   nptl: Add POSIX-proposed sem_clockwait
>   nptl: Add POSIX-proposed pthread_cond_clockwait
>   nptl: pthread_rwlock: Move timeout validation into _full functions
>   nptl/tst-rwlock14: Test pthread/rwlock_timedwrlock correctly
>   nptl/tst-rwlock: Use clock_gettime/timespec rather than gettimeofday/timeval
>   nptl: Add POSIX-proposed pthread_rwlock_clockrdlock & pthread_rwlock_clockwrlock
> 
>  conform/data/semaphore.h-data                        |   1 +-
>  manual/threads.texi                                  |  58 ++++-
>  nptl/Makefile                                        |   8 +-
>  nptl/Versions                                        |   5 +-
>  nptl/forward.c                                       |   5 +-
>  nptl/nptl-init.c                                     |   1 +-
>  nptl/pthreadP.h                                      |   4 +-
>  nptl/pthread_cond_wait.c                             |  45 ++-
>  nptl/pthread_mutex_timedlock.c                       |   8 +-
>  nptl/pthread_rwlock_clockrdlock.c                    |  27 ++-
>  nptl/pthread_rwlock_clockwrlock.c                    |  27 ++-
>  nptl/pthread_rwlock_common.c                         |  32 +-
>  nptl/pthread_rwlock_rdlock.c                         |   2 +-
>  nptl/pthread_rwlock_timedrdlock.c                    |  12 +-
>  nptl/pthread_rwlock_timedwrlock.c                    |  12 +-
>  nptl/pthread_rwlock_wrlock.c                         |   2 +-
>  nptl/sem_clockwait.c                                 |  45 +++-
>  nptl/sem_timedwait.c                                 |   3 +-
>  nptl/sem_wait.c                                      |   3 +-
>  nptl/sem_waitcommon.c                                |  15 +-
>  nptl/tst-abstime.c                                   |  28 ++-
>  nptl/tst-cond11.c                                    |  30 +-
>  nptl/tst-cond26.c                                    |  91 +++++++-
>  nptl/tst-cond27.c                                    | 113 +++++++++-
>  nptl/tst-rwlock14.c                                  | 159 +++++++++++-
>  nptl/tst-rwlock6.c                                   | 132 ++++++----
>  nptl/tst-rwlock7.c                                   | 108 +++++---
>  nptl/tst-rwlock9.c                                   |  90 +++++--
>  nptl/tst-sem13.c                                     |  50 +++-
>  nptl/tst-sem17.c                                     |  57 ++++-
>  nptl/tst-sem5.c                                      |  30 +-
>  support/timespec.h                                   |  27 ++-
>  sysdeps/nptl/futex-internal.h                        |   7 +-
>  sysdeps/nptl/lowlevellock-futex.h                    |  14 +-
>  sysdeps/nptl/pthread-functions.h                     |   4 +-
>  sysdeps/nptl/pthread.h                               |  21 ++-
>  sysdeps/pthread/semaphore.h                          |   4 +-
>  sysdeps/unix/sysv/linux/futex-internal.h             |  26 +-
>  sysdeps/unix/sysv/linux/lowlevellock-futex.h         |  29 +-
>  sysdeps/unix/sysv/linux/x86_64/64/libpthread.abilist |   4 +-
>  40 files changed, 1124 insertions(+), 215 deletions(-)
>  create mode 100644 nptl/pthread_rwlock_clockrdlock.c
>  create mode 100644 nptl/pthread_rwlock_clockwrlock.c
>  create mode 100644 nptl/sem_clockwait.c
>  create mode 100644 nptl/tst-cond26.c
>  create mode 100644 nptl/tst-cond27.c
>  create mode 100644 nptl/tst-sem17.c
>  create mode 100644 support/timespec.h
> 
> base-commit: a04549c19407a29a271779598a9518f9baf959e0
>
Joseph Myers March 6, 2019, 9:15 p.m. UTC | #2
On Tue, 5 Mar 2019, Adhemerval Zanella wrote:

> > The mq_clockreceive and mq_clocksend functions corresponding to
> > mq_timedreceive and mq_timedsend require kernel changes before they can be
> > implemented in glibc.
> 
> Should we add them as ENOSYS as generic stubs in this version?

I'd expect any generic stubs to work with CLOCK_REALTIME without returning 
ENOSYS errors in that case.

> > The changes have been tested with "make check" on x86_64 and arm64. Earlier
> > versions were tested on arm. I haven't updated the ChangeLog, NEWS or
> > architecture abilists yet (so some tests fail on arm64), but will do so if
> > the rest of the changes are acceptable.
> 
> Are arm64 failures related to the patchset itself?

My reading is that they result from missing ABI updates.  (Each patch 
ought to update all relevant ABI lists to avoid introducing failures.)
Mike Crowe March 10, 2019, 9:12 a.m. UTC | #3
On Wednesday 06 March 2019 at 21:15:18 +0000, Joseph Myers wrote:
> On Tue, 5 Mar 2019, Adhemerval Zanella wrote:
> 
> > > The mq_clockreceive and mq_clocksend functions corresponding to
> > > mq_timedreceive and mq_timedsend require kernel changes before they can be
> > > implemented in glibc.
> > 
> > Should we add them as ENOSYS as generic stubs in this version?
> 
> I'd expect any generic stubs to work with CLOCK_REALTIME without returning 
> ENOSYS errors in that case.

Yes. However, I wasn't planning to implement mq_clockreceive and
mq_clocksend or propose them for POSIX now. Apparently they can be
described as being "future direction". I'd rather get the other functions
completed first.

It would be straightforward to add them with support for CLOCK_REALTIME
only, but I'm not sure that it really brings much value. Adding
CLOCK_MONOTONIC support at the same time as the new functions would mean
that autoconf scripts would know that CLOCK_MONOTONIC was supported if the
functions were found, rather than having to fall back at runtime.

> > > The changes have been tested with "make check" on x86_64 and arm64. Earlier
> > > versions were tested on arm. I haven't updated the ChangeLog, NEWS or
> > > architecture abilists yet (so some tests fail on arm64), but will do so if
> > > the rest of the changes are acceptable.
> > 
> > Are arm64 failures related to the patchset itself?
> 
> My reading is that they result from missing ABI updates.  (Each patch 
> ought to update all relevant ABI lists to avoid introducing failures.)

Yes. I did spend a while updating various ABI lists, but I kept having to
change them each time there was a glibc release, so I decided that I'd wait
until the rest of the changes were acceptable. Similarly for NEWS and
ChangeLog.

Thanks, and thanks for all the other comments. I'll update the patches
accordingly.

Mike.
Joseph Myers March 11, 2019, 11:13 p.m. UTC | #4
On Sun, 10 Mar 2019, Mike Crowe wrote:

> It would be straightforward to add them with support for CLOCK_REALTIME
> only, but I'm not sure that it really brings much value. Adding
> CLOCK_MONOTONIC support at the same time as the new functions would mean
> that autoconf scripts would know that CLOCK_MONOTONIC was supported if the
> functions were found, rather than having to fall back at runtime.

No, they wouldn't know that, if kernel support is needed for 
CLOCK_MONOTONIC support, because the kernel at runtime might be too old 
(glibc generally supports at least the oldest active LTS kernel series).
Mike Crowe March 13, 2019, 9:42 p.m. UTC | #5
On Tuesday 05 March 2019 at 09:35:01 -0300, Adhemerval Zanella wrote:
> On 27/02/2019 15:23, Mike Crowe wrote:
> > My attempts[1] to add a variant of pthread_cond_timedwait that would accept
> > a clockid_t parameter led me to propose[2] to The Austin Group the addition
> > of an entire family of functions that accept a clockid_t parameter to
> > indicate the clock that the timespec absolute timeout parameter should be
> > measured against. They responded positively to the request but an
> > implementation is required before the proposal can proceed.
> >
> > This patch series is the first part of that implementation in glibc, it
> > contains four new functions:
> > 
> > int pthread_cond_clockwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
> >                            clockid_t clock, const struct timespec *abstime)
> > 
> > int pthread_rwlock_clockrdlock(pthread_rwlock_t *rwlock, clockid_t clock,
> >                                const struct timespec *abstime)
> > 
> > int pthread_rwlock_clockwrlock(pthread_rwlock_t *rwlock, clockid_t clock,
> >                                const struct timespec *abstime)
> > 
> > int sem_clockwait(sem_t *restrict, clockid_t clock_id, const struct
> >                   timespec *restrict)
> > 
> > These are implemented by replacing the underlying equivalent _timed
> > functions with ones that accept a clockid_t parameter, and then
> > implementing the existing _timed functions by passing CLOCK_REALTIME to the
> > new implementation. This requires clockid_t parameters to be added to the
> > underlying futex-internal and lowlevellock-futex functions.
> 
> I am not sure about how to name a new API in this specific case where
> the idea is to use as a reference one for POSIX inclusion.  MY understanding 
> is we still should use a non-portable name scheme with _np suffix and whence
> it is de factor accepted by POSIX we can just alias to the standard
> names. Also if it is done before the release I think we can avoid exporting 
> the _np naming scheme.

Well, non-POSIX functions are added all over glibc without _np extensions.
:) It seems that the convention is used only for pthreads. I can rework the
patches to use the _np extension if required, but I'd hoped that __USE_GNU
was sufficient protection. Perhaps we should see what Joseph and others
think.

> > pthread_mutex_clocklock is not yet implemented because doing so requires a
> > version of __lll_timedlock_wait that supports both CLOCK_MONOTONIC and
> > CLOCK_REALTIME. This function is currently architecture specific. I plan to
> > work on that next, if these changes are considered acceptable.
> 
> About that I think we can work towards once my lowlevellock.S cleanup
> is accepted [4]. Once __lll_timedlock_wait is implemented in generic
> way it would be much more easier to add such extension.
> 
> [1] https://sourceware.org/ml/libc-alpha/2019-02/msg00569.html

That's great. I'll rebase my changes on top of that one before tackling
pthread_mutex_clocklock.

> > The mq_clockreceive and mq_clocksend functions corresponding to
> > mq_timedreceive and mq_timedsend require kernel changes before they can be
> > implemented in glibc.
> 
> Should we add them as ENOSYS as generic stubs in this version?
> Because it seems the kernel work required would be just add a new
> syscall on ipc/mqueue.c to receive a clock_id since 
> schedule_hrtimeout_range_clock already handle CLOCK_MONOTONIC
> (and it luckily fits on 6 argument syscall limit).

If the six argument limit is universal then that will be very helpful. I'd
previously (apparently incorrectly) scared myself into believing that the
functions were already at the limit. I shall dig some more, but these
functions are currently at the bottom of the list.

[snip stuff about errors for clocks that I have no strong feelings about]

> > A number of tests have been updated to use struct timespec rather than
> > struct timeval so that they can call clock_gettime(CLOCK_MONOTONIC) rather
> > then gettimeofday. To make this easier I created the support/timespec.h
> > header file to contain functions to add and subtract timespec structures
> > borrowed from sysdeps/pthread/posix-timer.h. There's probably a better
> > place for these, but I don't know where that might be.
> 
> I think you forgot to update this part since you seemed to had chose
> to use support/timespec.h instead.

Indeed.

Thanks for all your comments.

Mike.
Adhemerval Zanella March 14, 2019, 11:30 a.m. UTC | #6
On 13/03/2019 18:42, Mike Crowe wrote:
> On Tuesday 05 March 2019 at 09:35:01 -0300, Adhemerval Zanella wrote:
>> On 27/02/2019 15:23, Mike Crowe wrote:
>>> My attempts[1] to add a variant of pthread_cond_timedwait that would accept
>>> a clockid_t parameter led me to propose[2] to The Austin Group the addition
>>> of an entire family of functions that accept a clockid_t parameter to
>>> indicate the clock that the timespec absolute timeout parameter should be
>>> measured against. They responded positively to the request but an
>>> implementation is required before the proposal can proceed.
>>>
>>> This patch series is the first part of that implementation in glibc, it
>>> contains four new functions:
>>>
>>> int pthread_cond_clockwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
>>>                            clockid_t clock, const struct timespec *abstime)
>>>
>>> int pthread_rwlock_clockrdlock(pthread_rwlock_t *rwlock, clockid_t clock,
>>>                                const struct timespec *abstime)
>>>
>>> int pthread_rwlock_clockwrlock(pthread_rwlock_t *rwlock, clockid_t clock,
>>>                                const struct timespec *abstime)
>>>
>>> int sem_clockwait(sem_t *restrict, clockid_t clock_id, const struct
>>>                   timespec *restrict)
>>>
>>> These are implemented by replacing the underlying equivalent _timed
>>> functions with ones that accept a clockid_t parameter, and then
>>> implementing the existing _timed functions by passing CLOCK_REALTIME to the
>>> new implementation. This requires clockid_t parameters to be added to the
>>> underlying futex-internal and lowlevellock-futex functions.
>>
>> I am not sure about how to name a new API in this specific case where
>> the idea is to use as a reference one for POSIX inclusion.  MY understanding 
>> is we still should use a non-portable name scheme with _np suffix and whence
>> it is de factor accepted by POSIX we can just alias to the standard
>> names. Also if it is done before the release I think we can avoid exporting 
>> the _np naming scheme.
> 
> Well, non-POSIX functions are added all over glibc without _np extensions.
> :) It seems that the convention is used only for pthreads. I can rework the
> patches to use the _np extension if required, but I'd hoped that __USE_GNU
> was sufficient protection. Perhaps we should see what Joseph and others
> think.

Indeed I was using the current practice for pthreads extension, but I if 
the idea is to use the same naming for a future POSIX inclusion I think
that guarding with __USE_GNU should be suffice.

> 
>>> pthread_mutex_clocklock is not yet implemented because doing so requires a
>>> version of __lll_timedlock_wait that supports both CLOCK_MONOTONIC and
>>> CLOCK_REALTIME. This function is currently architecture specific. I plan to
>>> work on that next, if these changes are considered acceptable.
>>
>> About that I think we can work towards once my lowlevellock.S cleanup
>> is accepted [4]. Once __lll_timedlock_wait is implemented in generic
>> way it would be much more easier to add such extension.
>>
>> [1] https://sourceware.org/ml/libc-alpha/2019-02/msg00569.html
> 
> That's great. I'll rebase my changes on top of that one before tackling
> pthread_mutex_clocklock.

I will try to get this upstream as well.

> 
>>> The mq_clockreceive and mq_clocksend functions corresponding to
>>> mq_timedreceive and mq_timedsend require kernel changes before they can be
>>> implemented in glibc.
>>
>> Should we add them as ENOSYS as generic stubs in this version?
>> Because it seems the kernel work required would be just add a new
>> syscall on ipc/mqueue.c to receive a clock_id since 
>> schedule_hrtimeout_range_clock already handle CLOCK_MONOTONIC
>> (and it luckily fits on 6 argument syscall limit).
> 
> If the six argument limit is universal then that will be very helpful. I'd
> previously (apparently incorrectly) scared myself into believing that the
> functions were already at the limit. I shall dig some more, but these
> functions are currently at the bottom of the list.

Strictly speaking it depends of the kABI, but afaik from the support ones
only MIPS-n32 actually uses 7 arguments syscalls. I think we can safely
assume all current ABIs support 6 arguments syscalls.

> 
> [snip stuff about errors for clocks that I have no strong feelings about]
> 
>>> A number of tests have been updated to use struct timespec rather than
>>> struct timeval so that they can call clock_gettime(CLOCK_MONOTONIC) rather
>>> then gettimeofday. To make this easier I created the support/timespec.h
>>> header file to contain functions to add and subtract timespec structures
>>> borrowed from sysdeps/pthread/posix-timer.h. There's probably a better
>>> place for these, but I don't know where that might be.
>>
>> I think you forgot to update this part since you seemed to had chose
>> to use support/timespec.h instead.
> 
> Indeed.
> 
> Thanks for all your comments.
> 
> Mike.
>
Yann Droneaud March 15, 2019, 1:25 p.m. UTC | #7
Hi,

Le mercredi 27 février 2019 à 18:23 +0000, Mike Crowe a écrit :
> My attempts[1] to add a variant of pthread_cond_timedwait that would accept
> a clockid_t parameter led me to propose[2] to The Austin Group the addition
> of an entire family of functions that accept a clockid_t parameter to
> indicate the clock that the timespec absolute timeout parameter should be
> measured against. They responded positively to the request but an
> implementation is required before the proposal can proceed.
> 
> This patch series is the first part of that implementation in glibc, it
> contains four new functions:
> 
> int pthread_cond_clockwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
>                            clockid_t clock, const struct timespec *abstime)
> 
> int pthread_rwlock_clockrdlock(pthread_rwlock_t *rwlock, clockid_t clock,
>                                const struct timespec *abstime)
> 
> int pthread_rwlock_clockwrlock(pthread_rwlock_t *rwlock, clockid_t clock,
>                                const struct timespec *abstime)
> 
> int sem_clockwait(sem_t *restrict, clockid_t clock_id, const struct
>                   timespec *restrict)
> 

There's also a pthread_timedjoin_np() that could benefit from having
"pthread_clockjoin_np()" sibling.

Regards.
Mike Crowe March 15, 2019, 1:36 p.m. UTC | #8
On Friday 15 March 2019 at 14:25:43 +0100, Yann Droneaud wrote:
> There's also a pthread_timedjoin_np() that could benefit from having
> "pthread_clockjoin_np()" sibling.

Yes. That's on my list to get to, but since it's not a POSIX function I
thought it best to get the others done first.

Mike.