Re: [PATCH v3 1/6] nptl: Add clockid parameter to futex timed wait calls

[Mike Crowe <mac at mcrowe dot com>]

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.

> 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.