[LTP] [PATCH v2] Correctly check setitimer params in setitimer01

[Li Wang]

Hi Andrea,

On Mon, Nov 7, 2022 at 7:03 PM Andrea Cervesato via ltp <ltp@lists.linux.it>
wrote:

> We use CLOCK_MONOTONIC_COARSE as our time resolution for checking
> setitimer counter boundaries.
>
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
> Switching to CLOCK_MONOTONIC_COARSE for setitimer time resolution.
>
>  .../kernel/syscalls/setitimer/setitimer01.c   | 33 +++++++++++--------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/setitimer/setitimer01.c
> b/testcases/kernel/syscalls/setitimer/setitimer01.c
> index eb62f02c6..5c880c6ef 100644
> --- a/testcases/kernel/syscalls/setitimer/setitimer01.c
> +++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
> @@ -8,7 +8,7 @@
>  /*\
>   * [Description]
>   *
> - * Spawn a child and verify that setitimer() syscall passes, and it ends
> up
> + * Spawn a child, verify that setitimer() syscall passes and it ends up
>   * counting inside expected boundaries. Then verify from the parent that
> our
>   * syscall sent the correct signal to the child.
>   */
> @@ -22,7 +22,8 @@
>  #include "tst_safe_clocks.h"
>
>  static struct itimerval *value, *ovalue;
> -static unsigned long time_step;
> +static long time_step;
> +static long time_count;
>
>  static struct tcase {
>         int which;
> @@ -56,7 +57,6 @@ static void verify_setitimer(unsigned int i)
>  {
>         pid_t pid;
>         int status;
> -       int usec = 3 * time_step;
>         struct tcase *tc = &tcases[i];
>
>         pid = SAFE_FORK();
> @@ -66,7 +66,7 @@ static void verify_setitimer(unsigned int i)
>
>                 tst_no_corefile(0);
>
> -               set_setitimer_value(usec, 0);
> +               set_setitimer_value(time_count, 0);
>                 TST_EXP_PASS(sys_setitimer(tc->which, value, NULL));
>
>                 set_setitimer_value(5 * time_step, 7 * time_step);
>

Maybe we can use 'time_count' instead of 'time_step' as well.



> @@ -76,7 +76,7 @@ static void verify_setitimer(unsigned int i)
>                         ovalue->it_value.tv_sec,
>                         ovalue->it_value.tv_usec);
>
> -               if (ovalue->it_value.tv_sec != 0 ||
> ovalue->it_value.tv_usec > usec)
> +               if (ovalue->it_value.tv_sec != 0 ||
> ovalue->it_value.tv_usec > time_count + time_step)
>

This is not correct for 'ITIMER_REAL', kernel does _not_
add that one jiffy when using high resolution. I'd suggest
taking reference to Martin's code in the last email thread.

And, I also think we'd better insert code comments here to
briefly declare why need to add the time_step for the result
(of ITIMER_VIRTUAL/ITIMER_PROF) comparison, otherwise,
people who are not familiar with this test will be confused.



>                         tst_res(TFAIL, "Ending counters are out of range");
>
>                 for (;;)
> @@ -93,24 +93,29 @@ static void verify_setitimer(unsigned int i)
>
>  static void setup(void)
>  {
> -       struct timespec res;
> +       struct timespec time_res;
>
> -       SAFE_CLOCK_GETRES(CLOCK_MONOTONIC, &res);
> +       SAFE_CLOCK_GETRES(CLOCK_MONOTONIC_COARSE, &time_res);
>

And here require code comments on why we choose to
use CLOCK_MONOTONIC_COARSE.



>
> -       time_step = res.tv_nsec / 1000;
> -       if (time_step < 10000)
> -               time_step = 10000;
> +       time_step = time_res.tv_nsec / 1000;
> +       if (time_step <= 0)
> +               time_step = 1000;
>
> -       tst_res(TINFO, "clock resolution: %luns, time step: %luus",
> -               res.tv_nsec,
> -               time_step);
> +       time_count = 3 * time_step;
> +
> +       tst_res(TINFO, "clock resolution: %luns, "
> +               "time step: %luus, "
> +               "time count: %luus",
> +               time_res.tv_nsec,
> +               time_step,
> +               time_count);
>  }
>
>  static struct tst_test test = {
>         .tcnt = ARRAY_SIZE(tcases),
>         .forks_child = 1,
> -       .test = verify_setitimer,
>         .setup = setup,
> +       .test = verify_setitimer,
>         .bufs = (struct tst_buffers[]) {
>                 {&value,  .size = sizeof(struct itimerval)},
>                 {&ovalue, .size = sizeof(struct itimerval)},
> --
> 2.35.3
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20221110/57184e4d/attachment-0001.htm>