Re: [PATCH v3 2/6] nptl: Add POSIX-proposed sem_clockwait

[Mike Crowe <mac at mcrowe dot com>]

On Wednesday 05 June 2019 at 18:07:18 -0300, Adhemerval Zanella wrote:
> On 27/05/2019 17:03, Mike Crowe wrote:
> > Add:
> > 
> >  int sem_clockwait (sem_t *sem, clockid_t clock, const struct timespec *abstime)
> > 
> > which behaves just like sem_timedwait, but measures abstime against the
> > specified clock. Currently supports CLOCK_REALTIME and CLOCK_MONOTONIC and
> > sets errno == EINVAL if any other clock is specified.
> 
> For non-POSIX definition we will need to first export it as a GNU extension
> with a possible non-reserved name and later, when it is included on the 
> standard, to add an alias to the expected name.  
> 
> The usual naming scheme is to append the _np suffix (non-portable) on 
> implementation, similar to recent posix_spawn file action extensions.
> In this case it would be sem_clockwait_np.

Hi Adhemerval,

I thought we went through this before[1] and agreed that the _np suffix wasn't
necessary in this case. However, I couldn't find a reply to Florian
agreeing to that.

[1] https://marc.info/?l=glibc-alpha&m=155256303513500

[snip other helpful review comments that I will work on]

> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <http://www.gnu.org/licenses/>.  */
> > +
> > +#include "sem_waitcommon.c"
> > +
> > +int
> > +sem_clockwait (sem_t *sem, clockid_t clockid,
> > +	       const struct timespec *abstime)
> > +{
> > +  /* Check that supplied clockid is one we support, even if we don't
> > +     end up waiting. */
> > +  if (!futex_abstimed_supported_clockid (clockid))
> > +    {
> > +      __set_errno (EINVAL);
> > +      return -1;
> > +    }
> > +
> > +  if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
> > +    {
> > +      __set_errno (EINVAL);
> > +      return -1;
> > +    }
> > +
> > +  if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0)
> > +    return 0;
> > +  else
> > +    return __new_sem_wait_slow ((struct new_sem *) sem, clockid, abstime);
> > +}
> 
> Ok.  As for BZ#23006 I think we should follow Austin Group latest resolution
> for sem_wait/sem_timedwait and not make it a cancellable entrypoint.

I think that's what I've done here (perhaps purely by accident because I'd
probably copied the code prior to
47677f2edc815e85d0383a89b09733e95e5d7302.) Would you like me to add a
comment to describe why there's no call to __pthread_test_cancel() here?

> > diff --git a/nptl/tst-sem5.c b/nptl/tst-sem5.c
> > index 396222b..843839b 100644
> > --- a/nptl/tst-sem5.c
> > +++ b/nptl/tst-sem5.c
> > @@ -25,10 +25,15 @@
> >  #include <support/timespec.h>
> >  #include <support/xtime.h>
> >  
> > +/* A bogus clock value that tells run_test to use
> > +   sem_timedwait rather than sem_clockwait */
> > +#define CLOCK_USE_TIMEDWAIT (-1)
> 
> Maybe use a enum instead of a magic constant ?

Do you mean just:

enum { CLOCK_USE_TIMEDWAIT = -1 };

or something like:

enum
{
  CLOCK_MONOTONIC_ = CLOCK_MONOTONIC,
  CLOCK_REALTIME_ = CLOCK_REALTIME,
  CLOCK_USE_TIMEDWAIT
};

in attempt to ensure that CLOCK_USE_TIMEDWAIT doesn't overlap? I can't see
how this can work with potentially-arbitrary values for CLOCK_MONOTONIC and
CLOCK_REALTIME. This would fail if CLOCK_REALTIME=42 and
CLOCK_MONOTONIC=43.

Since glibc is responsible declaring CLOCK_MONOTONIC and CLOCK_REALTIME, and can't change
them without breaking ABI, I thought -1 would be safe. If we're worried, I
could add explicit checks that CLOCK_USE_TIMEDWAIT doesn't have the same
value as CLOCK_MONOTONIC or CLOCK_REALTIME.

Thanks.

Mike.