Re: [PATCH v3] Fix time/tst-cpuclock1 intermitent failures

[Matheus Castanho <msc@linux.ibm.com>]

Hi Lucas,

I believe the main idea behind Carlos' initial suggestion was to make
something more generic that could also be used in other places. The
patch is following in this direction alright but I think there are still
a few details that could be improved in this regard. Comments below.

On 2/20/20 3:17 PM, Lucas A. M. Magalhaes wrote:
> This test fails intermittently in systems with heavy load as
> CLOCK_PROCESS_CPUTIME_ID is subject to scheduler pressure.  Thus the
> test boundaries where relaxed to keep it from fail on this systems.
"Thus the test boundaries were relaxed to keep it from failing on such
systems."

> 
> A refactor of the spent time checking was made with some support
> functions.  With the advantage to represent time jitter in percent
> of the target.
> 
> The values used by the test boundaries are all empirical.
> 
> ---
> 
> Hi,
> 
> changes on V3:
> 	- refactor support functions
> 	- use existing timespec-sub function
> 
> changes on V2:
> 	- Add support functions
> 
>  support/Makefile           |  1 +
>  support/support_cpuclock.c | 58 ++++++++++++++++++++++++++++++++++++++
>  support/support_cpuclock.h | 28 ++++++++++++++++++
>  time/tst-cpuclock1.c       | 47 +++++++++++-------------------
>  4 files changed, 103 insertions(+), 31 deletions(-)
>  create mode 100644 support/support_cpuclock.c
>  create mode 100644 support/support_cpuclock.h
> 
> diff --git a/support/Makefile b/support/Makefile
> index a0304e6def..1e45d99b27 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -31,6 +31,7 @@ libsupport-routines = \
>    check_dns_packet \
>    check_hostent \
>    check_netent \
> +  support_cpuclock \
>    delayed_exit \
>    ignore_stderr \
>    next_to_fault \
> diff --git a/support/support_cpuclock.c b/support/support_cpuclock.c
> new file mode 100644
> index 0000000000..cd5212bc32
> --- /dev/null
> +++ b/support/support_cpuclock.c
> @@ -0,0 +1,58 @@
> +/* Support functions for cpuclock tests.
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include "support_cpuclock.h"
> +#include <stdlib.h>
> +#include <assert.h>
> +
> +#define TIMESPEC_HZ 1000000000.0
> +
> +/* Returns t normalized timespec with .tv_nsec < TIMESPEC_HZ
> +   and the overflows added to .tv_sec.  */
> +struct timespec
> +support_timespec_normalize (struct timespec t)
> +{
> +  int diff;
> +  diff = (t.tv_nsec / TIMESPEC_HZ);
> +  t.tv_sec += diff;
> +  t.tv_nsec += -(diff * TIMESPEC_HZ);
> +  return t;
> +}

Wouldn't something like:

struct timespec
support_timespec_normalize (struct timespec t)
{
  t.tv_sec += (t.tv_nsec / TIMESPEC_HZ);
  t.tv_nsec = (t.tv_nsec % TIMESPEC_HZ);
  return t;
}

be more straightforward?

Also, since this is a fairly generic function for timespec, I think it
could be added to support/timespec.h, like timespec_add and timespec_sub.

> +
> +/* Returns TRUE if diff to base ratio is within the specified bounds, and
> +FALSE otherwise.
> +For example the call
> +
> +support_timespec_check_ratio(base, diff, 1.2, .5);
> +
> +will check if
> +
> +.5 <= diff/base <= 1.2
> +
> +In other words it will check if diff time is within 50% to 120% of
> +the base time.  */
> +int
> +support_timespec_check_ratio (struct timespec base, struct timespec diff,
> +			      double upper_bound, double lower_bound)

I believe support_timespec_check_in_range (as used somewhere previously
on this thread) or support_timespec_check_within_bounds (maybe too long)
would be more appropriate names, since the ratio is only part of the
calculation, not what the function actually does.

A minor detail, but my brain finds it confusing that the upper_bound
comes before the lower_bound in the arguments list when the final check
will be like: lower_bound <= diff/base <= upper_bound.

At last, diff seems to be a leftover from the current use case for this
function (in time/tst-cpuclock1.c). Since this is a generic function
that can be used in other places, I suggest changing the name to 'value'
(or similar).

> +{
> +  assert (upper_bound >= lower_bound);
> +  double base_norm = base.tv_sec + base.tv_nsec / TIMESPEC_HZ;
> +  double diff_norm = diff.tv_sec + diff.tv_nsec / TIMESPEC_HZ;
> +  double ratio = diff_norm / base_norm;
> +  return (ratio <= upper_bound && ratio >= lower_bound);
> +}

