Re: [PATCH 0/7] Implement proposed POSIX _clockwait variants of existing _timedwait functions

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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
>