[LTP] [PATCH v3 22/29] fuzzy_sync: Convert to runtime

[Li Wang]

On Thu, May 19, 2022 at 5:03 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > Okay, my perspective is shortsighted as well.
> >
> > This solution is correct in the direction but overlooks the significant
> > global value 'tst_start_time'. If that value reflush within tcnt loop we
> > don't need to reset max_runtime again, actually the real work in my
> > previous patch is to invoke heartbeat() which touches tst_start_time.
> >
> > So I would suggest using heartbeat() instead of only sending SIGUSR1
> > to lib_pid. Or, do simply revision like:
>
> Ah, right, I guess that we should just call heartbeat() before each
> iteration of the test then.
>
> So basically:
>
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index dad8aad92..f3090217b 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -1317,6 +1317,24 @@ static void do_cleanup(void)
>         cleanup_ipc();
>  }
>
> +static void heartbeat(void)
> +{
> +       if (tst_clock_gettime(CLOCK_MONOTONIC, &tst_start_time))
> +               tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
> +
> +       if (getppid() == 1) {
> +               tst_res(TFAIL, "Main test process might have exit!");
> +               /*
> +                * We need kill the task group immediately since the
> +                * main process has exit.
> +                */
> +               kill(0, SIGKILL);
> +               exit(TBROK);
> +       }
> +
> +       kill(getppid(), SIGUSR1);
> +}
> +
>  static void run_tests(void)
>  {
>         unsigned int i;
> @@ -1324,6 +1342,7 @@ static void run_tests(void)
>
>         if (!tst_test->test) {
>                 saved_results = *results;
> +               heartbeat();
>                 tst_test->test_all();
>
>                 if (getpid() != main_pid) {
> @@ -1339,6 +1358,7 @@ static void run_tests(void)
>
>         for (i = 0; i < tst_test->tcnt; i++) {
>                 saved_results = *results;
> +               heartbeat();
>                 tst_test->test(i);
>
>                 if (getpid() != main_pid) {
> @@ -1349,6 +1369,8 @@ static void run_tests(void)
>
>                 if (results_equal(&saved_results, results))
>                         tst_brk(TBROK, "Test %i haven't reported
> results!", i);
> +
> +               kill(getppid(), SIGUSR1);
>

As we already invoke heartbeat() at the beginning of each tcnt loop,
why are we still sending SIGUSR1 here?

Otherwise this version looks good to me.



>         }
>  }
>
> @@ -1379,24 +1401,6 @@ static void add_paths(void)
>         free(new_path);
>  }
>
> -static void heartbeat(void)
> -{
> -       if (tst_clock_gettime(CLOCK_MONOTONIC, &tst_start_time))
> -               tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
> -
> -       if (getppid() == 1) {
> -               tst_res(TFAIL, "Main test process might have exit!");
> -               /*
> -                * We need kill the task group immediately since the
> -                * main process has exit.
> -                */
> -               kill(0, SIGKILL);
> -               exit(TBROK);
> -       }
> -
> -       kill(getppid(), SIGUSR1);
> -}
> -
>  static void testrun(void)
>  {
>         unsigned int i = 0;
> @@ -1425,7 +1429,6 @@ static void testrun(void)
>                         break;
>
>                 run_tests();
> -               heartbeat();
>         }
>
>         do_test_cleanup();
>
>
> I guess that this should go in a separate patch on the top of the
> "Introduce concept of max runtime".
>

Agree.


-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20220519/24ff4987/attachment.htm>