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

[Carlos O'Donell]

On 3/24/20 3:42 PM, Lucas A. M. Magalhaes wrote:
> 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?

Please add a comment explaining why.

>> 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.

These *have* to be kernel bugs, and we should not paper over kernel bugs. We should
still fail if the kernel has bugs otherwise we'll never get them fixed upstream.

We should make tests robust with respect to waiting for infinite events to happen.
For example when sleeping we are guaranteed to sleep at-least a certain
amount, and our check should be flexible that we slept at least that amount and
then some amount more, to take into account system load.

In summary: We are making the test robust against system load, not against kernel
defects (which should show up).

-- 
Cheers,
Carlos.