Re: [PATCH v2] kunit: Warn if tests are slow

[Rae Moar]

On Wed, Sep 20, 2023 at 4:49 AM Maxime Ripard <mripard@xxxxxxxxxx> wrote:
>
> Kunit recently gained support to setup attributes, the first one being
> the speed of a given test, then allowing to filter out slow tests.
>
> A slow test is defined in the documentation as taking more than one
> second. There's an another speed attribute called "super slow" but whose
> definition is less clear.
>
> Add support to the test runner to check the test execution time, and
> report tests that should be marked as slow but aren't.
>
> Signed-off-by: Maxime Ripard <mripard@xxxxxxxxxx>
>

Hello!

Thanks for following up! Sorry for the delay in this response.

This looks great to me. I do have one comment below regarding the
KUNIT_SPEED_SLOW_THRESHOLD_S macro but other than that I would be
happy with this patch.

This patch does bring up the question of how to handle KUnit warnings
as mentioned before. But I am happy to approach that in a future
patch.

And I do still have concerns with this being annoying for those on
slower architectures but again that would depend on how we deal with
KUnit warnings.

Thanks!
-Rae

> ---
>
> To: Brendan Higgins <brendan.higgins@xxxxxxxxx>
> To: David Gow <davidgow@xxxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Rae Moar <rmoar@xxxxxxxxxx>
> Cc: linux-kselftest@xxxxxxxxxxxxxxx
> Cc: kunit-dev@xxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
>
> Changes from v1:
> - Split the patch out of the series
> - Change to trigger the warning only if the runtime is twice the
>   threshold (Jani, Rae)
> - Split the speed check into a separate function (Rae)
> - Link: https://lore.kernel.org/all/20230911-kms-slow-tests-v1-0-d3800a69a1a1@xxxxxxxxxx/
> ---
>  lib/kunit/test.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 49698a168437..a1d5dd2bf87d 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -372,6 +372,25 @@ void kunit_init_test(struct kunit *test, const char *name, char *log)
>  }
>  EXPORT_SYMBOL_GPL(kunit_init_test);
>
> +#define KUNIT_SPEED_SLOW_THRESHOLD_S   1
> +
> +static void kunit_run_case_check_speed(struct kunit *test,
> +                                      struct kunit_case *test_case,
> +                                      struct timespec64 duration)
> +{
> +       enum kunit_speed speed = test_case->attr.speed;
> +
> +       if (duration.tv_sec < (2 * KUNIT_SPEED_SLOW_THRESHOLD_S))

I think I would prefer that KUNIT_SPEED_SLOW_THRESHOLD_S is instead
set to 2 rather than using 2 as the multiplier. I realize the actual
threshold for the attributes is 1 sec but for the practical use of
this warning it is 2 sec.

Also I would still be open to this being 1 sec depending on others
opinions. David, what are your thoughts on this?

> +               return;
> +
> +       if (speed == KUNIT_SPEED_VERY_SLOW || speed == KUNIT_SPEED_SLOW)
> +               return;
> +
> +       kunit_warn(test,
> +                  "Test should be marked slow (runtime: %lld.%09lds)",
> +                  duration.tv_sec, duration.tv_nsec);
> +}
> +
>  /*
>   * Initializes and runs test case. Does not clean up or do post validations.
>   */
> @@ -379,6 +398,8 @@ static void kunit_run_case_internal(struct kunit *test,
>                                     struct kunit_suite *suite,
>                                     struct kunit_case *test_case)
>  {
> +       struct timespec64 start, end;
> +
>         if (suite->init) {
>                 int ret;
>
> @@ -390,7 +411,13 @@ static void kunit_run_case_internal(struct kunit *test,
>                 }
>         }
>
> +       ktime_get_ts64(&start);
> +
>         test_case->run_case(test);
> +
> +       ktime_get_ts64(&end);
> +
> +       kunit_run_case_check_speed(test, test_case, timespec64_sub(end, start));
>  }
>
>  static void kunit_case_internal_cleanup(struct kunit *test)
> --
> 2.41.0
>