[6/7] nptl/tst-rwlock: Use clock_gettime/timespec rather than gettimeofday/timeval

  In preparation for adding pthread_rwlock_clockrdlock and
pthread_rwlock_clockwrlock, convert various tests to only use clock_gettime
and struct timespec.

* support/timespec.h: Create header to provide timespec helper functions
  from sysdeps/pthread/posix-timer.h for tests to use.

* nptl/tst-rwlock6.c: Fix small bug in timeout-checking code that could
  erroneously pass if the function incorrectly took more than a second.

* nptl/tst-rwlock6.c: Use clock_gettime(2) rather than gettimeofday(2) and
  then converting to timespec in preparation for testing
  pthread_rwlock_clockrdclock and pthread_rwlock_clockwrlock.

* nptl/tst-rwlock9.c, nptl/tst-rwlock7.c: Likewise.
---
 nptl/tst-rwlock6.c | 47 ++++++++++++++++++-----------------------------
 nptl/tst-rwlock7.c | 43 +++++++++++++++++--------------------------
 nptl/tst-rwlock9.c |  8 ++------
 support/timespec.h | 27 +++++++++++++++++++++++++++-
 4 files changed, 64 insertions(+), 61 deletions(-)
 create mode 100644 support/timespec.h
  

On 27/02/2019 15:23, Mike Crowe wrote:
> In preparation for adding pthread_rwlock_clockrdlock and
> pthread_rwlock_clockwrlock, convert various tests to only use clock_gettime
> and struct timespec.
> 
> * support/timespec.h: Create header to provide timespec helper functions
>   from sysdeps/pthread/posix-timer.h for tests to use.
> 
> * nptl/tst-rwlock6.c: Fix small bug in timeout-checking code that could
>   erroneously pass if the function incorrectly took more than a second.
> 
> * nptl/tst-rwlock6.c: Use clock_gettime(2) rather than gettimeofday(2) and
>   then converting to timespec in preparation for testing
>   pthread_rwlock_clockrdclock and pthread_rwlock_clockwrlock.
> 
> * nptl/tst-rwlock9.c, nptl/tst-rwlock7.c: Likewise.

I am seeing this issue sporadically on i686-linux-gnu with 6/7 patches
applied:

$ ./testrun.sh nptl/tst-rwlock7 --direct
0: got timedrdlock
child: timedwrlock failed with ETIMEDOUT
child: timedwrlock failed with EINVAL
1: got timedrdlock
child: timedwrlock failed with ETIMEDOUT
child: timedwrlock failed with EINVAL
2: got timedrdlock
child: timedwrlock failed with ETIMEDOUT
2nd timedwrlock did not return EINVAL
failure in round 2


> ---
>  nptl/tst-rwlock6.c | 47 ++++++++++++++++++-----------------------------
>  nptl/tst-rwlock7.c | 43 +++++++++++++++++--------------------------
>  nptl/tst-rwlock9.c |  8 ++------
>  support/timespec.h | 27 +++++++++++++++++++++++++++-
>  4 files changed, 64 insertions(+), 61 deletions(-)
>  create mode 100644 support/timespec.h
> 
> diff --git a/nptl/tst-rwlock6.c b/nptl/tst-rwlock6.c
> index 8d6c3dc..67119fa 100644
> --- a/nptl/tst-rwlock6.c
> +++ b/nptl/tst-rwlock6.c
> @@ -22,6 +22,7 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <sys/time.h>
> +#include <support/timespec.h>
>  
>  
>  static int kind[] =
> @@ -38,21 +39,15 @@ tf (void *arg)
>    pthread_rwlock_t *r = arg;
>  
>    /* Timeout: 0.3 secs.  */
> -  struct timeval tv;
> -  (void) gettimeofday (&tv, NULL);
> +  struct timespec ts_start;
> +  (void) clock_gettime(CLOCK_REALTIME, &ts_start);

Space after symbol. Also I think there is no need for (void) cast here.

>  
> -  struct timespec ts;
> -  TIMEVAL_TO_TIMESPEC (&tv, &ts);
> -  ts.tv_nsec += 300000000;
> -  if (ts.tv_nsec >= 1000000000)
> -    {
> -      ts.tv_nsec -= 1000000000;
> -      ++ts.tv_sec;
> -    }
> +  struct timespec ts_timeout = {0, 300000000};
> +  timespec_add(&ts_timeout, &ts_start, &ts_timeout);

