This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3 1/6] nptl: Add clockid parameter to futex timed wait calls
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org, Mike Crowe <mac at mcrowe dot com>
- Date: Wed, 12 Jun 2019 16:32:08 -0300
- Subject: Re: [PATCH v3 1/6] nptl: Add clockid parameter to futex timed wait calls
- References: <cover.206947992e95511c1c56165c9aa98ae1a2296b75.1558987219.git-series.mac@mcrowe.com> <95894abea465528f7bd5f8c0971c80f0ccdd73f7.1558987219.git-series.mac@mcrowe.com> <1e09d801-ad10-2d2b-a2f2-53315478b001@linaro.org>
On 05/06/2019 14:52, Adhemerval Zanella wrote:
>> diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c
>> index 9a0f29e..7385562 100644
>> --- a/nptl/pthread_cond_wait.c
>> +++ b/nptl/pthread_cond_wait.c
>> @@ -509,35 +509,15 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
>> values despite them being valid. */
>> if (__glibc_unlikely (abstime->tv_sec < 0))
>> err = ETIMEDOUT;
>> -
>> - else if ((flags & __PTHREAD_COND_CLOCK_MONOTONIC_MASK) != 0)
>> - {
>> - /* CLOCK_MONOTONIC is requested. */
>> - struct timespec rt;
>> - if (__clock_gettime (CLOCK_MONOTONIC, &rt) != 0)
>> - __libc_fatal ("clock_gettime does not support "
>> - "CLOCK_MONOTONIC\n");
>> - /* Convert the absolute timeout value to a relative
>> - timeout. */
>> - rt.tv_sec = abstime->tv_sec - rt.tv_sec;
>> - rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
>> - if (rt.tv_nsec < 0)
>> - {
>> - rt.tv_nsec += 1000000000;
>> - --rt.tv_sec;
>> - }
>> - /* Did we already time out? */
>> - if (__glibc_unlikely (rt.tv_sec < 0))
>> - err = ETIMEDOUT;
>> - else
>> - err = futex_reltimed_wait_cancelable
>> - (cond->__data.__g_signals + g, 0, &rt, private);
>> - }
>> else
>> {
>> - /* Use CLOCK_REALTIME. */
>> + const clockid_t clockid =
>> + ((flags & __PTHREAD_COND_CLOCK_MONOTONIC_MASK) != 0) ?
>> + CLOCK_MONOTONIC : CLOCK_REALTIME;
>> +
>> err = futex_abstimed_wait_cancelable
>> - (cond->__data.__g_signals + g, 0, abstime, private);
>> + (cond->__data.__g_signals + g, 0, clockid, abstime,
>> + private);
>> }
>> }
>
> My understanding is we need to convert an absolute timeout to a relative one
> on either futex_abstimed_wait_cancelable or lll_futex_clock_wait_bitset for
> CLOCK_MONOTONIC.
Ok rechecking the patchset, it now uses futex_abstimed_wait_cancelable instead
of futex_reltimed_wait_cancelable, so my comment indeed does not apply.
However the naming is somewhat confusing because 'abstimed' only applies for
clockid being CLOCK_MONOTONIC. I think we should just name the new interfaces
as futex_timed_wait{_cancelable}.
As a side node maybe we could simplify the futex_{abs,rel}timed_wait to call
futex_timed_wait with expected clock on Linux implementation.