| Message ID | 20221112040107.3953862-1-liwang@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] setitimer01: add interval timer test | expand |
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.
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.
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.
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
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; + } }
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 >
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
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!
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 = {
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(-)