[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+GJov6YQ2YcfqudffKzAKmcwbgCvuXpd8HzKnwSuytF-ozvww@mail.gmail.com>
Date: Tue, 24 Oct 2023 15:41:33 -0400
From: Rae Moar <rmoar@...gle.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Brendan Higgins <brendan.higgins@...ux.dev>,
David Gow <davidgow@...gle.com>,
Jani Nikula <jani.nikula@...ux.intel.com>,
linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] kunit: Warn if tests are slow
On Wed, Sep 20, 2023 at 4:49 AM 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>
>
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@...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 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 | 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
>
Powered by blists - more mailing lists