When Adhermeval suggested to normalize this to nanoseconds instead of
seconds, you replied that this would not be used to check really small
values. But once it enters support/*, I believe it is fair game to be
reused in other places that might as well deal with small values, so
it's something to consider.

> diff --git a/support/support_cpuclock.h b/support/support_cpuclock.h
> new file mode 100644
> index 0000000000..a14bc91e3f
> --- /dev/null
> +++ b/support/support_cpuclock.h
> @@ -0,0 +1,28 @@
> +/* Support functions for cpuclock tests.
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#ifndef SUPPORT_CPUCLOCK_H
> +#define SUPPORT_CPUCLOCK_H
> +
> +#include <time.h>
> +
> +struct timespec support_timespec_normalize (struct timespec t);
> +int support_timespec_check_ratio (struct timespec base, struct timespec diff,
> +				  double upper_bound, double lower_bound);
> +
> +#endif /* SUPPORT_CPUCLOCK_H */
> diff --git a/time/tst-cpuclock1.c b/time/tst-cpuclock1.c
> index 0120906f23..72450f71ee 100644
> --- a/time/tst-cpuclock1.c
> +++ b/time/tst-cpuclock1.c
> @@ -26,6 +26,8 @@
>  #include <signal.h>
>  #include <stdint.h>
>  #include <sys/wait.h>
> +#include <support/support_cpuclock.h>
> +#include <support/timespec.h>
>  
>  /* This function is intended to rack up both user and system time.  */
>  static void
> @@ -155,19 +157,13 @@ do_test (void)
>    printf ("live PID %d after sleep => %ju.%.9ju\n",
>  	  child, (uintmax_t) after.tv_sec, (uintmax_t) after.tv_nsec);
>  
> -  struct timespec diff = { .tv_sec = after.tv_sec - before.tv_sec,
> -			   .tv_nsec = after.tv_nsec - before.tv_nsec };
> -  if (diff.tv_nsec < 0)
> -    {
> -      --diff.tv_sec;
> -      diff.tv_nsec += 1000000000;
> -    }
> -  if (diff.tv_sec != 0
> -      || diff.tv_nsec > 600000000
> -      || diff.tv_nsec < 100000000)
> +  support_timespec_normalize(after);
> +  support_timespec_normalize(before);
> +  struct timespec diff = timespec_sub(after, before);
> +  if (!support_timespec_check_ratio(sleeptime, diff, 1.3, .0025))
>      {
>        printf ("before - after %ju.%.9ju outside reasonable range\n",

Ok.

> -	      (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec);
> +	    (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec);

Unintended change?

>        result = 1;
>      }
>  
> @@ -194,19 +190,12 @@ do_test (void)
>  	}
>        else
>  	{
> -	  struct timespec d = { .tv_sec = afterns.tv_sec - after.tv_sec,
> -				.tv_nsec = afterns.tv_nsec - after.tv_nsec };
> -	  if (d.tv_nsec < 0)
> -	    {
> -	      --d.tv_sec;
> -	      d.tv_nsec += 1000000000;
> -	    }
> -	  if (d.tv_sec > 0
> -	      || d.tv_nsec < sleeptime.tv_nsec
> -	      || d.tv_nsec > sleeptime.tv_nsec * 2)
> +	  support_timespec_normalize(afterns);
> +	  diff = timespec_sub(afterns, after);
> +	  if (!support_timespec_check_ratio(sleeptime, diff, 1.6, .71))
>  	    {
>  	      printf ("nanosleep time %ju.%.9ju outside reasonable range\n",
> -		      (uintmax_t) d.tv_sec, (uintmax_t) d.tv_nsec);
> +		     (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec);
>  	      result = 1;
>  	    }
>  	}

Ok.

> @@ -241,17 +230,13 @@ do_test (void)
>    printf ("dead PID %d => %ju.%.9ju\n",
>  	  child, (uintmax_t) dead.tv_sec, (uintmax_t) dead.tv_nsec);
>  
> -  diff.tv_sec = dead.tv_sec - after.tv_sec;
> -  diff.tv_nsec = dead.tv_nsec - after.tv_nsec;
> -  if (diff.tv_nsec < 0)
> -    {
> -      --diff.tv_sec;
> -      diff.tv_nsec += 1000000000;
> -    }
> -  if (diff.tv_sec != 0 || diff.tv_nsec > 200000000)
> +  support_timespec_normalize(dead);
> +  diff = timespec_sub(dead, after);
> +  sleeptime.tv_nsec = 100000000;
> +  if (!support_timespec_check_ratio(sleeptime, diff, 1.6, .7))
>      {

Ok.

>        printf ("dead - after %ju.%.9ju outside reasonable range\n",
> -	      (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec);
> +	     (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec);

Unintended change?

>        result = 1;
>      }
>  
> 

Thanks,
Matheus Castanho