Re: [PATCH 2/2] nptl: Add pthread_clockjoin_np

[Mike Crowe <mac at mcrowe dot com>]

On Friday 28 June 2019 at 15:37:20 +0200, Yann Droneaud wrote:
> Le vendredi 28 juin 2019 à 13:13 +0100, Mike Crowe a écrit :
> > Introduce pthread_clockjoin_np as a version of pthread_timedjoin_np that
> > accepts a clockid_t parameter to indicate which clock the timeout should be
> > measured against. This mirrors the recently-added POSIX-proposed "clock"
> > wait functions.

[snip]

> > diff --git a/nptl/tst-join3.c b/nptl/tst-join3.c
> > index 460b862..2471de0 100644
> > --- a/nptl/tst-join3.c
> > +++ b/nptl/tst-join3.c
> > @@ -28,6 +28,8 @@
> >  #include <support/xtime.h>
> >  
> >  
> > +#define CLOCK_USE_TIMEDJOIN (-1)
> > +
> >  static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
> >  
> >  
> > @@ -35,19 +37,23 @@ static void *
> >  tf (void *arg)
> >  {
> >    xpthread_mutex_lock (&lock);
> > +  xpthread_mutex_unlock (&lock);
> >  
> >    return (void *) 42l;
> >  }
> >  
> >  
> >  static int
> > -do_test (void)
> > +do_test_clock (clockid_t clockid)
> >  {
> > +  const clockid_t clockid_for_get =
> > +    (clockid == CLOCK_USE_TIMEDJOIN) ? CLOCK_REALTIME : clockid;
> 
> I would prefer do_test_clock() having another parameter to enable usage
> of pthread_clockjoin_np() than having a "fake" clock identifier used as
> a boolean.

The fake clockid is the form I've used in all the other tests I added for
the new clocklock/clockwait functions in glibc v2.30. It's not really being
used as a boolean. Using a separate boolean would not be orthogonal to the
clockid - it would permit conflicting parameters to be specified where the
boolean could indicate that pthread_timedjoin_np should be tested with
CLOCK_MONOTONIC.

[snip]

> > diff --git a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h
> > index a767d6f..00d996c 100644
> > --- a/sysdeps/nptl/pthread.h
> > +++ b/sysdeps/nptl/pthread.h
> > @@ -263,6 +263,17 @@ extern int pthread_tryjoin_np (pthread_t __th, void **__thread_return) __THROW;
> >     __THROW.  */
> >  extern int pthread_timedjoin_np (pthread_t __th, void **__thread_return,
> >  				 const struct timespec *__abstime);
> > +
> > +/* Make calling thread wait for termination of the thread TH, but only
> > +   until TIMEOUT measured against the clock specified by CLOCKID.  The
> > +   exit status of the thread is stored in *THREAD_RETURN, if
> > +   THREAD_RETURN is not NULL.
> > +
> > +   This function is a cancellation point and therefore not marked with
> > +   __THROW.  */
> > +extern int pthread_clockjoin_np (pthread_t __th, void **__thread_return,
> > +                                 clockid_t clockid,
> > +				 const struct timespec *__abstime);
> 
> What's the behavior when __abstime is NULL ?

Currently it's the same as pthread_timedjoin_np. I shall add a test for
that to ensure that it isn't broken.

> pthread_timedjoin_np()'s code seems to behave like pthread_join() if
> __abstime is NULL. This behavior is fine but not documented.

The old LinuxThreads phread_timedjoin_np man page doesn't mention what
happens when __abstime is NULL. There don't appear to be any NPTL man
pages. :(

pthread_timedjoin_np isn't currently documented in the glibc threads.texi
file. I will try to write some documentation there for it. It's too late to
change the behaviour now, of course.

For pthread_clockjoin_np there's no backward compatibility problem, so I
could make it behave the same as pthread_timedjoin_np or always return
EINVAL. I'm probably inclined to keep it the same.

Thanks for the review.

Mike.