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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 06/06/2019 12:40, Mike Crowe wrote:
> 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]

I take it is rule of thumb to add names different than the expected one from
standard, but I don't have a strong opinion here.

The posix_spawn, for instance, were added to mimic the other system names 
(Solaris in this case) and the name originally were added with the extension 
to make it explicit it a non-portable extension. But Solaris also did not
seem to work towards to make it POSIX standard, so it might the reason to
it.

> 
>>> +
>>> +   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?

Yes, please do.

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

Yeah, I think this over-engineering is not really required.

> 
> Thanks.
> 
> Mike.
>