[LTP] [PATCH 1/2] setitimer01: add interval timer test

[LTP] [PATCH 1/2] setitimer01: add interval timer test

[Li Wang]

Split checking the return ovalue from testing the signal is
delivered, so that we could use two time value for verifying.

Also, adding interval timer test by handling the signal at
least 10 times. After that recover the signal behavior to
default and do deliver-signal checking.

Signed-off-by: Li Wang <liwang@redhat.com>
---
 .../kernel/syscalls/setitimer/setitimer01.c   | 63 ++++++++++++-------
 1 file changed, 39 insertions(+), 24 deletions(-)

diff --git a/testcases/kernel/syscalls/setitimer/setitimer01.c b/testcases/kernel/syscalls/setitimer/setitimer01.c
index 1f631d457..260590b0e 100644
--- a/testcases/kernel/syscalls/setitimer/setitimer01.c
+++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
@@ -22,8 +22,10 @@
 #include "tst_safe_clocks.h"
 
 static struct itimerval *value, *ovalue;
+static volatile unsigned long sigcnt;
 static long time_step;
-static long time_count;
+static long time_sec;
+static long time_usec;
 
 static struct tcase {
 	int which;
@@ -40,54 +42,66 @@ static int sys_setitimer(int which, void *new_value, void *old_value)
 	return tst_syscall(__NR_setitimer, which, new_value, old_value);
 }
 
-static void set_setitimer_value(int usec, int o_usec)
+static void sig_routine(int signo LTP_ATTRIBUTE_UNUSED)
 {
-	value->it_value.tv_sec = 0;
-	value->it_value.tv_usec = usec;
-	value->it_interval.tv_sec = 0;
-	value->it_interval.tv_usec = 0;
+	sigcnt++;
+}
 
