| Message ID | 20220329050351.688432-1-liwang@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] clock_gettime04: set threshold based on the clock resolution | expand |
Hi!
Looks good.
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
On 3/29/22 01:03, Li Wang wrote: > This is to get rid of the intermittent failures in clock_gettime04, > which are likely caused by different clock tick rates on platforms. > Here give two thresholds (in milliseconds) for comparison, one for > COARSE clock and one for the rest. > > Error log: > clock_gettime04.c:163: TFAIL: CLOCK_REALTIME_COARSE(syscall with old kernel spec): > Difference between successive readings greater than 5 ms (1): 10 > clock_gettime04.c:163: TFAIL: CLOCK_MONOTONIC_COARSE(vDSO with old kernel spec): > Difference between successive readings greater than 5 ms (2): 10 > > From Waiman Long: > That failure happens for CLOCK_REALTIME_COARSE which is a faster but less > precise version of CLOCK_REALTIME. The time resolution is actually a clock > tick. Since arm64 has a HZ rate of 100. That means each tick is 10ms. So a > CLOCK_REALTIME_COARSE threshold of 5ms is probably not enough. I would say > in the case of CLOCK_REALTIME_COARSE, we have to increase the threshold based > on the clock tick rate of the system. This is more a test failure than is > an inherent problem in the kernel. > > Fixes #898 > > Reported-by: Eirik Fuller <efuller@redhat.com> > Signed-off-by: Li Wang <liwang@redhat.com> > Cc: Waiman Long <llong@redhat.com> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > --- > > Notes: > v1 --> v2 > * make use of clock_getres to get clock resolution > * involve separate coarse_delta for COARSE clock > * count delta as 'clock resolution + elapse (5ms)' > > .../syscalls/clock_gettime/clock_gettime04.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c b/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c > index a8d2c5b38..c279da79e 100644 > --- a/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c > +++ b/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c > @@ -35,7 +35,7 @@ clockid_t clks[] = { > }; > > static gettime_t ptr_vdso_gettime, ptr_vdso_gettime64; > -static long long delta = 5; > +static long long delta, precise_delta, coarse_delta; > > static inline int do_vdso_gettime(gettime_t vdso, clockid_t clk_id, void *ts) > { > @@ -92,9 +92,18 @@ static struct time64_variants variants[] = { > > static void setup(void) > { > + struct timespec res; > + > + clock_getres(CLOCK_REALTIME, &res); > + precise_delta = 5 + res.tv_nsec / 1000000; > + > + clock_getres(CLOCK_REALTIME_COARSE, &res); > + coarse_delta = 5 + res.tv_nsec / 1000000; > + > if (tst_is_virt(VIRT_ANY)) { > tst_res(TINFO, "Running in a virtual machine, multiply the delta by 10."); > - delta *= 10; > + precise_delta *= 10; > + coarse_delta *= 10; > } The patch looks good in general. However, maybe we should do something like: diff --git a/clock_gettime04.c b/clock_gettime04.c index a8d2c5b..1ba218b 100644 --- a/clock_gettime04.c +++ b/clock_gettime04.c @@ -92,11 +92,18 @@ static struct time64_variants variants[] = { static void setup(void) { + delta = 5; if (tst_is_virt(VIRT_ANY)) { tst_res(TINFO, "Running in a virtual machine, multiply the delta by 10."); delta *= 10; } + clock_getres(CLOCK_REALTIME, &res); + precise_delta = delta + res.tv_nsec / 1000000; + + clock_getres(CLOCK_REALTIME_COARSE, &res); + coarse_delta = delta + res.tv_nsec / 1000000; + find_clock_gettime_vdso(&ptr_vdso_gettime, &ptr_vdso_gettime64); } to avoid a coarse_delta that is too large for vm. Cheers, Longman
Waiman Long <longman@redhat.com> wrote: The patch looks good in general. However, maybe we should do something like: > > diff --git a/clock_gettime04.c b/clock_gettime04.c > index a8d2c5b..1ba218b 100644 > --- a/clock_gettime04.c > +++ b/clock_gettime04.c > @@ -92,11 +92,18 @@ static struct time64_variants variants[] = { > > static void setup(void) > { > + delta = 5; > if (tst_is_virt(VIRT_ANY)) { > tst_res(TINFO, "Running in a virtual machine, multiply > the delta by 10."); > delta *= 10; > } > > + clock_getres(CLOCK_REALTIME, &res); > + precise_delta = delta + res.tv_nsec / 1000000; > + > + clock_getres(CLOCK_REALTIME_COARSE, &res); > + coarse_delta = delta + res.tv_nsec / 1000000; > + > find_clock_gettime_vdso(&ptr_vdso_gettime, &ptr_vdso_gettime64); > } > > to avoid a coarse_delta that is too large for vm. > Thierically that's right, we only make the resolution as additional value to tolerate. But I'm afraid this is the part we can not guarantee especially for VM. As from Eirik's test history, the KVM guest ever failed with "150ms" delay: clock_gettime04.c:163: TFAIL: CLOCK_BOOTTIME(vDSO with old kernel spec): Difference between successive readings greater than 50 ms (2): 150 If we decide to go with your suggestion, I think we'd better skip this test on VM.
Hi! > Thierically that's right, we only make the resolution as additional value > to tolerate. > > But I'm afraid this is the part we can not guarantee especially for VM. > As from Eirik's test history, the KVM guest ever failed with "150ms" delay: > clock_gettime04.c:163: TFAIL: CLOCK_BOOTTIME(vDSO with old kernel spec): > Difference between successive readings greater than 50 ms (2): 150 I do agree that on VMs all bets about timer precisions are off but I still think that it makes more sense to run the test with greater deltas there. As long as the VM has acces to high resolution timers we will will have smaller delta for these clocks using them.
On 3/29/22 23:16, Li Wang wrote: > Waiman Long <longman@redhat.com> wrote: > > > The patch looks good in general. However, maybe we should do > something like: > > diff --git a/clock_gettime04.c b/clock_gettime04.c > index a8d2c5b..1ba218b 100644 > --- a/clock_gettime04.c > +++ b/clock_gettime04.c > @@ -92,11 +92,18 @@ static struct time64_variants variants[] = { > > static void setup(void) > { > + delta = 5; > if (tst_is_virt(VIRT_ANY)) { > tst_res(TINFO, "Running in a virtual machine, > multiply > the delta by 10."); > delta *= 10; > } > > + clock_getres(CLOCK_REALTIME, &res); > + precise_delta = delta + res.tv_nsec / 1000000; > + > + clock_getres(CLOCK_REALTIME_COARSE, &res); > + coarse_delta = delta + res.tv_nsec / 1000000; > + > find_clock_gettime_vdso(&ptr_vdso_gettime, > &ptr_vdso_gettime64); > } > > to avoid a coarse_delta that is too large for vm. > > > Thierically that's right, we only make the resolution as additional > value to tolerate. > > But I'm afraid this is the part we can not guarantee especially for VM. > As from Eirik's test history, the KVM guest ever failed with "150ms" > delay: > clock_gettime04.c:163: TFAIL: CLOCK_BOOTTIME(vDSO with old kernel > spec): Difference between successivereadings greater than 50 ms (2): 150 > > If we decide to go with your suggestion, I think we'd better skip this > test on VM. > I see. So we really need more tolerance for vm. I am OK with your current patch then. Acked-by: Waiman Long <longman@redhat.com>
> I see. So we really need more tolerance for vm. I am OK with your current > patch then. > > Acked-by: Waiman Long <longman@redhat.com> <longman@redhat.com> > Patch applied, thanks for reviewing.
diff --git a/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c b/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c index a8d2c5b38..c279da79e 100644 --- a/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c +++ b/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c @@ -35,7 +35,7 @@ clockid_t clks[] = { }; static gettime_t ptr_vdso_gettime, ptr_vdso_gettime64; -static long long delta = 5; +static long long delta, precise_delta, coarse_delta; static inline int do_vdso_gettime(gettime_t vdso, clockid_t clk_id, void *ts) { @@ -92,9 +92,18 @@ static struct time64_variants variants[] = { static void setup(void) { + struct timespec res; + + clock_getres(CLOCK_REALTIME, &res); + precise_delta = 5 + res.tv_nsec / 1000000; + + clock_getres(CLOCK_REALTIME_COARSE, &res); + coarse_delta = 5 + res.tv_nsec / 1000000; + if (tst_is_virt(VIRT_ANY)) { tst_res(TINFO, "Running in a virtual machine, multiply the delta by 10."); - delta *= 10; + precise_delta *= 10; + coarse_delta *= 10; } find_clock_gettime_vdso(&ptr_vdso_gettime, &ptr_vdso_gettime64); @@ -108,6 +117,11 @@ static void run(unsigned int i) int count = 10000, ret; unsigned int j; + if (clks[i] == CLOCK_REALTIME_COARSE || clks[i] == CLOCK_MONOTONIC_COARSE) + delta = coarse_delta; + else + delta = precise_delta; + do { for (j = 0; j < ARRAY_SIZE(variants); j++) { /* Refresh time in start */
This is to get rid of the intermittent failures in clock_gettime04, which are likely caused by different clock tick rates on platforms. Here give two thresholds (in milliseconds) for comparison, one for COARSE clock and one for the rest. Error log: clock_gettime04.c:163: TFAIL: CLOCK_REALTIME_COARSE(syscall with old kernel spec): Difference between successive readings greater than 5 ms (1): 10 clock_gettime04.c:163: TFAIL: CLOCK_MONOTONIC_COARSE(vDSO with old kernel spec): Difference between successive readings greater than 5 ms (2): 10 From Waiman Long: That failure happens for CLOCK_REALTIME_COARSE which is a faster but less precise version of CLOCK_REALTIME. The time resolution is actually a clock tick. Since arm64 has a HZ rate of 100. That means each tick is 10ms. So a CLOCK_REALTIME_COARSE threshold of 5ms is probably not enough. I would say in the case of CLOCK_REALTIME_COARSE, we have to increase the threshold based on the clock tick rate of the system. This is more a test failure than is an inherent problem in the kernel. Fixes #898 Reported-by: Eirik Fuller <efuller@redhat.com> Signed-off-by: Li Wang <liwang@redhat.com> Cc: Waiman Long <llong@redhat.com> Cc: Viresh Kumar <viresh.kumar@linaro.org> --- Notes: v1 --> v2 * make use of clock_getres to get clock resolution * involve separate coarse_delta for COARSE clock * count delta as 'clock resolution + elapse (5ms)' .../syscalls/clock_gettime/clock_gettime04.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)