[v3,4/6] nptl: pthread_rwlock: Move timeout validation into _full functions

  As recommended by the comments in the implementations of
pthread_rwlock_timedrdlock and pthread_rwlock_timedwrlock, let's move the
timeout validity checks into the corresponding pthread_rwlock_rdlock_full
and pthread_rwlock_wrlock_full functions. Since these functions may be
called with abstime == NULL, an extra check for that is necessary too.

	* nptl/pthread_rwlock_common.c (__pthread_rwlock_rdlock_full):
	Check validity of abstime parameter.
	(__pthread_rwlock_rwlock_full): Likewise.

	* nptl/pthread_rwlock_timedrdlock.c (pthread_rwlock_timedrdlock):
	Remove check for validity of abstime parameter.

	* nptl/pthread_rwlock_timedwrlock.c (pthread_rwlock_timedwrlock):
	Likewise.
---
 ChangeLog                         | 14 ++++++++++++++
 nptl/pthread_rwlock_common.c      | 20 ++++++++++++++++++++
 nptl/pthread_rwlock_timedrdlock.c | 10 ----------
 nptl/pthread_rwlock_timedwrlock.c | 10 ----------
 4 files changed, 34 insertions(+), 20 deletions(-)
  

On 27/05/2019 17:03, Mike Crowe wrote:
> As recommended by the comments in the implementations of
> pthread_rwlock_timedrdlock and pthread_rwlock_timedwrlock, let's move the
> timeout validity checks into the corresponding pthread_rwlock_rdlock_full
> and pthread_rwlock_wrlock_full functions. Since these functions may be
> called with abstime == NULL, an extra check for that is necessary too.

Is this really a gain here? It just moves some code around and add
an extra non required check. Wouldn't be better just to remove the
comments?