Ditto.

>  
>    puts ("child calling timedrdlock");
>  
> -  int err = pthread_rwlock_timedrdlock (r, &ts);
> +  int err = pthread_rwlock_timedrdlock (r, &ts_timeout);
>    if (err == 0)
>      {
>        puts ("rwlock_timedrdlock returned");
> @@ -68,24 +63,24 @@ tf (void *arg)
>  
>    puts ("1st child timedrdlock done");
>  
> -  struct timeval tv2;
> -  (void) gettimeofday (&tv2, NULL);
> +  struct timespec ts_end;
> +  (void) clock_gettime (CLOCK_REALTIME, &ts_end);
>  
> -  timersub (&tv2, &tv, &tv);
> +  struct timespec ts_duration;
> +  timespec_sub(&ts_duration, &ts_end, &ts_start);

Ditto.

>  
> -  if (tv.tv_usec < 200000)
> +  if (ts_duration.tv_sec !=0 || ts_duration.tv_nsec < 200000000)

After before !=.

>      {
>        puts ("timeout too short");
>        pthread_exit ((void *) 1l);
>      }
>  
> -  (void) gettimeofday (&tv, NULL);
> -  TIMEVAL_TO_TIMESPEC (&tv, &ts);
> -  ts.tv_sec += 10;
> +  (void) clock_gettime (CLOCK_REALTIME, &ts_timeout);
> +  ts_timeout.tv_sec += 10;
>    /* Note that the following operation makes ts invalid.  */
> -  ts.tv_nsec += 1000000000;
> +  ts_timeout.tv_nsec += 1000000000;
>  
> -  err = pthread_rwlock_timedrdlock (r, &ts);
> +  err = pthread_rwlock_timedrdlock (r, &ts_timeout);
>    if (err == 0)
>      {
>        puts ("2nd timedrdlock succeeded");
> @@ -136,12 +131,8 @@ do_test (void)
>  	  exit (1);
>  	}
>  
> -      struct timeval tv;
> -      (void) gettimeofday (&tv, NULL);
> -
>        struct timespec ts;
> -      TIMEVAL_TO_TIMESPEC (&tv, &ts);
> -
> +      (void) clock_gettime (CLOCK_REALTIME, &ts);
>        ++ts.tv_sec;
>  
>        /* Get a write lock.  */
> @@ -154,8 +145,7 @@ do_test (void)
>  
>        puts ("1st timedwrlock done");
>  
> -      (void) gettimeofday (&tv, NULL);
> -      TIMEVAL_TO_TIMESPEC (&tv, &ts);
> +      (void) clock_gettime (CLOCK_REALTIME, &ts);
>        ++ts.tv_sec;
>        e = pthread_rwlock_timedrdlock (&r, &ts);
>        if (e == 0)
> @@ -171,8 +161,7 @@ do_test (void)
>  
>        puts ("1st timedrdlock done");
>  
> -      (void) gettimeofday (&tv, NULL);
> -      TIMEVAL_TO_TIMESPEC (&tv, &ts);
> +      (void) clock_gettime (CLOCK_REALTIME, &ts);
>        ++ts.tv_sec;
>        e = pthread_rwlock_timedwrlock (&r, &ts);
>        if (e == 0)

Ok.

> diff --git a/nptl/tst-rwlock7.c b/nptl/tst-rwlock7.c
> index 4d6f561..812506c 100644
> --- a/nptl/tst-rwlock7.c
> +++ b/nptl/tst-rwlock7.c
> @@ -22,6 +22,7 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <sys/time.h>
> +#include <support/timespec.h>
>  
>  
>  static int kind[] =
> @@ -38,19 +39,12 @@ tf (void *arg)
>    pthread_rwlock_t *r = arg;
>  
>    /* Timeout: 0.3 secs.  */
> -  struct timeval tv;
> -  (void) gettimeofday (&tv, NULL);
> +  struct timespec ts_start;
> +  (void) clock_gettime (CLOCK_REALTIME, &ts_start);
> +  struct timespec ts_timeout = {0, 300000000};
> +  timespec_add(&ts_timeout, &ts_start, &ts_timeout);

Ditto.

>  
> -  struct timespec ts;
> -  TIMEVAL_TO_TIMESPEC (&tv, &ts);
> -  ts.tv_nsec += 300000000;
> -  if (ts.tv_nsec >= 1000000000)
> -    {
> -      ts.tv_nsec -= 1000000000;
> -      ++ts.tv_sec;
> -    }
> -
> -  int err = pthread_rwlock_timedwrlock (r, &ts);
> +  int err = pthread_rwlock_timedwrlock (r, &ts_timeout);
>    if (err == 0)
>      {
>        puts ("rwlock_timedwrlock returned");
> @@ -65,24 +59,24 @@ tf (void *arg)
>      }
>    puts ("child: timedwrlock failed with ETIMEDOUT");
>  
> -  struct timeval tv2;
> -  (void) gettimeofday (&tv2, NULL);
> -
> -  timersub (&tv2, &tv, &tv);
> +  struct timespec ts_end;
> +  (void) clock_gettime (CLOCK_REALTIME, &ts_end);
> +  struct timespec ts_diff;
> +  timespec_sub (&ts_diff, &ts_end, &ts_start);
>  
> -  if (tv.tv_usec < 200000)
> +  if (ts_diff.tv_sec != 0 || ts_diff.tv_nsec < 200000000)
>      {
>        puts ("timeout too short");
>        pthread_exit ((void *) 1l);
>      }
>  
> -  (void) gettimeofday (&tv, NULL);
> -  TIMEVAL_TO_TIMESPEC (&tv, &ts);
> -  ts.tv_sec += 10;
> +  struct timespec ts_invalid;
> +  (void) clock_gettime (CLOCK_REALTIME, &ts_invalid);
> +  ts_invalid.tv_sec += 10;
>    /* Note that the following operation makes ts invalid.  */
> -  ts.tv_nsec += 1000000000;
> +  ts_invalid.tv_nsec += 1000000000000;
>  
> -  err = pthread_rwlock_timedwrlock (r, &ts);
> +  err = pthread_rwlock_timedwrlock (r, &ts_invalid);
>    if (err == 0)
>      {
>        puts ("2nd timedwrlock succeeded");
> @@ -132,11 +126,8 @@ do_test (void)
>  	  exit (1);
>  	}
>  
> -      struct timeval tv;
> -      (void) gettimeofday (&tv, NULL);
> -
>        struct timespec ts;
> -      TIMEVAL_TO_TIMESPEC (&tv, &ts);
> +      (void) clock_gettime (CLOCK_REALTIME, &ts);
>  
>        ++ts.tv_sec;
>  

Ok.

> diff --git a/nptl/tst-rwlock9.c b/nptl/tst-rwlock9.c
> index 34f2d04..ff15f90 100644
> --- a/nptl/tst-rwlock9.c
> +++ b/nptl/tst-rwlock9.c
> @@ -56,9 +56,7 @@ writer_thread (void *nr)
>        int e;
>        do
>  	{
> -	  struct timeval tv;
> -	  (void) gettimeofday (&tv, NULL);
> -	  TIMEVAL_TO_TIMESPEC (&tv, &ts);
> +	  (void) clock_gettime (CLOCK_REALTIME, &ts);
>  
>  	  ts.tv_nsec += 2 * TIMEOUT;
>  	  if (ts.tv_nsec >= 1000000000)
> @@ -110,9 +108,7 @@ reader_thread (void *nr)
>        int e;
>        do
>  	{
> -	  struct timeval tv;
> -	  (void) gettimeofday (&tv, NULL);
> -	  TIMEVAL_TO_TIMESPEC (&tv, &ts);
> +	  (void) clock_gettime (CLOCK_REALTIME, &ts);
>  
>  	  ts.tv_nsec += TIMEOUT;
>  	  if (ts.tv_nsec >= 1000000000)

Ok.

> diff --git a/support/timespec.h b/support/timespec.h
> new file mode 100644
> index 0000000..e5c89df
> --- /dev/null
> +++ b/support/timespec.h
> @@ -0,0 +1,27 @@
> +static inline void
> +timespec_sub (struct timespec *diff, const struct timespec *left,
> +	      const struct timespec *right)
> +{
> +  diff->tv_sec = left->tv_sec - right->tv_sec;
> +  diff->tv_nsec = left->tv_nsec - right->tv_nsec;
> +
> +  if (diff->tv_nsec < 0)
> +    {
> +      --diff->tv_sec;
> +      diff->tv_nsec += 1000000000;
> +    }
> +}
> +
> +static inline void
> +timespec_add (struct timespec *sum, const struct timespec *left,
> +	      const struct timespec *right)
> +{
> +  sum->tv_sec = left->tv_sec + right->tv_sec;
> +  sum->tv_nsec = left->tv_nsec + right->tv_nsec;
> +
> +  if (sum->tv_nsec >= 1000000000)
> +    {
> +      ++sum->tv_sec;
> +      sum->tv_nsec -= 1000000000;
> +    }
> +}
> 

Ok.
  

Hi,

Le mercredi 27 février 2019 à 18:23 +0000, Mike Crowe a écrit :
> diff --git a/nptl/tst-rwlock9.c b/nptl/tst-rwlock9.c
> index 34f2d04..ff15f90 100644
> --- a/nptl/tst-rwlock9.c
> +++ b/nptl/tst-rwlock9.c
> @@ -56,9 +56,7 @@ writer_thread (void *nr)
>        int e;
>        do
>  	{
> -	  struct timeval tv;
> -	  (void) gettimeofday (&tv, NULL);
> -	  TIMEVAL_TO_TIMESPEC (&tv, &ts);
> +	  (void) clock_gettime (CLOCK_REALTIME, &ts);
>  
>  	  ts.tv_nsec += 2 * TIMEOUT;
>  	  if (ts.tv_nsec >= 1000000000)

Should timespec_add() be used here too ?

> @@ -110,9 +108,7 @@ reader_thread (void *nr)
>        int e;
>        do
>  	{
> -	  struct timeval tv;
> -	  (void) gettimeofday (&tv, NULL);
> -	  TIMEVAL_TO_TIMESPEC (&tv, &ts);
> +	  (void) clock_gettime (CLOCK_REALTIME, &ts);
>  
>  	  ts.tv_nsec += TIMEOUT;
>  	  if (ts.tv_nsec >= 1000000000)

Same here

Regards.
  

On Tuesday 05 March 2019 at 15:02:21 -0300, Adhemerval Zanella wrote:
> On 27/02/2019 15:23, Mike Crowe wrote:
> > In preparation for adding pthread_rwlock_clockrdlock and
> > pthread_rwlock_clockwrlock, convert various tests to only use clock_gettime
> > and struct timespec.
> > 
> > * support/timespec.h: Create header to provide timespec helper functions
> >   from sysdeps/pthread/posix-timer.h for tests to use.
> > 
> > * nptl/tst-rwlock6.c: Fix small bug in timeout-checking code that could
> >   erroneously pass if the function incorrectly took more than a second.
> > 
> > * nptl/tst-rwlock6.c: Use clock_gettime(2) rather than gettimeofday(2) and
> >   then converting to timespec in preparation for testing
> >   pthread_rwlock_clockrdclock and pthread_rwlock_clockwrlock.
> > 
> > * nptl/tst-rwlock9.c, nptl/tst-rwlock7.c: Likewise.
> 
> I am seeing this issue sporadically on i686-linux-gnu with 6/7 patches
> applied:
> 
> $ ./testrun.sh nptl/tst-rwlock7 --direct
> 0: got timedrdlock
> child: timedwrlock failed with ETIMEDOUT
> child: timedwrlock failed with EINVAL
> 1: got timedrdlock
> child: timedwrlock failed with ETIMEDOUT
> child: timedwrlock failed with EINVAL
> 2: got timedrdlock
> child: timedwrlock failed with ETIMEDOUT
> 2nd timedwrlock did not return EINVAL
> failure in round 2

Sorry, I hadn't spotted this part of your email until today. Luckily it's
clear what I got wrong and why I didn't see it on AArch64 and x86_64:

> > +  struct timespec ts_invalid;
> > +  (void) clock_gettime (CLOCK_REALTIME, &ts_invalid);
> > +  ts_invalid.tv_sec += 10;
> >    /* Note that the following operation makes ts invalid.  */
> > -  ts.tv_nsec += 1000000000;
> > +  ts_invalid.tv_nsec += 1000000000000;

I got so used to converting from µs to ns that I added three extra zeroes
unnecessarily here. It fits in a 64-bit long, but not a 32-bit one.

Thanks. I shall do more testing on 32-bit targets.

Mike.