Re: [PATCH v2] Loosen the limits of time/tst-cpuclock1.

[Stefan Liebler via Libc-alpha <libc-alpha@sourceware.org>]

On 10/21/20 2:58 PM, Adhemerval Zanella wrote:
> 
> 
> On 19/10/2020 11:47, Stefan Liebler via Libc-alpha wrote:
>> Starting with the commit 04deeaa9ea74b0679dfc9d9155a37b6425f19a9f
>> "Fix time/tst-cpuclock1 intermitent failures" (2020-07-11),
>> this test fails quite often on s390x/s390 with one/multiple of those:
>> "before - after" / "nanosleep time" / "dead - after" ourside reasonable range.
>>
>> On a zVM/kvm guest the CPUs are shared between multiple guests.
>> And even on the lpar (kvm host) the CPUs are usually shared between multiple lpars.
>> The defined CPUs for a lpar/zVM-system could also have lower weights compared
>> to other lpars which let the steal time further grow.
>>
>> Usually I build (-j$(nproc)) and test (PARALLELMFLAGS="-j$(nproc)") glibc multiple
>> times, e.g. with different GCCs, on various lpars or zVM guests at the same time.
>> During this time, I've run the test for 13500 times and obvserved the following fails:
>> ~600x "before - after"
>> ~60x "nanosleep time"
>> ~70x "dead - after"
>>
>> I've also observed a lot of "before - after" fails on a intel kvm-guest while
>> building/testing glibc on it.
>>
>> The mentioned commit has tighten the limits of valid tv_nsec ranges:
>> "before - after" (expected: 500000000):
>> - 100000000 ... 600000000
>> + 450000000 ... 550000000
>>
>> "nanosleep time" (expected: 100000000):
>> - 100000000 ... 200000000
>> + 090000000 ... 120000000
>>
>> "dead - after" (expected: 100000000):
>> -           ... 200000000
>> + 090000000 ... 120000000
>>
>> The test itself forks a child process which chew_cpu (user- and kernel-space).
>> The parent process sleeps with nanosleep(0.5s) and measures the child_clock time:
>> diff = after - before
>> With much workload on the machine, the child won't make much progess
>> and it can fall much beyond the minimum limit. Thus this check is now removed!
> 
> Ok.
> 
>>
>> Afterwards the parent process sleeps with clock_nanosleep (child_clock, 0.1s):
>> diff = afterns - after
>> The test currently also allows 0.9 * 0.1s.  As this would be an error, the
>> hard limit of 1.0 * 0.1s is now used as minimum border!
>> Depending on the workload, the maximum limit can exceed the 1.2 * 0.1s.
>> Therefore the upper limit is set to 2.0 which was also used before the
>> mentioned commit.
> 
> This is still tricky and add heuristic values that might fail depending
> of the architecture/kernel.  Wouldn't be better to follow Carlos suggestion
> and strip down from the test all the time related checks and only keep 
> the functional interfaces of the ABI:
> 
>   * clock_getcpuclockid vs. ENOSYS / ESRCH / EPERM
>   * clock_getcpuclockid vs. valid child
>   * clock_gettime of dead child where clock is no longer valid
> 
Sure, I can just remove this last time related check. This would be the
following diff and the title of the new commit would be "Remove timing
related checks of time/tst-cpuclock1":
diff --git a/time/tst-cpuclock1.c b/time/tst-cpuclock1.c
index cc08150654..f40b590111 100644
--- a/time/tst-cpuclock1.c
+++ b/time/tst-cpuclock1.c
@@ -26,7 +26,6 @@
 #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
@@ -163,21 +162,6 @@ do_test (void)
          printf ("live PID %d after sleep => %ju.%.9ju\n",
                  child, (uintmax_t) afterns.tv_sec,
                  (uintmax_t) afterns.tv_nsec);
-
-         /* As the sleep is based on the child clock, the diff should never
-            be less than the specified sleeptime.  Otherwise this is an
error.
-            The upper bound is quite high in order to get no failure if
running
-            with high cpu usage and/or on virtualized environments with
shared
-            CPUs.  */
-         struct timespec diff;
-         diff = timespec_sub (support_timespec_normalize (afterns),
-                              support_timespec_normalize (before));
-         if (!support_timespec_check_in_range (sleeptime, diff, 1.0, 2.0))
-           {
-             printf ("nanosleep time %ju.%.9ju outside reasonable range\n",
-                     (uintmax_t) diff.tv_sec, (uintmax_t) diff.tv_nsec);
-             result = 1;
-           }
        }
     }


> And then maybe we can add *another* test that might evaluate timings report
> as the tests was originally intended?
> 
Thus we would have time/tst-cpuclock1 which just performs functional checks
and time/tst-cpuclock1-timings which additionally performs the just
removed timing checks?

The timing checks could be enabled by setting a macro:
tst-cpuclock1-timings.c:
#define ENABLE_TIMING_CHECKS 1
#include <tst-cpuclock1.c>

But then time/tst-cpuclock1-timings would fail as often as the current
time/tst-cpuclock1 test if run on systems with high cpu-load /
virtualized CPUs. Should the valid limits be adjusted? If yes, which
limits should be used?
I think at least the "before - after" check which compares different
clocks should be removed.

Bye,
Stefan