[LTP] [PATCH v2] clock_gettime04: set threshold based on the clock resolution

[LTP] [PATCH v2] clock_gettime04: set threshold based on the clock resolution

[Li Wang]

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;
 	}
 
 	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 */
-- 
2.31.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] clock_gettime04: set threshold based on the clock resolution

[Cyril Hrubis]

Hi!
Looks good.

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] clock_gettime04: set threshold based on the clock resolution

[Waiman Long]

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


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] clock_gettime04: set threshold based on the clock resolution

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1408 bytes --]

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.

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2721 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] clock_gettime04: set threshold based on the clock resolution

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] clock_gettime04: set threshold based on the clock resolution

[Waiman Long]

[-- Attachment #1.1: Type: text/plain, Size: 1782 bytes --]

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>

[-- Attachment #1.2: Type: text/html, Size: 4079 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] clock_gettime04: set threshold based on the clock resolution

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 219 bytes --]

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

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 733 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp