This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3 4/6] nptl: pthread_rwlock: Move timeout validation into _full functions
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Thu, 6 Jun 2019 16:18:05 -0300
- Subject: Re: [PATCH v3 4/6] nptl: pthread_rwlock: Move timeout validation into _full functions
- References: <cover.206947992e95511c1c56165c9aa98ae1a2296b75.1558987219.git-series.mac@mcrowe.com> <c1ca08fdcb05d29a98dd118e618e91a5be1d2439.1558987219.git-series.mac@mcrowe.com>
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);
> }
>