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
On 18/06/2019 08:44, Mike Crowe wrote:
> On Wednesday 12 June 2019 at 16:32:08 -0300, Adhemerval Zanella wrote:
>> 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}.
>
> I'm afraid that I don't understand this paragraph. I don't believe that
> I've added a new interface here (well, apart from the
> futex_abstimed_supported_clockid function.)
>
> futex_abstimed_wait{,_cancelable} always take an absolute timeout measured
> against the specified clock. They end up calling
> lll_futex_clock_wait_bitset, which passes that absolute timeout directly to
> the futex syscall. It is not converted to a relative timeout at any point.
It was a confusion from my part, sorry for the noise. I withdrew my suggestion
here, the change looks ok thanks.
>
>> 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.
>
> This confuses me too, but that is probably due to my earlier confusion.
>
> Thanks.
>
> Mike.
>