-	ovalue->it_value.tv_sec = o_usec;
-	ovalue->it_value.tv_usec = o_usec;
-	ovalue->it_interval.tv_sec = 0;
-	ovalue->it_interval.tv_usec = 0;
+static void set_setitimer_value(int sec, int usec)
+{
+	value->it_value.tv_sec = sec;
+	value->it_value.tv_usec = usec;
+	value->it_interval.tv_sec = sec;
+	value->it_interval.tv_usec = usec;
 }
 
 static void verify_setitimer(unsigned int i)
 {
 	pid_t pid;
 	int status;
-	long margin;
 	struct tcase *tc = &tcases[i];
 
+	tst_res(TINFO, "tc->which = %s", tc->des);
+
 	pid = SAFE_FORK();
 
 	if (pid == 0) {
-		tst_res(TINFO, "tc->which = %s", tc->des);
-
 		tst_no_corefile(0);
 
-		set_setitimer_value(time_count, 0);
+		set_setitimer_value(time_sec, time_usec);
 		TST_EXP_PASS(sys_setitimer(tc->which, value, NULL));
 
-		set_setitimer_value(5 * time_step, 7 * time_step);
+		set_setitimer_value(2 * time_sec, 2 * time_usec);
 		TST_EXP_PASS(sys_setitimer(tc->which, value, ovalue));
 
-		tst_res(TINFO, "tv_sec=%ld, tv_usec=%ld",
-			ovalue->it_value.tv_sec,
-			ovalue->it_value.tv_usec);
+		TST_EXP_EQ_LI(ovalue->it_interval.tv_sec, time_sec);
+		TST_EXP_EQ_LI(ovalue->it_interval.tv_usec, time_usec);
+
+		tst_res(TINFO, "ovalue->it_value.tv_sec=%ld, ovalue->it_value.tv_usec=%ld",
+			ovalue->it_value.tv_sec, ovalue->it_value.tv_usec);
 
 		/*
 		 * ITIMER_VIRTUAL and ITIMER_PROF timers always expire a
 		 * time_step afterward the elapsed time to make sure that
 		 * at least counters take effect.
 		 */
-		margin = tc->which == ITIMER_REAL ? 0 : time_step;
+		long margin = (tc->which == ITIMER_REAL) ? 0 : time_step;
 
-		if (ovalue->it_value.tv_sec != 0 || ovalue->it_value.tv_usec > time_count + margin)
+		if (ovalue->it_value.tv_sec > time_sec ||
+				ovalue->it_value.tv_usec > time_usec + margin)
 			tst_res(TFAIL, "Ending counters are out of range");
 
-		for (;;)
+		SAFE_SIGNAL(tc->signo, sig_routine);
+
+		set_setitimer_value(0, time_usec);
+		TST_EXP_PASS(sys_setitimer(tc->which, value, NULL));
+
+		while (sigcnt <= 10UL)
+			;
+
+		SAFE_SIGNAL(tc->signo, SIG_DFL);
+
+		while (1)
 			;
 	}
 
@@ -114,10 +128,11 @@ static void setup(void)
 	if (time_step <= 0)
 		time_step = 1000;
 
-	time_count = 3 * time_step;
+	tst_res(TINFO, "clock low-resolution: %luns, time step: %luus",
+		time_res.tv_nsec, time_step);
 
-	tst_res(TINFO, "low-resolution: %luns, time step: %luus, time count: %luus",
-		time_res.tv_nsec, time_step, time_count);
+	time_sec  = 9 + time_step / 1000;
+	time_usec = 3 * time_step;
 }
 
 static struct tst_test test = {
-- 
2.35.3


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

[LTP] [PATCH 2/2] getitimer01: add checking for nonzero timer

[Li Wang]

By default a new process disabled the timer and getitimer()
returned zero value. But we also need to check if the timer
is correct when reset to nonzero.

Signed-off-by: Li Wang <liwang@redhat.com>
---

Notes:
      The reason for using jiffy instead of time_step is that it only
      checks the timer is set expectedly but not really expired. So we
      use a rough value in the macro definition is enough.

 .../kernel/syscalls/getitimer/getitimer01.c   | 84 ++++++++++++++++---
 1 file changed, 71 insertions(+), 13 deletions(-)

diff --git a/testcases/kernel/syscalls/getitimer/getitimer01.c b/testcases/kernel/syscalls/getitimer/getitimer01.c
index 5ecfac55c..a49f63a85 100644
--- a/testcases/kernel/syscalls/getitimer/getitimer01.c
+++ b/testcases/kernel/syscalls/getitimer/getitimer01.c
@@ -12,25 +12,83 @@
  */
 
 #include "tst_test.h"
+#include "tst_safe_clocks.h"
 
-static int itimer_name[] = {
-	ITIMER_REAL,
-	ITIMER_VIRTUAL,
-	ITIMER_PROF,
+#define SEC  100
+#define USEC 10000
+
+static struct itimerval *value;
+static long jiffy;
+
+static struct tcase {
+	int which;
+	char *des;
+} tcases[] = {
+	{ITIMER_REAL,    "ITIMER_REAL"},
+	{ITIMER_VIRTUAL, "ITIMER_VIRTUAL"},
+	{ITIMER_PROF,    "ITIMER_PROF"},
 };
 
-static void run(void)
+static void set_setitimer_value(int sec, int usec)
 {
-	long unsigned int i;
-	struct itimerval value;
+	value->it_value.tv_sec = sec;
+	value->it_value.tv_usec = usec;
+	value->it_interval.tv_sec = sec;
+	value->it_interval.tv_usec = usec;
+}
 
-	for (i = 0; i < ARRAY_SIZE(itimer_name); i++) {
-		TST_EXP_PASS(getitimer(itimer_name[i], &value));
-		TST_EXP_EQ_LI(value.it_value.tv_sec, 0);
-		TST_EXP_EQ_LI(value.it_value.tv_usec, 0);
-	}
+static void verify_getitimer(unsigned int i)
+{
+	struct tcase *tc = &tcases[i];
+
+	tst_res(TINFO, "tc->which = %s", tc->des);
+
+	TST_EXP_PASS(getitimer(tc->which, value));
+	TST_EXP_EQ_LI(value->it_value.tv_sec, 0);
+	TST_EXP_EQ_LI(value->it_value.tv_usec, 0);
+	TST_EXP_EQ_LI(value->it_interval.tv_sec, 0);
+	TST_EXP_EQ_LI(value->it_interval.tv_usec, 0);
+
+	set_setitimer_value(SEC, USEC);
+	TST_EXP_PASS(setitimer(tc->which, value, NULL));
+
+	set_setitimer_value(0, 0);
+	TST_EXP_PASS(getitimer(tc->which, value));
+
+	TST_EXP_EQ_LI(value->it_interval.tv_sec, SEC);
+	TST_EXP_EQ_LI(value->it_interval.tv_usec, USEC);
+
+	tst_res(TINFO, "value->it_value.tv_sec=%ld, value->it_value.tv_usec=%ld",
+			value->it_value.tv_sec, value->it_value.tv_usec);
+
+	/*
+	 * ITIMER_VIRTUAL and ITIMER_PROF timers always expire a
+	 * TICK_NSEC (jiffy) afterward the elapsed time to make
+	 * sure that at least time counters take effect.
+	 */
+	long margin = (tc->which == ITIMER_REAL) ? 0 : jiffy;
+
+	if (value->it_value.tv_sec > SEC ||
+			value->it_value.tv_usec > USEC + margin)
+		tst_res(TFAIL, "timer value is not within the expected range");
+	else
+		tst_res(TPASS, "timer value is within the expected range");
+}
+
+static void setup(void)
+{
+	struct timespec time_res;
+
+	SAFE_CLOCK_GETRES(CLOCK_MONOTONIC_COARSE, &time_res);
+	jiffy = (time_res.tv_nsec + 999) / 1000;
 }
 
 static struct tst_test test = {
-	.test_all = run
+	.tcnt = ARRAY_SIZE(tcases),
+	.setup = setup,
+	.test = verify_getitimer,
+	.bufs = (struct tst_buffers[]) {
+		{&value,  .size = sizeof(struct itimerval)},
+		{}
+	}
 };
-- 
2.35.3


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

Re: [LTP] [PATCH 1/2] setitimer01: add interval timer test

[Richard Palethorpe]

Hello,

Li Wang <liwang@redhat.com> writes:

> Split checking the return ovalue from testing the signal is
> delivered, so that we could use two time value for verifying.
>
> Also, adding interval timer test by handling the signal at
> least 10 times. After that recover the signal behavior to
> default and do deliver-signal checking.
>
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>  .../kernel/syscalls/setitimer/setitimer01.c   | 63 ++++++++++++-------
>  1 file changed, 39 insertions(+), 24 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/setitimer/setitimer01.c b/testcases/kernel/syscalls/setitimer/setitimer01.c
> index 1f631d457..260590b0e 100644
> --- a/testcases/kernel/syscalls/setitimer/setitimer01.c
> +++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
> @@ -22,8 +22,10 @@
>  #include "tst_safe_clocks.h"
>  
>  static struct itimerval *value, *ovalue;
> +static volatile unsigned long sigcnt;
>  static long time_step;
> -static long time_count;
> +static long time_sec;
> +static long time_usec;
>  
>  static struct tcase {
>  	int which;
> @@ -40,54 +42,66 @@ static int sys_setitimer(int which, void *new_value, void *old_value)
>  	return tst_syscall(__NR_setitimer, which, new_value, old_value);
>  }
>  
> -static void set_setitimer_value(int usec, int o_usec)
> +static void sig_routine(int signo LTP_ATTRIBUTE_UNUSED)
>  {
> -	value->it_value.tv_sec = 0;
> -	value->it_value.tv_usec = usec;
> -	value->it_interval.tv_sec = 0;
> -	value->it_interval.tv_usec = 0;
> +	sigcnt++;
> +}
>  
> -	ovalue->it_value.tv_sec = o_usec;
> -	ovalue->it_value.tv_usec = o_usec;
> -	ovalue->it_interval.tv_sec = 0;
> -	ovalue->it_interval.tv_usec = 0;
> +static void set_setitimer_value(int sec, int usec)
> +{
> +	value->it_value.tv_sec = sec;
> +	value->it_value.tv_usec = usec;
> +	value->it_interval.tv_sec = sec;
> +	value->it_interval.tv_usec = usec;
>  }
>  
>  static void verify_setitimer(unsigned int i)
>  {
>  	pid_t pid;
>  	int status;
> -	long margin;
>  	struct tcase *tc = &tcases[i];
>  
> +	tst_res(TINFO, "tc->which = %s", tc->des);
> +
>  	pid = SAFE_FORK();
>  
>  	if (pid == 0) {
> -		tst_res(TINFO, "tc->which = %s", tc->des);
> -
>  		tst_no_corefile(0);
>  
> -		set_setitimer_value(time_count, 0);
> +		set_setitimer_value(time_sec, time_usec);
>  		TST_EXP_PASS(sys_setitimer(tc->which, value, NULL));
>  
> -		set_setitimer_value(5 * time_step, 7 * time_step);
> +		set_setitimer_value(2 * time_sec, 2 * time_usec);

IDK if there was some reason for choosing 5 and 7 in the first place?

Andrea seemed to be going through the prime numbers.

>  		TST_EXP_PASS(sys_setitimer(tc->which, value, ovalue));
>  
> -		tst_res(TINFO, "tv_sec=%ld, tv_usec=%ld",
> -			ovalue->it_value.tv_sec,
> -			ovalue->it_value.tv_usec);
> +		TST_EXP_EQ_LI(ovalue->it_interval.tv_sec, time_sec);
> +		TST_EXP_EQ_LI(ovalue->it_interval.tv_usec, time_usec);
> +
> +		tst_res(TINFO, "ovalue->it_value.tv_sec=%ld, ovalue->it_value.tv_usec=%ld",
> +			ovalue->it_value.tv_sec, ovalue->it_value.tv_usec);
>  
>  		/*
>  		 * ITIMER_VIRTUAL and ITIMER_PROF timers always expire a
>  		 * time_step afterward the elapsed time to make sure that
>  		 * at least counters take effect.
>  		 */
> -		margin = tc->which == ITIMER_REAL ? 0 : time_step;
> +		long margin = (tc->which == ITIMER_REAL) ? 0 : time_step;

Looks like an unecessary change?

>  
> -		if (ovalue->it_value.tv_sec != 0 || ovalue->it_value.tv_usec > time_count + margin)
> +		if (ovalue->it_value.tv_sec > time_sec ||
> +				ovalue->it_value.tv_usec > time_usec + margin)

Looking at the man page, technically speaking the valid range for
ovalue->it_value.tv_sec is 0 to value->it_value.tv_sec when
ovalue->it_value.tv_usec > 0.

Practically speaking we have to assume a large amount of time has passed
when using ITIMER_REAL. It has to be *much* larger than a VM is likely
to be paused for and then successfully resumed. Or the amount of time a
clock may be corrected by (for e.g. with NTP).

>  			tst_res(TFAIL, "Ending counters are out of range");

While we are here; we should make our lives easier when the test fails
by printing any relevant values.

-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH 2/2] getitimer01: add checking for nonzero timer

[Richard Palethorpe]

Hello,

Li Wang <liwang@redhat.com> writes:

> By default a new process disabled the timer and getitimer()
> returned zero value. But we also need to check if the timer
> is correct when reset to nonzero.
>
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>
> Notes:
>       The reason for using jiffy instead of time_step is that it only
>       checks the timer is set expectedly but not really expired. So we
>       use a rough value in the macro definition is enough.
>
>  .../kernel/syscalls/getitimer/getitimer01.c   | 84 ++++++++++++++++---
>  1 file changed, 71 insertions(+), 13 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/getitimer/getitimer01.c b/testcases/kernel/syscalls/getitimer/getitimer01.c
> index 5ecfac55c..a49f63a85 100644
> --- a/testcases/kernel/syscalls/getitimer/getitimer01.c
> +++ b/testcases/kernel/syscalls/getitimer/getitimer01.c
> @@ -12,25 +12,83 @@
>   */
>  
>  #include "tst_test.h"
> +#include "tst_safe_clocks.h"
>  
> -static int itimer_name[] = {
> -	ITIMER_REAL,
> -	ITIMER_VIRTUAL,
> -	ITIMER_PROF,
> +#define SEC  100
> +#define USEC 10000
> +
> +static struct itimerval *value;
> +static long jiffy;
> +
> +static struct tcase {
> +	int which;
> +	char *des;
> +} tcases[] = {
> +	{ITIMER_REAL,    "ITIMER_REAL"},
> +	{ITIMER_VIRTUAL, "ITIMER_VIRTUAL"},
> +	{ITIMER_PROF,    "ITIMER_PROF"},
>  };
>  
> -static void run(void)
> +static void set_setitimer_value(int sec, int usec)
>  {
> -	long unsigned int i;
> -	struct itimerval value;
> +	value->it_value.tv_sec = sec;
> +	value->it_value.tv_usec = usec;
> +	value->it_interval.tv_sec = sec;
> +	value->it_interval.tv_usec = usec;
> +}
>  
> -	for (i = 0; i < ARRAY_SIZE(itimer_name); i++) {
> -		TST_EXP_PASS(getitimer(itimer_name[i], &value));
> -		TST_EXP_EQ_LI(value.it_value.tv_sec, 0);
> -		TST_EXP_EQ_LI(value.it_value.tv_usec, 0);
> -	}
> +static void verify_getitimer(unsigned int i)
> +{
> +	struct tcase *tc = &tcases[i];
> +
> +	tst_res(TINFO, "tc->which = %s", tc->des);
> +
> +	TST_EXP_PASS(getitimer(tc->which, value));
> +	TST_EXP_EQ_LI(value->it_value.tv_sec, 0);
> +	TST_EXP_EQ_LI(value->it_value.tv_usec, 0);
> +	TST_EXP_EQ_LI(value->it_interval.tv_sec, 0);
> +	TST_EXP_EQ_LI(value->it_interval.tv_usec, 0);
> +
> +	set_setitimer_value(SEC, USEC);
> +	TST_EXP_PASS(setitimer(tc->which, value, NULL));
> +
> +	set_setitimer_value(0, 0);
> +	TST_EXP_PASS(getitimer(tc->which, value));
> +
> +	TST_EXP_EQ_LI(value->it_interval.tv_sec, SEC);
> +	TST_EXP_EQ_LI(value->it_interval.tv_usec, USEC);
> +
> +	tst_res(TINFO, "value->it_value.tv_sec=%ld, value->it_value.tv_usec=%ld",
> +			value->it_value.tv_sec, value->it_value.tv_usec);
> +
> +	/*
> +	 * ITIMER_VIRTUAL and ITIMER_PROF timers always expire a
> +	 * TICK_NSEC (jiffy) afterward the elapsed time to make
> +	 * sure that at least time counters take effect.
> +	 */
> +	long margin = (tc->which == ITIMER_REAL) ? 0 : jiffy;
> +
> +	if (value->it_value.tv_sec > SEC ||
> +			value->it_value.tv_usec > USEC + margin)

If a second is able to elapse then it_value.tv_usec can be greater than
USEC + margin. Or is there something I am missing?

I guess again there is the issue with wall clock time jumping around.

> +		tst_res(TFAIL, "timer value is not within the expected range");
> +	else
> +		tst_res(TPASS, "timer value is within the expected
> range");

Also we want to print all the relevant values in case of failure.

> +}
> +
> +static void setup(void)
> +{
> +	struct timespec time_res;
> +
> +	SAFE_CLOCK_GETRES(CLOCK_MONOTONIC_COARSE, &time_res);
> +	jiffy = (time_res.tv_nsec + 999) / 1000;
>  }
>  
>  static struct tst_test test = {
> -	.test_all = run
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.setup = setup,
> +	.test = verify_getitimer,
> +	.bufs = (struct tst_buffers[]) {
> +		{&value,  .size = sizeof(struct itimerval)},
> +		{}
> +	}
>  };
> -- 
> 2.35.3


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH 1/2] setitimer01: add interval timer test

[Li Wang]

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

On Mon, Nov 14, 2022 at 11:52 PM Richard Palethorpe <rpalethorpe@suse.de>
wrote:

> Hello,
>
> Li Wang <liwang@redhat.com> writes:
>
> > Split checking the return ovalue from testing the signal is
> > delivered, so that we could use two time value for verifying.
> >
> > Also, adding interval timer test by handling the signal at
> > least 10 times. After that recover the signal behavior to
> > default and do deliver-signal checking.
> >
> > Signed-off-by: Li Wang <liwang@redhat.com>
> > ---
> >  .../kernel/syscalls/setitimer/setitimer01.c   | 63 ++++++++++++-------
> >  1 file changed, 39 insertions(+), 24 deletions(-)
> >
> > diff --git a/testcases/kernel/syscalls/setitimer/setitimer01.c
> b/testcases/kernel/syscalls/setitimer/setitimer01.c
> > index 1f631d457..260590b0e 100644
> > --- a/testcases/kernel/syscalls/setitimer/setitimer01.c
> > +++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
> > @@ -22,8 +22,10 @@
> >  #include "tst_safe_clocks.h"
> >
> >  static struct itimerval *value, *ovalue;
> > +static volatile unsigned long sigcnt;
> >  static long time_step;
> > -static long time_count;
> > +static long time_sec;
> > +static long time_usec;
> >
> >  static struct tcase {
> >       int which;
> > @@ -40,54 +42,66 @@ static int sys_setitimer(int which, void *new_value,
> void *old_value)
> >       return tst_syscall(__NR_setitimer, which, new_value, old_value);
> >  }
> >
> > -static void set_setitimer_value(int usec, int o_usec)
> > +static void sig_routine(int signo LTP_ATTRIBUTE_UNUSED)
> >  {
> > -     value->it_value.tv_sec = 0;
> > -     value->it_value.tv_usec = usec;
> > -     value->it_interval.tv_sec = 0;
> > -     value->it_interval.tv_usec = 0;
> > +     sigcnt++;
> > +}
> >
> > -     ovalue->it_value.tv_sec = o_usec;
> > -     ovalue->it_value.tv_usec = o_usec;
> > -     ovalue->it_interval.tv_sec = 0;
> > -     ovalue->it_interval.tv_usec = 0;
> > +static void set_setitimer_value(int sec, int usec)
> > +{
> > +     value->it_value.tv_sec = sec;
> > +     value->it_value.tv_usec = usec;
> > +     value->it_interval.tv_sec = sec;
> > +     value->it_interval.tv_usec = usec;
> >  }
> >
> >  static void verify_setitimer(unsigned int i)
> >  {
> >       pid_t pid;
> >       int status;
> > -     long margin;
> >       struct tcase *tc = &tcases[i];
> >
> > +     tst_res(TINFO, "tc->which = %s", tc->des);
> > +
> >       pid = SAFE_FORK();
> >
> >       if (pid == 0) {
> > -             tst_res(TINFO, "tc->which = %s", tc->des);
> > -
> >               tst_no_corefile(0);
> >
> > -             set_setitimer_value(time_count, 0);
> > +             set_setitimer_value(time_sec, time_usec);
> >               TST_EXP_PASS(sys_setitimer(tc->which, value, NULL));
> >
> > -             set_setitimer_value(5 * time_step, 7 * time_step);
> > +             set_setitimer_value(2 * time_sec, 2 * time_usec);
>
> IDK if there was some reason for choosing 5 and 7 in the first place?
>

I don't see any necessary reasons for using prime numbers here.

@Andrea, did I miss something?



>
> Andrea seemed to be going through the prime numbers.
>
> >               TST_EXP_PASS(sys_setitimer(tc->which, value, ovalue));
> >
> > -             tst_res(TINFO, "tv_sec=%ld, tv_usec=%ld",
> > -                     ovalue->it_value.tv_sec,
> > -                     ovalue->it_value.tv_usec);
> > +             TST_EXP_EQ_LI(ovalue->it_interval.tv_sec, time_sec);
> > +             TST_EXP_EQ_LI(ovalue->it_interval.tv_usec, time_usec);
> > +
> > +             tst_res(TINFO, "ovalue->it_value.tv_sec=%ld,
> ovalue->it_value.tv_usec=%ld",
> > +                     ovalue->it_value.tv_sec, ovalue->it_value.tv_usec);
> >
> >               /*
> >                * ITIMER_VIRTUAL and ITIMER_PROF timers always expire a
> >                * time_step afterward the elapsed time to make sure that
> >                * at least counters take effect.
> >                */
> > -             margin = tc->which == ITIMER_REAL ? 0 : time_step;
> > +             long margin = (tc->which == ITIMER_REAL) ? 0 : time_step;
>
> Looks like an unecessary change?
>

Yes, but the 'margin' is only used in children, and I moved
the declaration here is just to highlight and cause attention
in code reading.



>
> >
> > -             if (ovalue->it_value.tv_sec != 0 ||
> ovalue->it_value.tv_usec > time_count + margin)
> > +             if (ovalue->it_value.tv_sec > time_sec ||
> > +                             ovalue->it_value.tv_usec > time_usec +
> margin)
>
> Looking at the man page, technically speaking the valid range for
> ovalue->it_value.tv_sec is 0 to value->it_value.tv_sec when
> ovalue->it_value.tv_usec > 0.
>

Good point!! Seems we have to split the situation into two,

1. no tv_sec elapse happen, just check
    'it_value.tv_usec' within [0, time_usec + margin]

2. tv_sec elapses happen, then check
    'it_value.tv_sec' within [0, time_sec]

Something maybe like:

--- a/testcases/kernel/syscalls/setitimer/setitimer01.c
+++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
@@ -87,9 +87,17 @@ static void verify_setitimer(unsigned int i)
                 */
                long margin = (tc->which == ITIMER_REAL) ? 0 : time_step;

-               if (ovalue->it_value.tv_sec > time_sec ||
-                               ovalue->it_value.tv_usec > time_usec +
margin)
-                       tst_res(TFAIL, "Ending counters are out of range");
+               if (ovalue->it_value.tv_sec == time_sec) {
+                       if (ovalue->it_value.tv_usec < 0 ||
+                                       ovalue->it_value.tv_usec >
time_usec + margin)
+                               tst_res(TFAIL, "ovalue->it_value.tv_usec is
out of range: %ld",
+                                               ovalue->it_value.tv_usec);
+               } else {
+                       if (ovalue->it_value.tv_sec < 0 ||
+                                       ovalue->it_value.tv_sec > time_sec)
+                               tst_res(TFAIL, "ovalue->it_value.tv_sec is
out of range: %ld",
+                                               ovalue->it_value.tv_usec);
+               }

                SAFE_SIGNAL(tc->signo, sig_routine);



>
> Practically speaking we have to assume a large amount of time has passed
> when using ITIMER_REAL. It has to be *much* larger than a VM is likely
> to be paused for and then successfully resumed. Or the amount of time a
> clock may be corrected by (for e.g. with NTP).
>

Hmm, not sure if I understand correctly above, will that
timer value be out of the range but reasonable?

Or is there any additional situation we should cover?



>
> >                       tst_res(TFAIL, "Ending counters are out of range");
>
> While we are here; we should make our lives easier when the test fails
> by printing any relevant values.
>

We already do that in the above print, but it's fine to have that here as
well.

-- 
Regards,
Li Wang

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

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


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

Re: [LTP] [PATCH 1/2] setitimer01: add interval timer test

[Richard Palethorpe]

Hello,

Li Wang <liwang@redhat.com> writes:

> On Mon, Nov 14, 2022 at 11:52 PM Richard Palethorpe <rpalethorpe@suse.de> wrote:
>
>  Hello,
>
>  Li Wang <liwang@redhat.com> writes:
>
>  > Split checking the return ovalue from testing the signal is
>  > delivered, so that we could use two time value for verifying.
>  >
>  > Also, adding interval timer test by handling the signal at
>  > least 10 times. After that recover the signal behavior to
>  > default and do deliver-signal checking.
>  >
>  > Signed-off-by: Li Wang <liwang@redhat.com>
>  > ---
>  >  .../kernel/syscalls/setitimer/setitimer01.c   | 63 ++++++++++++-------
>  >  1 file changed, 39 insertions(+), 24 deletions(-)
>  >
>  > diff --git a/testcases/kernel/syscalls/setitimer/setitimer01.c b/testcases/kernel/syscalls/setitimer/setitimer01.c
>  > index 1f631d457..260590b0e 100644
>  > --- a/testcases/kernel/syscalls/setitimer/setitimer01.c
>  > +++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
>  > @@ -22,8 +22,10 @@
>  >  #include "tst_safe_clocks.h"
>  >  
>  >  static struct itimerval *value, *ovalue;
>  > +static volatile unsigned long sigcnt;
>  >  static long time_step;
>  > -static long time_count;
>  > +static long time_sec;
>  > +static long time_usec;
>  >  
>  >  static struct tcase {
>  >       int which;
>  > @@ -40,54 +42,66 @@ static int sys_setitimer(int which, void *new_value, void *old_value)
>  >       return tst_syscall(__NR_setitimer, which, new_value, old_value);
>  >  }
>  >  
>  > -static void set_setitimer_value(int usec, int o_usec)
>  > +static void sig_routine(int signo LTP_ATTRIBUTE_UNUSED)
>  >  {
>  > -     value->it_value.tv_sec = 0;
>  > -     value->it_value.tv_usec = usec;
>  > -     value->it_interval.tv_sec = 0;
>  > -     value->it_interval.tv_usec = 0;
>  > +     sigcnt++;
>  > +}
>  >  
>  > -     ovalue->it_value.tv_sec = o_usec;
>  > -     ovalue->it_value.tv_usec = o_usec;
>  > -     ovalue->it_interval.tv_sec = 0;
>  > -     ovalue->it_interval.tv_usec = 0;
>  > +static void set_setitimer_value(int sec, int usec)
>  > +{
>  > +     value->it_value.tv_sec = sec;
>  > +     value->it_value.tv_usec = usec;
>  > +     value->it_interval.tv_sec = sec;
>  > +     value->it_interval.tv_usec = usec;
>  >  }
>  >  
>  >  static void verify_setitimer(unsigned int i)
>  >  {
>  >       pid_t pid;
>  >       int status;
>  > -     long margin;
>  >       struct tcase *tc = &tcases[i];
>  >  
>  > +     tst_res(TINFO, "tc->which = %s", tc->des);
>  > +
>  >       pid = SAFE_FORK();
>  >  
>  >       if (pid == 0) {
>  > -             tst_res(TINFO, "tc->which = %s", tc->des);
>  > -
>  >               tst_no_corefile(0);
>  >  
>  > -             set_setitimer_value(time_count, 0);
>  > +             set_setitimer_value(time_sec, time_usec);
>  >               TST_EXP_PASS(sys_setitimer(tc->which, value, NULL));
>  >  
>  > -             set_setitimer_value(5 * time_step, 7 * time_step);
>  > +             set_setitimer_value(2 * time_sec, 2 * time_usec);
>
>  IDK if there was some reason for choosing 5 and 7 in the first place?
>
> I don't see any necessary reasons for using prime numbers here.
>
> @Andrea, did I miss something?
>
>  
>  
>  Andrea seemed to be going through the prime numbers.
>
>  >               TST_EXP_PASS(sys_setitimer(tc->which, value, ovalue));
>  >  
>  > -             tst_res(TINFO, "tv_sec=%ld, tv_usec=%ld",
>  > -                     ovalue->it_value.tv_sec,
>  > -                     ovalue->it_value.tv_usec);
>  > +             TST_EXP_EQ_LI(ovalue->it_interval.tv_sec, time_sec);
>  > +             TST_EXP_EQ_LI(ovalue->it_interval.tv_usec, time_usec);
>  > +
>  > +             tst_res(TINFO, "ovalue->it_value.tv_sec=%ld, ovalue->it_value.tv_usec=%ld",
>  > +                     ovalue->it_value.tv_sec, ovalue->it_value.tv_usec);
>  >  
>  >               /*
>  >                * ITIMER_VIRTUAL and ITIMER_PROF timers always expire a
>  >                * time_step afterward the elapsed time to make sure that
>  >                * at least counters take effect.
>  >                */
>  > -             margin = tc->which == ITIMER_REAL ? 0 : time_step;
>  > +             long margin = (tc->which == ITIMER_REAL) ? 0 : time_step;
>
>  Looks like an unecessary change?
>
> Yes, but the 'margin' is only used in children, and I moved
> the declaration here is just to highlight and cause attention
> in code reading.

It's a valid change, but the benefit is very minor IMO. I would prefer
to have the 'git blame' show the original commit which added this logic.

>
>  
>  
>  >  
>  > -             if (ovalue->it_value.tv_sec != 0 || ovalue->it_value.tv_usec > time_count + margin)
>  > +             if (ovalue->it_value.tv_sec > time_sec ||
>  > +                             ovalue->it_value.tv_usec > time_usec + margin)
>
>  Looking at the man page, technically speaking the valid range for
>  ovalue->it_value.tv_sec is 0 to value->it_value.tv_sec when
>  ovalue->it_value.tv_usec > 0.
>
> Good point!! Seems we have to split the situation into two,
>
> 1. no tv_sec elapse happen, just check
>     'it_value.tv_usec' within [0, time_usec + margin]
>
> 2. tv_sec elapses happen, then check 
>     'it_value.tv_sec' within [0, time_sec]
>
> Something maybe like:
>
> --- a/testcases/kernel/syscalls/setitimer/setitimer01.c
> +++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
> @@ -87,9 +87,17 @@ static void verify_setitimer(unsigned int i)
>                  */
>                 long margin = (tc->which == ITIMER_REAL) ? 0 : time_step;
>  
> -               if (ovalue->it_value.tv_sec > time_sec ||
> -                               ovalue->it_value.tv_usec > time_usec + margin)
> -                       tst_res(TFAIL, "Ending counters are out of range");
> +               if (ovalue->it_value.tv_sec == time_sec) {
> +                       if (ovalue->it_value.tv_usec < 0 ||
> +                                       ovalue->it_value.tv_usec > time_usec + margin)
> +                               tst_res(TFAIL, "ovalue->it_value.tv_usec is out of range: %ld",
> +                                               ovalue->it_value.tv_usec);
> +               } else {
> +                       if (ovalue->it_value.tv_sec < 0 ||
> +                                       ovalue->it_value.tv_sec > time_sec)
> +                               tst_res(TFAIL, "ovalue->it_value.tv_sec is out of range: %ld",
> +                                               ovalue->it_value.tv_usec);
> +               }

Yes, I think this is correct.

>  
>                 SAFE_SIGNAL(tc->signo, sig_routine);
>
>  
>  
>  Practically speaking we have to assume a large amount of time has passed
>  when using ITIMER_REAL. It has to be *much* larger than a VM is likely
>  to be paused for and then successfully resumed. Or the amount of time a
>  clock may be corrected by (for e.g. with NTP).
>
> Hmm, not sure if I understand correctly above, will that
> timer value be out of the range but reasonable?
>
> Or is there any additional situation we should cover?

Sorry that is confusing.

The question is what happens if the clock jumps backwards?

I don't see anything which says it_value.tv_sec can't go out of
range. OTOH I would expect it to compensate for large jumps in time.

If the test randomly fails because it_value.tv_sec > time_sec then what
action will we take?

>
>  
>  
>  >                       tst_res(TFAIL, "Ending counters are out of range");
>
>  While we are here; we should make our lives easier when the test fails
>  by printing any relevant values.
>
> We already do that in the above print, but it's fine to have that here as well.


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH 1/2] setitimer01: add interval timer test

[Andrea Cervesato via ltp]

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

Hi Li,

On 11/15/22 05:00, Li Wang wrote:
>
>
> On Mon, Nov 14, 2022 at 11:52 PM Richard Palethorpe 
> <rpalethorpe@suse.de> wrote:
>
>     Hello,
>
>     Li Wang <liwang@redhat.com> writes:
>
>     > Split checking the return ovalue from testing the signal is
>     > delivered, so that we could use two time value for verifying.
>     >
>     > Also, adding interval timer test by handling the signal at
>     > least 10 times. After that recover the signal behavior to
>     > default and do deliver-signal checking.
>     >
>     > Signed-off-by: Li Wang <liwang@redhat.com>
>     > ---
>     >  .../kernel/syscalls/setitimer/setitimer01.c   | 63
>     ++++++++++++-------
>     >  1 file changed, 39 insertions(+), 24 deletions(-)
>     >
>     > diff --git a/testcases/kernel/syscalls/setitimer/setitimer01.c
>     b/testcases/kernel/syscalls/setitimer/setitimer01.c
>     > index 1f631d457..260590b0e 100644
>     > --- a/testcases/kernel/syscalls/setitimer/setitimer01.c
>     > +++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
>     > @@ -22,8 +22,10 @@
>     >  #include "tst_safe_clocks.h"
>     >
>     >  static struct itimerval *value, *ovalue;
>     > +static volatile unsigned long sigcnt;
>     >  static long time_step;
>     > -static long time_count;
>     > +static long time_sec;
>     > +static long time_usec;
>     >
>     >  static struct tcase {
>     >       int which;
>     > @@ -40,54 +42,66 @@ static int sys_setitimer(int which, void
>     *new_value, void *old_value)
>     >       return tst_syscall(__NR_setitimer, which, new_value,
>     old_value);
>     >  }
>     >
>     > -static void set_setitimer_value(int usec, int o_usec)
>     > +static void sig_routine(int signo LTP_ATTRIBUTE_UNUSED)
>     >  {
>     > -     value->it_value.tv_sec = 0;
>     > -     value->it_value.tv_usec = usec;
>     > -     value->it_interval.tv_sec = 0;
>     > -     value->it_interval.tv_usec = 0;
>     > +     sigcnt++;
>     > +}
>     >
>     > -     ovalue->it_value.tv_sec = o_usec;
>     > -     ovalue->it_value.tv_usec = o_usec;
>     > -     ovalue->it_interval.tv_sec = 0;
>     > -     ovalue->it_interval.tv_usec = 0;
>     > +static void set_setitimer_value(int sec, int usec)
>     > +{
>     > +     value->it_value.tv_sec = sec;
>     > +     value->it_value.tv_usec = usec;
>     > +     value->it_interval.tv_sec = sec;
>     > +     value->it_interval.tv_usec = usec;
>     >  }
>     >
>     >  static void verify_setitimer(unsigned int i)
>     >  {
>     >       pid_t pid;
>     >       int status;
>     > -     long margin;
>     >       struct tcase *tc = &tcases[i];
>     >
>     > +     tst_res(TINFO, "tc->which = %s", tc->des);
>     > +
>     >       pid = SAFE_FORK();
>     >
>     >       if (pid == 0) {
>     > -             tst_res(TINFO, "tc->which = %s", tc->des);
>     > -
>     >               tst_no_corefile(0);
>     >
>     > -             set_setitimer_value(time_count, 0);
>     > +             set_setitimer_value(time_sec, time_usec);
>     >               TST_EXP_PASS(sys_setitimer(tc->which, value, NULL));
>     >
>     > -             set_setitimer_value(5 * time_step, 7 * time_step);
>     > +             set_setitimer_value(2 * time_sec, 2 * time_usec);
>
>     IDK if there was some reason for choosing 5 and 7 in the first place?
>
>
> I don't see any necessary reasons for using prime numbers here.
>
> @Andrea, did I miss something?
>
The reason is that we want to trace input values in the setitimer 
syscalls. By setting them to x5/7 we are able to debug setitimer easily 
if bug appears.
>
>
>     Andrea seemed to be going through the prime numbers.
>
>     >               TST_EXP_PASS(sys_setitimer(tc->which, value, ovalue));
>     >
>     > -             tst_res(TINFO, "tv_sec=%ld, tv_usec=%ld",
>     > -                     ovalue->it_value.tv_sec,
>     > -                     ovalue->it_value.tv_usec);
>     > +  TST_EXP_EQ_LI(ovalue->it_interval.tv_sec, time_sec);
>     > +  TST_EXP_EQ_LI(ovalue->it_interval.tv_usec, time_usec);
>     > +
>     > +             tst_res(TINFO, "ovalue->it_value.tv_sec=%ld,
>     ovalue->it_value.tv_usec=%ld",
>     > +                     ovalue->it_value.tv_sec,
>     ovalue->it_value.tv_usec);
>     >
>     >               /*
>     >                * ITIMER_VIRTUAL and ITIMER_PROF timers always
>     expire a
>     >                * time_step afterward the elapsed time to make
>     sure that
>     >                * at least counters take effect.
>     >                */
>     > -             margin = tc->which == ITIMER_REAL ? 0 : time_step;
>     > +             long margin = (tc->which == ITIMER_REAL) ? 0 :
>     time_step;
>
>     Looks like an unecessary change?
>
>
> Yes, but the 'margin' is only used in children, and I moved
> the declaration here is just to highlight and cause attention
> in code reading.
>
>
>     >
>     > -             if (ovalue->it_value.tv_sec != 0 ||
>     ovalue->it_value.tv_usec > time_count + margin)
>     > +             if (ovalue->it_value.tv_sec > time_sec ||
>     > +  ovalue->it_value.tv_usec > time_usec + margin)
>
>     Looking at the man page, technically speaking the valid range for
>     ovalue->it_value.tv_sec is 0 to value->it_value.tv_sec when
>     ovalue->it_value.tv_usec > 0.
>
>
> Good point!! Seems we have to split the situation into two,
>
> 1. no tv_sec elapse happen, just check
> 'it_value.tv_usec' within [0, time_usec + margin]
>
> 2. tv_sec elapses happen, then check
> 'it_value.tv_sec' within [0, time_sec]
>
> Something maybe like:
>
> --- a/testcases/kernel/syscalls/setitimer/setitimer01.c
> +++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
> @@ -87,9 +87,17 @@ static void verify_setitimer(unsigned int i)
>                  */
>                 long margin = (tc->which == ITIMER_REAL) ? 0 : time_step;
>
> -               if (ovalue->it_value.tv_sec > time_sec ||
> -                               ovalue->it_value.tv_usec > time_usec + 
> margin)
> -                       tst_res(TFAIL, "Ending counters are out of 
> range");
> +               if (ovalue->it_value.tv_sec == time_sec) {
> +                       if (ovalue->it_value.tv_usec < 0 ||
> + ovalue->it_value.tv_usec > time_usec + margin)
> +                               tst_res(TFAIL, 
> "ovalue->it_value.tv_usec is out of range: %ld",
> + ovalue->it_value.tv_usec);
> +               } else {
> +                       if (ovalue->it_value.tv_sec < 0 ||
> + ovalue->it_value.tv_sec > time_sec)
> +                               tst_res(TFAIL, 
> "ovalue->it_value.tv_sec is out of range: %ld",
> + ovalue->it_value.tv_usec);
> +               }
>
>                 SAFE_SIGNAL(tc->signo, sig_routine);
>
>
>     Practically speaking we have to assume a large amount of time has
>     passed
>     when using ITIMER_REAL. It has to be *much* larger than a VM is likely
>     to be paused for and then successfully resumed. Or the amount of
>     time a
>     clock may be corrected by (for e.g. with NTP).
>
>
> Hmm, not sure if I understand correctly above, will that
> timer value be out of the range but reasonable?
>
> Or is there any additional situation we should cover?
>
>
>     >                       tst_res(TFAIL, "Ending counters are out of
>     range");
>
>     While we are here; we should make our lives easier when the test fails
>     by printing any relevant values.
>
>
> We already do that in the above print, but it's fine to have that here 
> as well.
>
> -- 
> Regards,
> Li Wang

Andrea

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

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


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

Re: [LTP] [PATCH 1/2] setitimer01: add interval timer test

[Li Wang]

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

Richard Palethorpe <rpalethorpe@suse.de> wrote:


> >
> >  Practically speaking we have to assume a large amount of time has passed
> >  when using ITIMER_REAL. It has to be *much* larger than a VM is likely
> >  to be paused for and then successfully resumed. Or the amount of time a
> >  clock may be corrected by (for e.g. with NTP).
> >
> > Hmm, not sure if I understand correctly above, will that
> > timer value be out of the range but reasonable?
> >
> > Or is there any additional situation we should cover?
>
> Sorry that is confusing.
>
> The question is what happens if the clock jumps backwards?
>
> I don't see anything which says it_value.tv_sec can't go out of
> range. OTOH I would expect it to compensate for large jumps in time.
>
> If the test randomly fails because it_value.tv_sec > time_sec then what
> action will we take?
>

How about increasing the time_sec on virtual machine?

Seems no perfect way to completely resolve but only reducing
the odds of happening.

Or do you have another better suggestion?

--- a/testcases/kernel/syscalls/setitimer/setitimer01.c
+++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
@@ -142,6 +142,11 @@ static void setup(void)

        time_sec  = 9 + time_step / 1000;
        time_usec = 3 * time_step;
+
+       if (tst_is_virt(VIRT_ANY)) {
+               tst_res(TINFO, "Running in a VM, multiply the time_sec by
10.");
+               time_sec *= 10;
+       }
 }


-- 
Regards,
Li Wang

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

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


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

Re: [LTP] [PATCH 1/2] setitimer01: add interval timer test

[Li Wang]

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

On Tue, Nov 15, 2022 at 6:00 PM Li Wang <liwang@redhat.com> wrote:

> Richard Palethorpe <rpalethorpe@suse.de> wrote:
>
>
>> >
>> >  Practically speaking we have to assume a large amount of time has
>> passed
>> >  when using ITIMER_REAL. It has to be *much* larger than a VM is likely
>> >  to be paused for and then successfully resumed. Or the amount of time a
>> >  clock may be corrected by (for e.g. with NTP).
>> >
>> > Hmm, not sure if I understand correctly above, will that
>> > timer value be out of the range but reasonable?
>> >
>> > Or is there any additional situation we should cover?
>>
>> Sorry that is confusing.
>>
>> The question is what happens if the clock jumps backwards?
>>
>> I don't see anything which says it_value.tv_sec can't go out of
>> range. OTOH I would expect it to compensate for large jumps in time.
>>
>> If the test randomly fails because it_value.tv_sec > time_sec then what
>> action will we take?
>>
>
Or, we do nothing on this, just let the test report TFAIL, because that
is not what this test can control.



>
> How about increasing the time_sec on virtual machine?
>
> Seems no perfect way to completely resolve but only reducing
> the odds of happening.
>
> Or do you have another better suggestion?
>
> --- a/testcases/kernel/syscalls/setitimer/setitimer01.c
> +++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
> @@ -142,6 +142,11 @@ static void setup(void)
>
>         time_sec  = 9 + time_step / 1000;
>         time_usec = 3 * time_step;
> +
> +       if (tst_is_virt(VIRT_ANY)) {
> +               tst_res(TINFO, "Running in a VM, multiply the time_sec by
> 10.");
> +               time_sec *= 10;
> +       }
>  }
>
>
> --
> Regards,
> Li Wang
>


-- 
Regards,
Li Wang

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

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


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

Re: [LTP] [PATCH 1/2] setitimer01: add interval timer test

[Richard Palethorpe]

Hello,

Li Wang <liwang@redhat.com> writes:

> On Tue, Nov 15, 2022 at 6:00 PM Li Wang <liwang@redhat.com> wrote:
>
>  Richard Palethorpe <rpalethorpe@suse.de> wrote:
>   
>  >  
>  >  Practically speaking we have to assume a large amount of time has passed
>  >  when using ITIMER_REAL. It has to be *much* larger than a VM is likely
>  >  to be paused for and then successfully resumed. Or the amount of time a
>  >  clock may be corrected by (for e.g. with NTP).
>  >
>  > Hmm, not sure if I understand correctly above, will that
>  > timer value be out of the range but reasonable?
>  >
>  > Or is there any additional situation we should cover?
>
>  Sorry that is confusing.
>
>  The question is what happens if the clock jumps backwards?
>
>  I don't see anything which says it_value.tv_sec can't go out of
>  range. OTOH I would expect it to compensate for large jumps in time.
>
>  If the test randomly fails because it_value.tv_sec > time_sec then what
>  action will we take?
>
> Or, we do nothing on this, just let the test report TFAIL, because that
> is not what this test can control.
>
>  
>  
>  How about increasing the time_sec on virtual machine?

It could happen on bare metal as well.

>
>  Seems no perfect way to completely resolve but only reducing
>  the odds of happening. 
>
>  Or do you have another better suggestion?

TBH I don't know if it will happen. An acceptable outcome for me is to
print the time at the beginning and end of the test. Then if the test
fails we can see if it was due to a time jump and start investigating
what the kernel is supposed to do in this case.

The alternative is to find out now what the kernel should do. We could
also write a test which deliberately changes the system time during an
interval. Depending how motivated you are.

>
>  --- a/testcases/kernel/syscalls/setitimer/setitimer01.c
>  +++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
>  @@ -142,6 +142,11 @@ static void setup(void)
>   
>          time_sec  = 9 + time_step / 1000;
>          time_usec = 3 * time_step;
>  +
>  +       if (tst_is_virt(VIRT_ANY)) {
>  +               tst_res(TINFO, "Running in a VM, multiply the time_sec by 10.");
>  +               time_sec *= 10;
>  +       }
>   }
>
>   
>  -- 
>  Regards,
>  Li Wang


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH 1/2] setitimer01: add interval timer test

[Li Wang]

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

On Tue, Nov 15, 2022 at 7:12 PM Richard Palethorpe <rpalethorpe@suse.de>
wrote:

> Hello,
>
> Li Wang <liwang@redhat.com> writes:
>
> > On Tue, Nov 15, 2022 at 6:00 PM Li Wang <liwang@redhat.com> wrote:
> >
> >  Richard Palethorpe <rpalethorpe@suse.de> wrote:
> >
> >  >
> >  >  Practically speaking we have to assume a large amount of time has
> passed
> >  >  when using ITIMER_REAL. It has to be *much* larger than a VM is
> likely
> >  >  to be paused for and then successfully resumed. Or the amount of
> time a
> >  >  clock may be corrected by (for e.g. with NTP).
> >  >
> >  > Hmm, not sure if I understand correctly above, will that
> >  > timer value be out of the range but reasonable?
> >  >
> >  > Or is there any additional situation we should cover?
> >
> >  Sorry that is confusing.
> >
> >  The question is what happens if the clock jumps backwards?
>


I did some research on the work theory of setitimer() for modern
kernels, it seems that situation won't happen, because the kernel
uses a relative mode for the timer countdown. IOW, once the system
wall clock getting changed, the timer for the process will update its
timer relatively.

To verify this I adjust the wall-clock with adding whatever 10 sec or
24 hour, it all works well and get expected signal:

--- a/testcases/kernel/syscalls/setitimer/setitimer01.c
+++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
@@ -21,6 +21,7 @@
 #include "lapi/syscalls.h"
 #include "tst_safe_clocks.h"

+static struct timespec now;
 static struct itimerval *value, *ovalue;
 static volatile unsigned long sigcnt;
 static long time_step;
@@ -33,8 +34,8 @@ static struct tcase {
        int signo;
 } tcases[] = {
        {ITIMER_REAL,    "ITIMER_REAL",    SIGALRM},
-       {ITIMER_VIRTUAL, "ITIMER_VIRTUAL", SIGVTALRM},
-       {ITIMER_PROF,    "ITIMER_PROF",    SIGPROF},
+//     {ITIMER_VIRTUAL, "ITIMER_VIRTUAL", SIGVTALRM},
+//     {ITIMER_PROF,    "ITIMER_PROF",    SIGPROF},
 };

 static int sys_setitimer(int which, void *new_value, void *old_value)
@@ -72,6 +73,10 @@ static void verify_setitimer(unsigned int i)
                set_setitimer_value(time_sec, time_usec);
                TST_EXP_PASS(sys_setitimer(tc->which, value, NULL));

+               now.tv_sec += 20; // 86400s == 24h
+               SAFE_CLOCK_SETTIME(CLOCK_REALTIME, &now);
+               tst_res(TINFO, "debug only: add 20 secs to now.tv_sec
%ld\n", now.tv_sec);
+
                set_setitimer_value(5 * time_sec, 7 * time_usec);
                TST_EXP_PASS(sys_setitimer(tc->which, value, ovalue));

@@ -120,6 +125,9 @@ static void verify_setitimer(unsigned int i)
                tst_res(TPASS, "Child received signal: %s",
tst_strsig(tc->signo));
        else
                tst_res(TFAIL, "Child: %s", tst_strstatus(status));
+
+       SAFE_CLOCK_GETTIME(CLOCK_REALTIME, &now);
+       tst_res(TINFO, "debug only: now.tv_sec is %ld\n", now.tv_sec);
 }

 static void setup(void)
@@ -142,6 +150,9 @@ static void setup(void)

        time_sec  = 9 + time_step / 1000;
        time_usec = 3 * time_step;
+
+       SAFE_CLOCK_GETTIME(CLOCK_REALTIME, &now);
+       tst_res(TINFO, "debug only: now.tv_sec is %ld\n", now.tv_sec);
 }

 static struct tst_test test = {


>  Or do you have another better suggestion?
>
> TBH I don't know if it will happen. An acceptable outcome for me is to
> print the time at the beginning and end of the test. Then if the test
> fails we can see if it was due to a time jump and start investigating
> what the kernel is supposed to do in this case.
>

But I think this print is still necessary, in case that some older kernels
do not use the relative mode for timer. I'll add this and send patch v2.


> The alternative is to find out now what the kernel should do. We could
> also write a test which deliberately changes the system time during an
> interval. Depending how motivated you are.
>

Thanks!


-- 
Regards,
Li Wang

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

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


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

Re: [LTP] [PATCH 1/2] setitimer01: add interval timer test

[Li Wang]

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

On Mon, Nov 7, 2022 at 8:13 PM Richard Palethorpe <rpalethorpe@suse.de>
wrote:

> Hello Li,
>
> Li Wang <liwang@redhat.com> writes:
>
> > Plz ignore this patch, it needs code rebase after Anderea's work:
> >
> >     b606a7c7d setitimer01: Fix checking of setitimer() parameters
> >
> > --
> > Regards,
> > Li Wang
>
> I'll set to "changes requested" in patchwork. Plase can you update
> Patchwork in the future?
>

Yes, I should do that but I somehow forget. sorry~

-- 
Regards,
Li Wang

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

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


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

Re: [LTP] [PATCH 1/2] setitimer01: add interval timer test

[Richard Palethorpe]

Hello Li,

Li Wang <liwang@redhat.com> writes:

> Plz ignore this patch, it needs code rebase after Anderea's work:
>
>     b606a7c7d setitimer01: Fix checking of setitimer() parameters
>
> -- 
> Regards,
> Li Wang

I'll set to "changes requested" in patchwork. Plase can you update Patchwork in the future?

-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH 1/2] setitimer01: add interval timer test

[Li Wang]

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

Plz ignore this patch, it needs code rebase after Anderea's work:

    b606a7c7d setitimer01: Fix checking of setitimer() parameters

-- 
Regards,
Li Wang

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

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


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

[LTP] [PATCH 1/2] setitimer01: add interval timer test

[Li Wang]

First, split checking the return ovalue from testing the
signal is delivered. The benefit is that we could use a
long timeout value for verifying.

Second, add an interval timer test by handling the signal
at least 10 times. After that recover the signal behavior
to default and do deliver-signal checking.

Signed-off-by: Li Wang <liwang@redhat.com>
---
 .../kernel/syscalls/setitimer/setitimer01.c   | 56 ++++++++++++-------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/testcases/kernel/syscalls/setitimer/setitimer01.c b/testcases/kernel/syscalls/setitimer/setitimer01.c
index f04cb5a69..a59a9af1b 100644
--- a/testcases/kernel/syscalls/setitimer/setitimer01.c
+++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
@@ -18,9 +18,11 @@
 #include "tst_test.h"
 #include "lapi/syscalls.h"
 
-#define USEC1	10000
-#define USEC2	20000
+#define SEC1	300
+#define SEC2	600
+#define USEC	100
 
+static volatile unsigned long sigcnt;
 static struct itimerval *value, *ovalue;
 
 static struct tcase {
@@ -38,17 +40,17 @@ static int sys_setitimer(int which, void *new_value, void *old_value)
 	return tst_syscall(__NR_setitimer, which, new_value, old_value);
 }
 
-static void set_setitimer_value(int usec, int o_usec)
+static void sig_routine(int signo LTP_ATTRIBUTE_UNUSED)
 {
-	value->it_value.tv_sec = 0;
-	value->it_value.tv_usec = usec;
-	value->it_interval.tv_sec = 0;
-	value->it_interval.tv_usec = 0;
+	sigcnt++;
+}
 
-	ovalue->it_value.tv_sec = o_usec;
-	ovalue->it_value.tv_usec = o_usec;
-	ovalue->it_interval.tv_sec = 0;
-	ovalue->it_interval.tv_usec = 0;
+static void set_setitimer_value(int sec, int usec)
+{
+	value->it_value.tv_sec = sec;
+	value->it_value.tv_usec = usec;
+	value->it_interval.tv_sec = sec;
+	value->it_interval.tv_usec = usec;
 }
 
 static void verify_setitimer(unsigned int i)
@@ -57,23 +59,37 @@ static void verify_setitimer(unsigned int i)
 	int status;
 	struct tcase *tc = &tcases[i];
 
-	pid = SAFE_FORK();
+	tst_res(TINFO, "tc->which = %s", tc->des);
 
-	if (pid == 0) {
-		tst_res(TINFO, "tc->which = %s", tc->des);
+	set_setitimer_value(SEC1, USEC);
+	TST_EXP_PASS_SILENT(sys_setitimer(tc->which, value, NULL));
+
+	set_setitimer_value(SEC2, USEC);
+	TST_EXP_PASS_SILENT(sys_setitimer(tc->which, value, ovalue));
 
+	TST_EXP_EQ_LI(ovalue->it_interval.tv_sec, SEC1);
+	TST_EXP_EQ_LI(ovalue->it_interval.tv_usec, USEC);
+
+	if (ovalue->it_value.tv_sec + ovalue->it_value.tv_usec/1000000 <= SEC1)
+		tst_res(TPASS, "old timer value is within the expected range");
+	else
+		tst_res(TFAIL, "old timer value is not within the expected range");
+
+	pid = SAFE_FORK();
+	if (pid == 0) {
 		tst_no_corefile(0);
 
-		set_setitimer_value(USEC1, 0);
+		SAFE_SIGNAL(tc->signo, sig_routine);
+
+		set_setitimer_value(0, USEC);
 		TST_EXP_PASS(sys_setitimer(tc->which, value, NULL));
 
-		set_setitimer_value(USEC2, USEC2);
-		TST_EXP_PASS(sys_setitimer(tc->which, value, ovalue));
+		while (sigcnt <= 10UL)
+			;
 
-		if (ovalue->it_value.tv_sec != 0 || ovalue->it_value.tv_usec >= USEC2)
-			tst_brk(TFAIL, "old timer value is not within the expected range");
+		SAFE_SIGNAL(tc->signo, SIG_DFL);
 
-		for (;;)
+		while (1)
 			;
 	}
 
-- 
2.35.3


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