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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 27 Oct 2023 11:56:59 +0800
From:   David Gow <davidgow@...gle.com>
To:     Maxime Ripard <mripard@...nel.org>
Cc:     Brendan Higgins <brendan.higgins@...ux.dev>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Rae Moar <rmoar@...gle.com>, linux-kselftest@...r.kernel.org,
        kunit-dev@...glegroups.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] kunit: Warn if tests are slow

On Thu, 26 Oct 2023 at 16:59, Maxime Ripard <mripard@...nel.org> 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@...nel.org>
>
> ---

Looks good to me!

Reviewed-by: David Gow <davidgow@...gle.com>

Thanks,
-- David

>
> To: Brendan Higgins <brendan.higgins@...ux.dev>
> To: David Gow <davidgow@...gle.com>
> Cc: Jani Nikula <jani.nikula@...ux.intel.com>
> Cc: Rae Moar <rmoar@...gle.com>
> Cc: linux-kselftest@...r.kernel.org
> Cc: kunit-dev@...glegroups.com
> Cc: linux-kernel@...r.kernel.org
>
> 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@kernel.org/
>
> 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@kernel.org/
> ---
>  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
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)