[PATCH v5] Fix time/tst-cpuclock1 intermitent failures

[Lucas A. M. Magalhaes]

Hi Carlos,

Thanks for the review.

Quoting Carlos O'Donell (2020-03-23 18:06:34)
> On 3/23/20 1:20 PM, Lucas A. M. Magalhaes via Libc-alpha wrote:
> > +
> > +/* Returns TRUE if the observed time is within the given percentage bounds of
> > +the expected time, and FALSE otherwise.
> > +For example the call
> > +
> > +support_timespec_check_in_range(expected, observed, .5, 1.2);
> > +
> > +will check if
> > +
> > +.5 <= observed/expected <= 1.2
> > +
> > +In other words it will check if observed time is within 50% to 120% of
> > +the expected time.  */
> > +int
> > +support_timespec_check_in_range (struct timespec expected, struct timespec observed,
> > +                           double lower_bound, double upper_bound)
> > +{
> > +  assert (upper_bound >= lower_bound);
> > +  long expected_norm = expected.tv_sec * TIMESPEC_HZ + expected.tv_nsec;
> 
> This can cause overflow/underflow.
> 
> Please review timespec_add.
> 
> We should set this to a extreme value just like timepsec_add for both overflow/underflow.
> 
> > +  assert(expected_norm != 0);
> 
> Why can't expected_norm be zero?
> 

It can't be zero because of the division below.  Do you have any suggestions on
this matter?

> If you have an abstract timespec you may want to check against that.
> 
> I would assert that all values are *positive* and write that into the comments
> above.
> 
> > +  long observed_norm = observed.tv_sec * TIMESPEC_HZ + observed.tv_nsec;
> > +  double ratio = (double)observed_norm / expected_norm;
> > +  return (lower_bound <= ratio && ratio <= upper_bound);
> > +}

[...]

> > +
> > +#include <support/test-driver.c>
> > diff --git a/time/tst-cpuclock1.c b/time/tst-cpuclock1.c
> > index 0120906f23..fe9bb0a31e 100644
> > --- a/time/tst-cpuclock1.c
> > +++ b/time/tst-cpuclock1.c
> > @@ -26,6 +26,7 @@
> >  #include <signal.h>
> >  #include <stdint.h>
> >  #include <sys/wait.h>
> > +#include <support/timespec.h>
> >  
> >  /* This function is intended to rack up both user and system time.  */
> >  static void
> > @@ -155,16 +156,11 @@ 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)
> > +  /* The bound values are empirically defined by testing this code over high cpu
> > +     usage and different nice values.  */
> > +  struct timespec diff = timespec_sub (support_timespec_normalize (after),
> > +                                    support_timespec_normalize (before));
> > +  if (!support_timespec_check_in_range (sleeptime, diff, .0025,  1.3))
> 
> The value of 0.0025 doesn't seem correct, can you please confirm that?
> 

I got values as low as 0,0008s for this. But I can be more restrict.  The
values lower than 0.1 are less than < 1% of my sample.  But these are the ones
bothering during cpu stress.

> This is supposed to be a 0.5s wait, and wait of 1ms doesn't seem correct.
> 

---
Lucas A. M. Magalhães