> 
> 	* nptl/pthread_rwlock_common.c (__pthread_rwlock_rdlock_full):
> 	Check validity of abstime parameter.
> 	(__pthread_rwlock_rwlock_full): Likewise.
> 
> 	* nptl/pthread_rwlock_timedrdlock.c (pthread_rwlock_timedrdlock):
> 	Remove check for validity of abstime parameter.
> 
> 	* nptl/pthread_rwlock_timedwrlock.c (pthread_rwlock_timedwrlock):
> 	Likewise.
> ---
>  ChangeLog                         | 14 ++++++++++++++
>  nptl/pthread_rwlock_common.c      | 20 ++++++++++++++++++++
>  nptl/pthread_rwlock_timedrdlock.c | 10 ----------
>  nptl/pthread_rwlock_timedwrlock.c | 10 ----------
>  4 files changed, 34 insertions(+), 20 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 1f95dd4..22a8bdc 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,19 @@
>  2019-05-27  Mike Crowe  <mac@mcrowe.com>
>  
> +	nptl: pthread_rwlock: Move timeout validation into _full functions
> +
> +	* nptl/pthread_rwlock_common.c (__pthread_rwlock_rdlock_full):
> +	Check validity of abstime parameter.
> +	(__pthread_rwlock_rwlock_full): Likewise.
> +
> +	* nptl/pthread_rwlock_timedrdlock.c (pthread_rwlock_timedrdlock):
> +	Remove check for validity of abstime parameter.
> +
> +	* nptl/pthread_rwlock_timedwrlock.c (pthread_rwlock_timedwrlock):
> +	Likewise.
> +
> +2019-05-27  Mike Crowe  <mac@mcrowe.com>
> +
>  	nptl: Add POSIX-proposed pthread_cond_clockwait which behaves just
>  	like pthread_cond_timedwait except it always measures abstime
>  	against the supplied clockid.
> diff --git a/nptl/pthread_rwlock_common.c b/nptl/pthread_rwlock_common.c
> index 89ba21a..120b880 100644
> --- a/nptl/pthread_rwlock_common.c
> +++ b/nptl/pthread_rwlock_common.c
> @@ -282,6 +282,16 @@ __pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock,
>  {
>    unsigned int r;
>  
> +  /* Make sure any passed in timeout value is valid.  Note that the previous
> +     implementation assumed that this check *must* not be performed if there
> +     would in fact be no blocking; however, POSIX only requires that "the
> +     validity of the abstime parameter need not be checked if the lock can be
> +     immediately acquired" (i.e., we need not but may check it).  */
> +  if (abstime
> +      && __glibc_unlikely (abstime->tv_nsec >= 1000000000
> +      || abstime->tv_nsec < 0))
> +    return EINVAL;
> +
>    /* Make sure we are not holding the rwlock as a writer.  This is a deadlock
>       situation we recognize and report.  */
>    if (__glibc_unlikely (atomic_load_relaxed (&rwlock->__data.__cur_writer)
> @@ -576,6 +586,16 @@ static __always_inline int
>  __pthread_rwlock_wrlock_full (pthread_rwlock_t *rwlock,
>      const struct timespec *abstime)
>  {
> +  /* Make sure any passed in timeout value is valid.  Note that the previous
> +     implementation assumed that this check *must* not be performed if there
> +     would in fact be no blocking; however, POSIX only requires that "the
> +     validity of the abstime parameter need not be checked if the lock can be
> +     immediately acquired" (i.e., we need not but may check it).  */
> +  if (abstime
> +      && __glibc_unlikely (abstime->tv_nsec >= 1000000000
> +      || abstime->tv_nsec < 0))
> +    return EINVAL;
> +
>    /* Make sure we are not holding the rwlock as a writer.  This is a deadlock
>       situation we recognize and report.  */
>    if (__glibc_unlikely (atomic_load_relaxed (&rwlock->__data.__cur_writer)
> diff --git a/nptl/pthread_rwlock_timedrdlock.c b/nptl/pthread_rwlock_timedrdlock.c
> index aa00530..84c1983 100644
> --- a/nptl/pthread_rwlock_timedrdlock.c
> +++ b/nptl/pthread_rwlock_timedrdlock.c
> @@ -23,15 +23,5 @@ int
>  pthread_rwlock_timedrdlock (pthread_rwlock_t *rwlock,
>      const struct timespec *abstime)
>  {
> -  /* Make sure the passed in timeout value is valid.  Note that the previous
> -     implementation assumed that this check *must* not be performed if there
> -     would in fact be no blocking; however, POSIX only requires that "the
> -     validity of the abstime parameter need not be checked if the lock can be
> -     immediately acquired" (i.e., we need not but may check it).  */
> -  /* ??? Just move this to __pthread_rwlock_rdlock_full?  */
> -  if (__glibc_unlikely (abstime->tv_nsec >= 1000000000
> -      || abstime->tv_nsec < 0))
> -    return EINVAL;
> -
>    return __pthread_rwlock_rdlock_full (rwlock, abstime);
>  }
> diff --git a/nptl/pthread_rwlock_timedwrlock.c b/nptl/pthread_rwlock_timedwrlock.c
> index 3c92e44..f0b745d 100644
> --- a/nptl/pthread_rwlock_timedwrlock.c
> +++ b/nptl/pthread_rwlock_timedwrlock.c
> @@ -23,15 +23,5 @@ int
>  pthread_rwlock_timedwrlock (pthread_rwlock_t *rwlock,
>      const struct timespec *abstime)
>  {
> -  /* Make sure the passed in timeout value is valid.  Note that the previous
> -     implementation assumed that this check *must* not be performed if there
> -     would in fact be no blocking; however, POSIX only requires that "the
> -     validity of the abstime parameter need not be checked if the lock can be
> -     immediately acquired" (i.e., we need not but may check it).  */
> -  /* ??? Just move this to __pthread_rwlock_wrlock_full?  */
> -  if (__glibc_unlikely (abstime->tv_nsec >= 1000000000
> -      || abstime->tv_nsec < 0))
> -    return EINVAL;
> -
>    return __pthread_rwlock_wrlock_full (rwlock, abstime);
>  }
>
  

On Thursday 06 June 2019 at 16:18:05 -0300, Adhemerval Zanella wrote:
> 
> 
> On 27/05/2019 17:03, Mike Crowe wrote:
> > As recommended by the comments in the implementations of
> > pthread_rwlock_timedrdlock and pthread_rwlock_timedwrlock, let's move the
> > timeout validity checks into the corresponding pthread_rwlock_rdlock_full
> > and pthread_rwlock_wrlock_full functions. Since these functions may be
> > called with abstime == NULL, an extra check for that is necessary too.
> 
> Is this really a gain here? It just moves some code around and add
> an extra non required check. Wouldn't be better just to remove the
> comments?

It looks like the comment was added by Torvald Riegel in
cc25c8b4c1196a8c29e9a45b1e096b99a87b7f8c back in 2017.

From what I can tell from the x86_64 disassembly, the compiler throws away the extra check because
these functions are all inline and it knows that abstime will be NULL.

However, I'm perfectly happy not to make this change, but it means that
I'll need to add the same checks to pthread_rwlock_clockwrlock and
pthread_wlock_clockrdlock.

I think that you've suggested consolidating these checks into a single
function in the past, which would make that easier. How about adding
is_valid_timespec or similar? Where would it be declared?

Thanks.

Mike.
  

On 07/06/2019 14:01, Mike Crowe wrote:
> On Thursday 06 June 2019 at 16:18:05 -0300, Adhemerval Zanella wrote:
>>
>>
>> On 27/05/2019 17:03, Mike Crowe wrote:
>>> As recommended by the comments in the implementations of
>>> pthread_rwlock_timedrdlock and pthread_rwlock_timedwrlock, let's move the
>>> timeout validity checks into the corresponding pthread_rwlock_rdlock_full
>>> and pthread_rwlock_wrlock_full functions. Since these functions may be
>>> called with abstime == NULL, an extra check for that is necessary too.
>>
>> Is this really a gain here? It just moves some code around and add
>> an extra non required check. Wouldn't be better just to remove the
>> comments?
> 
> It looks like the comment was added by Torvald Riegel in
> cc25c8b4c1196a8c29e9a45b1e096b99a87b7f8c back in 2017.
> 
> From what I can tell from the x86_64 disassembly, the compiler throws away the extra check because
> these functions are all inline and it knows that abstime will be NULL.
> 
> However, I'm perfectly happy not to make this change, but it means that
> I'll need to add the same checks to pthread_rwlock_clockwrlock and
> pthread_wlock_clockrdlock.
> 
> I think that you've suggested consolidating these checks into a single
> function in the past, which would make that easier. How about adding
> is_valid_timespec or similar? Where would it be declared?

It seems we have multiple check that might be redundant, although it would
be likely removed from compiler.  On __pthread_rwlock_rdlock_full we have
the abstime check and futex_abstimed_wait, also present on futex_abstimed_wait.

However I think this should be ok and if we eventually make 
__pthread_rwlock_wrlock_full we can rearrange the checks to avoid
useless one.