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

[David Gow]

On Thu, 26 Oct 2023 at 16:59, 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>
>
> ---

Looks good to me!

Reviewed-by: David Gow <davidgow@xxxxxxxxxx>

Thanks,
-- David

>
> 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 v2:
> - Add defines and comments to make the warning reporting threshold more
>   obvious
> - Switch the duration comparisons to timespec64_compare to be more
>   accurate
> - Link: https://lore.kernel.org/all/20230920084903.1522728-1-mripard@xxxxxxxxxx/
>
> 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 | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 49698a168437..4b710c92340a 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -372,6 +372,36 @@ void kunit_init_test(struct kunit *test, const char *name, char *log)
>  }
>  EXPORT_SYMBOL_GPL(kunit_init_test);
>
> +/* Only warn when a test takes more than twice the threshold */
> +#define KUNIT_SPEED_WARNING_MULTIPLIER 2
> +
> +/* Slow tests are defined as taking more than 1s */
> +#define KUNIT_SPEED_SLOW_THRESHOLD_S   1
> +
> +#define KUNIT_SPEED_SLOW_WARNING_THRESHOLD_S   \
> +       (KUNIT_SPEED_WARNING_MULTIPLIER * KUNIT_SPEED_SLOW_THRESHOLD_S)
> +
> +#define s_to_timespec64(s) ns_to_timespec64((s) * NSEC_PER_SEC)
> +
> +static void kunit_run_case_check_speed(struct kunit *test,
> +                                      struct kunit_case *test_case,
> +                                      struct timespec64 duration)
> +{
> +       struct timespec64 slow_thr =
> +               s_to_timespec64(KUNIT_SPEED_SLOW_WARNING_THRESHOLD_S);
> +       enum kunit_speed speed = test_case->attr.speed;
> +
> +       if (timespec64_compare(&duration, &slow_thr) < 0)
> +               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 +409,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 +422,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
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature