[LTP] [PATCH] clock_gettime04: set threshold based on the clock tick rate

[LTP] [PATCH] clock_gettime04: set threshold based on the clock tick rate

[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 set the threshold no less than each clock tick in millisecond:

	delta = 1000(ms)/ticks_number_per_sec + 5;

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>
---
 testcases/kernel/syscalls/clock_gettime/clock_gettime04.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c b/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c
index a8d2c5b38..cccbc9383 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;
 
 static inline int do_vdso_gettime(gettime_t vdso, clockid_t clk_id, void *ts)
 {
@@ -92,6 +92,7 @@ static struct time64_variants variants[] = {
 
 static void setup(void)
 {
+	delta = 1000/sysconf(_SC_CLK_TCK) + 5;
 	if (tst_is_virt(VIRT_ANY)) {
 		tst_res(TINFO, "Running in a virtual machine, multiply the delta by 10.");
 		delta *= 10;
-- 
2.31.1


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

Re: [LTP] [PATCH] clock_gettime04: set threshold based on the clock tick rate

[Cyril Hrubis]

On Fri, Mar 25, 2022 at 12:00:57PM +0800, 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 set the threshold no less than each clock tick in millisecond:
> 
> 	delta = 1000(ms)/ticks_number_per_sec + 5;
> 
> 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>
> ---
>  testcases/kernel/syscalls/clock_gettime/clock_gettime04.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c b/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c
> index a8d2c5b38..cccbc9383 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;
>  
>  static inline int do_vdso_gettime(gettime_t vdso, clockid_t clk_id, void *ts)
>  {
> @@ -92,6 +92,7 @@ static struct time64_variants variants[] = {
>  
>  static void setup(void)
>  {
> +	delta = 1000/sysconf(_SC_CLK_TCK) + 5;

This does not look correct to me. The sysconf(_SC_CLK_TCK) returns 100
on systems where the test was working fine with 5 second delta. I think
that the difference is that _SC_CLK_TCK returns how fast are the jiffies
incremented which does not really matter for most of the modern hardware
that uses high resolution harware for timers.

I think that we should really use whatever is returned by the
clock_getres(CLOCK_REALTIME_COARSE, &res), as long as high resolution
timers are not available this call will return resolution in miliseconds
and with high resolution timers available the reported resolution will
be in nanosecond range, so it should probably be something as:

	clock_getres(CLOCK_REALTIME_COARSE, &res);

	delta = 5 + (res.tv_nsec / 1000000) * 5;

>  	if (tst_is_virt(VIRT_ANY)) {
>  		tst_res(TINFO, "Running in a virtual machine, multiply the delta by 10.");
>  		delta *= 10;
> -- 
> 2.31.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] clock_gettime04: set threshold based on the clock tick rate

[Li Wang]

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

Cyril Hrubis <chrubis@suse.cz> wrote:


> > --- 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;
> >
> >  static inline int do_vdso_gettime(gettime_t vdso, clockid_t clk_id,
> void *ts)
> >  {
> > @@ -92,6 +92,7 @@ static struct time64_variants variants[] = {
> >
> >  static void setup(void)
> >  {
> > +     delta = 1000/sysconf(_SC_CLK_TCK) + 5;
>
> This does not look correct to me. The sysconf(_SC_CLK_TCK) returns 100
> on systems where the test was working fine with 5 second delta. I think
> that the difference is that _SC_CLK_TCK returns how fast are the jiffies
> incremented which does not really matter for most of the modern hardware
> that uses high resolution harware for timers.
>

You're right, I checked some documents and confirmed this.
There I used the system tick rate (in the worst situations) that
seemed a bit loose to other clockids.


> I think that we should really use whatever is returned by the
> clock_getres(CLOCK_REALTIME_COARSE, &res), as long as high resolution
> timers are not available this call will return resolution in miliseconds
> and with high resolution timers available the reported resolution will
> be in nanosecond range, so it should probably be something as:
>
>         clock_getres(CLOCK_REALTIME_COARSE, &res);
>
>         delta = 5 + (res.tv_nsec / 1000000) * 5;
>

Sounds reasonable.

But I don't understand why you multiply 5 for the resolution
(in milliseconds) here. Or, a wiser choice is to get the real
resolution for each clockid? i.e.

--- a/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c
+++ b/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c
@@ -92,23 +92,27 @@ static struct time64_variants variants[] = {

 static void setup(void)
 {
-       delta = 1000/sysconf(_SC_CLK_TCK) + 5;
-       if (tst_is_virt(VIRT_ANY)) {
-               tst_res(TINFO, "Running in a virtual machine, multiply the
delta by 10.");
-               delta *= 10;
-       }
-
        find_clock_gettime_vdso(&ptr_vdso_gettime, &ptr_vdso_gettime64);
 }

 static void run(unsigned int i)
 {
        struct tst_ts ts;
+       struct timespec res;
        long long start, end = 0, diff, slack;
        struct time64_variants *tv;
        int count = 10000, ret;
        unsigned int j;

+       clock_getres(clks[i], &res);
+       tst_res(TINFO, "%s: resolution is %ldns", tst_clock_name(clks[i]),
res.tv_nsec);
+
+       delta = 5 + res.tv_nsec/1000000;
+       if (tst_is_virt(VIRT_ANY)) {
+               delta *= 10;
+               tst_res(TINFO, "Multiply the delta by 10 in virtual
machine: %lld", delta);
+       }
+
        do {
                for (j = 0; j < ARRAY_SIZE(variants); j++) {
                        /* Refresh time in start */

-- 
Regards,
Li Wang

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

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


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

Re: [LTP] [PATCH] clock_gettime04: set threshold based on the clock tick rate

[Cyril Hrubis]

Hi!
> >         clock_getres(CLOCK_REALTIME_COARSE, &res);
> >
> >         delta = 5 + (res.tv_nsec / 1000000) * 5;
> >
> 
> Sounds reasonable.
> 
> But I don't understand why you multiply 5 for the resolution
> (in milliseconds) here. Or, a wiser choice is to get the real
> resolution for each clockid? i.e.

I did multiply it with 5 just to add some error margin. I guess that we
can as well multiply it with 2 if that works well enough.

I do not think that we should get resolution for each clock, the COARSE
clock should have the worst resolution of all clocks.

> --- a/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c
> +++ b/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c
> @@ -92,23 +92,27 @@ static struct time64_variants variants[] = {
> 
>  static void setup(void)
>  {
> -       delta = 1000/sysconf(_SC_CLK_TCK) + 5;
> -       if (tst_is_virt(VIRT_ANY)) {
> -               tst_res(TINFO, "Running in a virtual machine, multiply the
> delta by 10.");
> -               delta *= 10;
> -       }
> -
>         find_clock_gettime_vdso(&ptr_vdso_gettime, &ptr_vdso_gettime64);
>  }
> 
>  static void run(unsigned int i)
>  {
>         struct tst_ts ts;
> +       struct timespec res;
>         long long start, end = 0, diff, slack;
>         struct time64_variants *tv;
>         int count = 10000, ret;
>         unsigned int j;
> 
> +       clock_getres(clks[i], &res);
> +       tst_res(TINFO, "%s: resolution is %ldns", tst_clock_name(clks[i]),
> res.tv_nsec);
> +
> +       delta = 5 + res.tv_nsec/1000000;
> +       if (tst_is_virt(VIRT_ANY)) {
> +               delta *= 10;
> +               tst_res(TINFO, "Multiply the delta by 10 in virtual
> machine: %lld", delta);
> +       }
> +
>         do {
>                 for (j = 0; j < ARRAY_SIZE(variants); j++) {
>                         /* Refresh time in start */
> 
> -- 
> Regards,
> Li Wang

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] clock_gettime04: set threshold based on the clock tick rate

[Li Wang]

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

On Mon, Mar 28, 2022 at 4:56 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > >         clock_getres(CLOCK_REALTIME_COARSE, &res);
> > >
> > >         delta = 5 + (res.tv_nsec / 1000000) * 5;
> > >
> >
> > Sounds reasonable.
> >
> > But I don't understand why you multiply 5 for the resolution
> > (in milliseconds) here. Or, a wiser choice is to get the real
> > resolution for each clockid? i.e.
>
> I did multiply it with 5 just to add some error margin. I guess that we
> can as well multiply it with 2 if that works well enough.
>

Ok, I'd like to multiply 5 in case VM needs more.


> I do not think that we should get resolution for each clock, the COARSE
> clock should have the worst resolution of all clocks.
>

Yes, quite enough for the rest.
For precise clock, the second part will be zero since divided by 1000000.

    delta = 5 + (res.tv_nsec / 1000000) * 5;


Test data from aarch4 (kernel-5.14) FYI:
===================
clock_gettime04.c:108: TINFO: CLOCK_REALTIME: resolution is 1ns
...
clock_gettime04.c:108: TINFO: CLOCK_REALTIME_COARSE: resolution is
10000000ns
clock_gettime04.c:108: TINFO: CLOCK_MONOTONIC: resolution is 1ns
clock_gettime04.c:108: TINFO: CLOCK_MONOTONIC_COARSE: resolution is
10000000ns
clock_gettime04.c:108: TINFO: CLOCK_MONOTONIC_RAW: resolution is 1ns
clock_gettime04.c:108: TINFO: CLOCK_BOOTTIME: resolution is 1ns


-- 
Regards,
Li Wang

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

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


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

Re: [LTP] [PATCH] clock_gettime04: set threshold based on the clock tick rate

[Li Wang]

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

Hi Cyril,

Sorry, I didn't complete my input but sent the email by touch
key combination...  Comments in below.

On Mon, Mar 28, 2022 at 5:54 PM Li Wang <liwang@redhat.com> wrote:


>
>> I did multiply it with 5 just to add some error margin. I guess that we
>> can as well multiply it with 2 if that works well enough.
>>
>
> Ok, I'd like to multiply 5 in case VM needs more.
>
>
>> I do not think that we should get resolution for each clock, the COARSE
>> clock should have the worst resolution of all clocks.
>>
>
> Yes, quite enough for the rest.
> For precise clock, the second part will be zero since divided by 1000000.
>
>     delta = 5 + (res.tv_nsec / 1000000) * 5;
>


My concern here is that seems a bit looser to the rest clocks.
Because "10ms" as delta is a big value to the precise clock since
the resolution of some is only 1ns.
(The delta will be "10ms" on x86_64 and "15ms" on aarch64.)

In my path V2, we can make the second part _zero_ by dividing 1000000.
Then we can keep using the proper delta for each clock.


Test data from x86_64:
========

clock_gettime04.c:108: TINFO: CLOCK_REALTIME: resolution is 1ns
clock_gettime04.c:112: TINFO: delta = : 5
...
clock_gettime04.c:108: TINFO: CLOCK_REALTIME_COARSE: resolution is 1000000ns
clock_gettime04.c:112: TINFO: delta = : 10
clock_gettime04.c:108: TINFO: CLOCK_MONOTONIC: resolution is 1ns
clock_gettime04.c:112: TINFO: delta = : 5
clock_gettime04.c:108: TINFO: CLOCK_MONOTONIC_COARSE: resolution is
1000000ns
clock_gettime04.c:112: TINFO: delta = : 10
clock_gettime04.c:108: TINFO: CLOCK_MONOTONIC_RAW: resolution is 1ns
clock_gettime04.c:112: TINFO: delta = : 5
clock_gettime04.c:108: TINFO: CLOCK_BOOTTIME: resolution is 1ns
clock_gettime04.c:112: TINFO: delta = : 5



>
>
> Test data from aarch4 (kernel-5.14) FYI:
> ===================
> clock_gettime04.c:108: TINFO: CLOCK_REALTIME: resolution is 1ns
> ...
> clock_gettime04.c:108: TINFO: CLOCK_REALTIME_COARSE: resolution is
> 10000000ns
> clock_gettime04.c:108: TINFO: CLOCK_MONOTONIC: resolution is 1ns
> clock_gettime04.c:108: TINFO: CLOCK_MONOTONIC_COARSE: resolution is
> 10000000ns
> clock_gettime04.c:108: TINFO: CLOCK_MONOTONIC_RAW: resolution is 1ns
> clock_gettime04.c:108: TINFO: CLOCK_BOOTTIME: resolution is 1ns
>
>
> --
> Regards,
> Li Wang
>


-- 
Regards,
Li Wang

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

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


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

Re: [LTP] [PATCH] clock_gettime04: set threshold based on the clock tick rate

[Li Wang]

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

> My concern here is that seems a bit looser to the rest clocks.
> Because "10ms" as delta is a big value to the precise clock since
> the resolution of some is only 1ns.
> (The delta will be "10ms" on x86_64 and "15ms" on aarch64.)
>

delta will be 55ms (not 15ms) on aarch64, this is too big  ^

-- 
Regards,
Li Wang

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

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


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

Re: [LTP] [PATCH] clock_gettime04: set threshold based on the clock tick rate

[Cyril Hrubis]

Hi!
Thinking of it again I think that:

- the COARSE clocks runs on jiffies, the rest of the clocks may use high
  resolution if available

- we need two deltas, one for the coarse clocks and one for the rest

- the delta should generally be clock resolution + epsion (5ms)

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] clock_gettime04: set threshold based on the clock tick rate

[Waiman Long]

On 3/25/22 00:00, 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 set the threshold no less than each clock tick in millisecond:
>
> 	delta = 1000(ms)/ticks_number_per_sec + 5;
>
> 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>
> ---
>   testcases/kernel/syscalls/clock_gettime/clock_gettime04.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c b/testcases/kernel/syscalls/clock_gettime/clock_gettime04.c
> index a8d2c5b38..cccbc9383 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;
>   
>   static inline int do_vdso_gettime(gettime_t vdso, clockid_t clk_id, void *ts)
>   {
> @@ -92,6 +92,7 @@ static struct time64_variants variants[] = {
>   
>   static void setup(void)
>   {
> +	delta = 1000/sysconf(_SC_CLK_TCK) + 5;
>   	if (tst_is_virt(VIRT_ANY)) {
>   		tst_res(TINFO, "Running in a virtual machine, multiply the delta by 10.");
>   		delta *= 10;

Actually, only CLOCK_REALTIME_COARSE and CLOCK_MONOTONIC_COARSE use 
clock tick for its time measurement. The rests use a higher resolution 
internal timer. Perhaps, we could have a separate coarse_delta for these 
two cases and use delta for the rest.

Cheers,
Longman


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

Re: [LTP] [PATCH] clock_gettime04: set threshold based on the clock tick rate

[Li Wang]

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

Waiman Long <longman@redhat.com> wrote:


> >   static void setup(void)
> >   {
> > +     delta = 1000/sysconf(_SC_CLK_TCK) + 5;
> >       if (tst_is_virt(VIRT_ANY)) {
> >               tst_res(TINFO, "Running in a virtual machine, multiply the
> delta by 10.");
> >               delta *= 10;
>
> Actually, only CLOCK_REALTIME_COARSE and CLOCK_MONOTONIC_COARSE use
> clock tick for its time measurement. The rests use a higher resolution
> internal timer. Perhaps, we could have a separate coarse_delta for these
> two cases and use delta for the rest.
>

Good point, at least for aarch64 the CLOCK*_COARSE makes use
of the clock tick is 10ms.

However, as many architectures have varying tick rates and use
dynamically timer, maybe a further method is to get it via clock_getres()
just like Cyril mentioned in another email.

(I draft patch V2 in that email, feel free to review)

-- 
Regards,
Li Wang

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

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


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