[PATCH 0/2] drm/tests: Flag slow kunit tests as such

閲覧: 5 回
最初の未読メッセージにスキップ

Maxime Ripard

未読、
2023/09/11 5:51:142023/09/11
To: Brendan Higgins、David Gow、David Airlie、Daniel Vetter、Maarten Lankhorst、Thomas Zimmermann、Maíra Canal、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org、dri-...@lists.freedesktop.org、Maxime Ripard
Hi,

Here's a series that sets the speed attribute to slow on DRM tests that
are taking a while to execute.

With those patches, an initial run of the drm tests on arm64 were taking
59s to execute with:

$ ./tools/testing/kunit/kunit.py run \
--kunitconfig=drivers/gpu/drm/tests \
--arch arm64 \
--cross_compile aarch64-linux-gnu-
...
[11:50:07] Testing complete. Ran 340 tests: passed: 340
[11:50:07] Elapsed time: 62.261s total, 0.001s configuring, 2.703s building, 59.532s running

and are now taking 1.7s when filtering out the slow tests:

$ ./tools/testing/kunit/kunit.py run \
--kunitconfig=drivers/gpu/drm/tests \
--arch arm64 \
--cross_compile aarch64-linux-gnu- \
--filter "speed>slow"
...
[11:47:52] Testing complete. Ran 332 tests: passed: 332
[11:47:52] Elapsed time: 6.449s total, 0.001s configuring, 4.728s building, 1.678s running

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard <mri...@kernel.org>
---
Maxime Ripard (2):
kunit: Warn if tests are slow
drm/tests: Flag slow tests as such

drivers/gpu/drm/tests/drm_buddy_test.c | 2 +-
drivers/gpu/drm/tests/drm_mm_test.c | 14 +++++++-------
lib/kunit/test.c | 16 ++++++++++++++++
3 files changed, 24 insertions(+), 8 deletions(-)
---
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
change-id: 20230911-kms-slow-tests-0261bee9a54b

Best regards,
--
Maxime Ripard <mri...@kernel.org>

Maxime Ripard

未読、
2023/09/11 5:51:172023/09/11
To: Brendan Higgins、David Gow、David Airlie、Daniel Vetter、Maarten Lankhorst、Thomas Zimmermann、Maíra Canal、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org、dri-...@lists.freedesktop.org、Maxime Ripard
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 <mri...@kernel.org>
---
lib/kunit/test.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 49698a168437..a3b924501f3d 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -379,6 +379,9 @@ static void kunit_run_case_internal(struct kunit *test,
struct kunit_suite *suite,
struct kunit_case *test_case)
{
+ struct timespec64 start, end;
+ struct timespec64 duration;
+
if (suite->init) {
int ret;

@@ -390,7 +393,20 @@ static void kunit_run_case_internal(struct kunit *test,
}
}

+ ktime_get_ts64(&start);
+
test_case->run_case(test);
+
+ ktime_get_ts64(&end);
+
+ duration = timespec64_sub(end, start);
+
+ if (duration.tv_sec >= 1 &&
+ (test_case->attr.speed == KUNIT_SPEED_UNSET ||
+ test_case->attr.speed >= KUNIT_SPEED_NORMAL))
+ kunit_warn(test,
+ "Test should be marked slow (runtime: %lld.%09lds)",
+ duration.tv_sec, duration.tv_nsec);
}

static void kunit_case_internal_cleanup(struct kunit *test)

--
2.41.0

Maxime Ripard

未読、
2023/09/11 5:51:232023/09/11
To: Brendan Higgins、David Gow、David Airlie、Daniel Vetter、Maarten Lankhorst、Thomas Zimmermann、Maíra Canal、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org、dri-...@lists.freedesktop.org、Maxime Ripard
Kunit recently gained a speed attribute that allows to filter out slow
tests. A slow test is defined in the documentation as a test taking more
than a second to execute.

Let's flag the few tests that are doing so on my machine when running:

./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
--cross_compile aarch64-linux-gnu- --arch arm64

Suggested-by: David Gow <davi...@google.com>
Signed-off-by: Maxime Ripard <mri...@kernel.org>
---
drivers/gpu/drm/tests/drm_buddy_test.c | 2 +-
drivers/gpu/drm/tests/drm_mm_test.c | 14 +++++++-------
2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
index 09ee6f6af896..6f79cde2df55 100644
--- a/drivers/gpu/drm/tests/drm_buddy_test.c
+++ b/drivers/gpu/drm/tests/drm_buddy_test.c
@@ -742,7 +742,7 @@ static struct kunit_case drm_buddy_tests[] = {
KUNIT_CASE(drm_test_buddy_alloc_range),
KUNIT_CASE(drm_test_buddy_alloc_optimistic),
KUNIT_CASE(drm_test_buddy_alloc_pessimistic),
- KUNIT_CASE(drm_test_buddy_alloc_smoke),
+ KUNIT_CASE_SLOW(drm_test_buddy_alloc_smoke),
KUNIT_CASE(drm_test_buddy_alloc_pathological),
{}
};
diff --git a/drivers/gpu/drm/tests/drm_mm_test.c b/drivers/gpu/drm/tests/drm_mm_test.c
index 186b28dc7038..c1e662c2a76c 100644
--- a/drivers/gpu/drm/tests/drm_mm_test.c
+++ b/drivers/gpu/drm/tests/drm_mm_test.c
@@ -2228,23 +2228,23 @@ module_param(max_prime, uint, 0400);
static struct kunit_case drm_mm_tests[] = {
KUNIT_CASE(drm_test_mm_init),
KUNIT_CASE(drm_test_mm_debug),
- KUNIT_CASE(drm_test_mm_reserve),
- KUNIT_CASE(drm_test_mm_insert),
- KUNIT_CASE(drm_test_mm_replace),
- KUNIT_CASE(drm_test_mm_insert_range),
+ KUNIT_CASE_SLOW(drm_test_mm_reserve),
+ KUNIT_CASE_SLOW(drm_test_mm_insert),
+ KUNIT_CASE_SLOW(drm_test_mm_replace),
+ KUNIT_CASE_SLOW(drm_test_mm_insert_range),
KUNIT_CASE(drm_test_mm_frag),
KUNIT_CASE(drm_test_mm_align),
KUNIT_CASE(drm_test_mm_align32),
KUNIT_CASE(drm_test_mm_align64),
- KUNIT_CASE(drm_test_mm_evict),
+ KUNIT_CASE_SLOW(drm_test_mm_evict),
KUNIT_CASE(drm_test_mm_evict_range),
KUNIT_CASE(drm_test_mm_topdown),
KUNIT_CASE(drm_test_mm_bottomup),
KUNIT_CASE(drm_test_mm_lowest),
KUNIT_CASE(drm_test_mm_highest),
KUNIT_CASE(drm_test_mm_color),
- KUNIT_CASE(drm_test_mm_color_evict),
- KUNIT_CASE(drm_test_mm_color_evict_range),
+ KUNIT_CASE_SLOW(drm_test_mm_color_evict),
+ KUNIT_CASE_SLOW(drm_test_mm_color_evict_range),
{}
};


--
2.41.0

Jani Nikula

未読、
2023/09/11 6:07:462023/09/11
To: Maxime Ripard、Brendan Higgins、David Gow、David Airlie、Daniel Vetter、Maarten Lankhorst、Thomas Zimmermann、linux-...@vger.kernel.org、Maxime Ripard、Maíra Canal、dri-...@lists.freedesktop.org、linux-k...@vger.kernel.org、kuni...@googlegroups.com
Two thoughts:

Should there be some tolerance here? Otherwise we're flagging this on
the slowest machines, and we'll be defining tests slow based on
that. Like, warn if it takes more than 2 seconds.

What if someone makes a test faster, but forgets to update the
attribute? Should we also flag slow tests that are in fact fast?


BR,
Jani.


> }
>
> static void kunit_case_internal_cleanup(struct kunit *test)

--
Jani Nikula, Intel Open Source Graphics Center

Maxime Ripard

未読、
2023/09/11 7:25:302023/09/11
To: Jani Nikula、Brendan Higgins、David Gow、David Airlie、Daniel Vetter、Maarten Lankhorst、Thomas Zimmermann、linux-...@vger.kernel.org、Maíra Canal、dri-...@lists.freedesktop.org、linux-k...@vger.kernel.org、kuni...@googlegroups.com
Hi Jani,
I'm not sure what the expectation from David and Brendan are here. I'll
follow what they suggest, but with a couple of hundreds tests like we
have in DRM at the moment, the difference in run time can be up to 5
minutes :/

> What if someone makes a test faster, but forgets to update the
> attribute? Should we also flag slow tests that are in fact fast?

I'm not sure we can do that actually, because it certainly depends on
the hardware running the tests. So I would definitely expect most of the
slow tests to be running faster on some hardware.

Like, running kunit natively on my workstation clears all the DRM tests
in 6s, while it takes about 60s using qemu to test it on arm64, so they
would be considered slow on arm64 but not by default.

Maxime
signature.asc

Daniel Vetter

未読、
2023/09/12 3:36:182023/09/12
To: Maxime Ripard、Brendan Higgins、David Gow、David Airlie、Daniel Vetter、Maarten Lankhorst、Thomas Zimmermann、Maíra Canal、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org、dri-...@lists.freedesktop.org
On Mon, Sep 11, 2023 at 11:51:06AM +0200, Maxime Ripard wrote:
> Kunit recently gained a speed attribute that allows to filter out slow
> tests. A slow test is defined in the documentation as a test taking more
> than a second to execute.
>
> Let's flag the few tests that are doing so on my machine when running:
>
> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
> --cross_compile aarch64-linux-gnu- --arch arm64
>
> Suggested-by: David Gow <davi...@google.com>
> Signed-off-by: Maxime Ripard <mri...@kernel.org>

Ugh ... not a fan.

igt has a really bad habit of making disastrously combinatorial tests with
impossible runtimes, and then just filtering these out so it's still fast.

Maybe some stress tests for overall system make sense like this, but
absolutely not for unit tests. And I did spot check some of these, they're
just combinatorial explosions with large repetition counts and some fun
stuff like going through prime numbers because surely that's a good idea.

Imo delete them all, and if that causes a real gap in coverage, ask the
authors to write some actual good unit tests for these corner cases.

Cheers, Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Maxime Ripard

未読、
2023/09/14 9:24:492023/09/14
To: Daniel Vetter、Brendan Higgins、David Gow、David Airlie、Maarten Lankhorst、Thomas Zimmermann、Maíra Canal、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org、dri-...@lists.freedesktop.org
Hi Sima,

(For some reason, it looks like your mailer sets up the headers to reply
to every recipient but you)

On Tue, Sep 12, 2023 at 09:36:12AM +0200, Daniel Vetter wrote:
> On Mon, Sep 11, 2023 at 11:51:06AM +0200, Maxime Ripard wrote:
> > Kunit recently gained a speed attribute that allows to filter out slow
> > tests. A slow test is defined in the documentation as a test taking more
> > than a second to execute.
> >
> > Let's flag the few tests that are doing so on my machine when running:
> >
> > ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
> > --cross_compile aarch64-linux-gnu- --arch arm64
> >
> > Suggested-by: David Gow <davi...@google.com>
> > Signed-off-by: Maxime Ripard <mri...@kernel.org>
>
> Ugh ... not a fan.
>
> igt has a really bad habit of making disastrously combinatorial tests with
> impossible runtimes, and then just filtering these out so it's still fast.
>
> Maybe some stress tests for overall system make sense like this, but
> absolutely not for unit tests.

I agree, I didn't want to reduce testing though.

> And I did spot check some of these, they're just combinatorial
> explosions with large repetition counts and some fun stuff like going
> through prime numbers because surely that's a good idea.
>
> Imo delete them all, and if that causes a real gap in coverage, ask
> the authors to write some actual good unit tests for these corner
> cases.

Ack, I will send a patch doing so.

Thanks!
Maxime
signature.asc

Maxime Ripard

未読、
2023/09/20 3:06:572023/09/20
To: Rae Moar、Brendan Higgins、David Gow、David Airlie、Daniel Vetter、Maarten Lankhorst、Thomas Zimmermann、Maíra Canal、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org、dri-...@lists.freedesktop.org
Hi,

On Tue, Sep 19, 2023 at 03:48:55PM -0400, Rae Moar wrote:
> On Mon, Sep 11, 2023 at 5:51 AM Maxime Ripard <mri...@kernel.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 <mri...@kernel.org>
>
> I like this idea especially if it was helpful in identifying slow
> tests already! I have a few thoughts on this. I share Jani's concern
> for warning all tests on slow machines. I can think of a few options.
>
> First, we could increase the threshold to about 2s even though that
> would eliminate warnings on potentially slow tests. However, this
> would point out the slowest tests.

I don't have a strong opinion there, so whatever works for you :)

> Second, we could change this to warn users only when they choose by
> making this a configurable option or making this a script to output a
> list of all unmarked slow tests.

I'm not really sure. Adding an option would hide it away from users and
only a fraction of the users (including devs working on tests) would see
that their test should actually be marked as slow.

That will prevent the wider use of it imo, and instead of catching it
early (when we're working on it), will lead to more patches.

Plus, a runtime of more than a second, no matter the platform, is
usually a good indication that what your test is doing probably
shouldn't be done that way.

> Third, we could leave this as is. As the KUnit warnings do not show up
> in the kunit.py output and do not cause the test to fail in any way
> they are relatively harmless if they are unwanted by the user.

I was looking at it the other day, and I think we can modify the TAP
output to expose the warning through the kunit.py command to the user.

It looks like it allows to provide any keyword after the comment mark
and allows to extend it, so we could have something like

ok $TEST # KUNIT_WARN $MESSAGE

and then parse that in kunit.py, pretty much like we handle SKIP. But
that's a separate discussion really.

But yeah, whether or not this is reported to the user, it must not fail
the test.

> Not quite sure which I prefer? The second option might be the cleanest
> for the user and the time threshold could even be customizable. Let me
> know what you think.

I'm in favour of the second one as well.
> I would consider moving this if statement into a separate function.

Ack.

I'll send a v2 with your suggestions

Thanks!
Maxime
signature.asc

Maxime Ripard

未読、
2023/09/20 4:49:102023/09/20
To: Brendan Higgins、David Gow、Maxime Ripard、Jani Nikula、Rae Moar、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org
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 <mri...@kernel.org>

---

To: Brendan Higgins <brendan...@linux.dev>
To: David Gow <davi...@google.com>
Cc: Jani Nikula <jani....@linux.intel.com>
Cc: Rae Moar <rm...@google.com>
Cc: linux-k...@vger.kernel.org
Cc: kuni...@googlegroups.com
Cc: linux-...@vger.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-te...@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))
+ 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

Maxime Ripard

未読、
2023/10/24 8:52:022023/10/24
To: Brendan Higgins、David Gow、Jani Nikula、Rae Moar、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org
On Wed, Sep 20, 2023 at 10:49:03AM +0200, Maxime Ripard 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 <mri...@kernel.org>

Ping?

Thanks!
Maxime
signature.asc

Rae Moar

未読、
2023/10/24 15:41:482023/10/24
To: Maxime Ripard、Brendan Higgins、David Gow、Jani Nikula、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org
On Wed, Sep 20, 2023 at 4:49 AM Maxime Ripard <mri...@kernel.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 <mri...@kernel.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
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?

Maxime Ripard

未読、
2023/10/25 5:17:252023/10/25
To: Rae Moar、Brendan Higgins、David Gow、Jani Nikula、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org
Hi Rae,

On Tue, Oct 24, 2023 at 03:41:33PM -0400, Rae Moar wrote:
> On Wed, Sep 20, 2023 at 4:49 AM Maxime Ripard <mri...@kernel.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 <mri...@kernel.org>
> >
>
> Hello!
>
> Thanks for following up! Sorry for the delay in this response.

np, I kind of forgot about it too to be fair :)

> 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.

Yeah, I agree there
Right. So I kind of disagree here. To me, the define should match the
definition we have for a slow test. We chose to report it only if it
exceeds it by a margin, but that's a separate thing from the actual
threshold.

I guess I could add a new version to make that distinction clearer.
Would that work for you?

Maxime
signature.asc

David Gow

未読、
2023/10/26 3:06:552023/10/26
To: Maxime Ripard、Brendan Higgins、Jani Nikula、Rae Moar、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org
On Wed, 20 Sept 2023 at 16:49, Maxime Ripard <mri...@kernel.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 <mri...@kernel.org>
>
> ---
>
> To: Brendan Higgins <brendan...@linux.dev>
> To: David Gow <davi...@google.com>
> Cc: Jani Nikula <jani....@linux.intel.com>
> Cc: Rae Moar <rm...@google.com>
> Cc: linux-k...@vger.kernel.org
> Cc: kuni...@googlegroups.com
> Cc: linux-...@vger.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-te...@kernel.org/
> ---

I quite like this, though agree somewhat with Rae's comments below.

I personally think the time thresholds are, by necessity, very
'fuzzy', due to the varying speeds of different hardware. Fortunately,
the actual runtime of tests seems pretty well stratified, so the exact
threshold doesn't really matter much.

I ran some tests here, and all of the tests currently not marked slow
take <1s on every machine I tried (including the ancient 66MHz 486),
except for the drm_mm_* ones (which takes ~6s on my laptop, and times
out after 15 minutes on the aforementioned 486). Both the 1s and 2s
timeouts successfully distinguish those cases.

Ideally, I think we'd have something like:
#define KUNIT_SPEED_SLOW_THRESHOLD_S 1 /* 1 sec threshold for 'slow' tests */
#define KUNIT_SPEED_WARNING_MULTIPLIER 2 /* Warn when a test takes >
twice the threshold. */
#define KUNIT_SPEED_SLOW_WARNING_THRESHOLD_S
(KUNIT_SPEED_WARNING_MULTIPLIER * KUNIT_SPEED_SLOW_THRESHOLD_S)

Which is perhaps excessively verbose, but is very clear as to what
we're doing. It also gives more scope to allow the ratio to be
configured for people with very slow / fast machines in the future.

Thoughts?

Otherwise, this looks good to me.

Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David

Maxime Ripard

未読、
2023/10/26 4:44:272023/10/26
To: David Gow、Brendan Higgins、Jani Nikula、Rae Moar、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org
Hi,
I had a similar experience running the tests in qemu on a Pi4, which is
probably the slowest machine we can reasonably expect.

> Ideally, I think we'd have something like:
> #define KUNIT_SPEED_SLOW_THRESHOLD_S 1 /* 1 sec threshold for 'slow' tests */
> #define KUNIT_SPEED_WARNING_MULTIPLIER 2 /* Warn when a test takes >
> twice the threshold. */
> #define KUNIT_SPEED_SLOW_WARNING_THRESHOLD_S
> (KUNIT_SPEED_WARNING_MULTIPLIER * KUNIT_SPEED_SLOW_THRESHOLD_S)
>
> Which is perhaps excessively verbose, but is very clear as to what
> we're doing. It also gives more scope to allow the ratio to be
> configured for people with very slow / fast machines in the future.
>
> Thoughts?

That looks like a good compromise to me, I'll send another version :)

Thanks!
Maxime
signature.asc

Maxime Ripard

未読、
2023/10/26 4:59:362023/10/26
To: Brendan Higgins、David Gow、Maxime Ripard、Jani Nikula、Rae Moar、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org
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 <mri...@kernel.org>

---

To: Brendan Higgins <brendan...@linux.dev>
To: David Gow <davi...@google.com>
Cc: Jani Nikula <jani....@linux.intel.com>
Cc: Rae Moar <rm...@google.com>
Cc: linux-k...@vger.kernel.org
Cc: kuni...@googlegroups.com
Cc: linux-...@vger.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.1...@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-te...@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,

David Gow

未読、
2023/10/26 23:57:142023/10/26
To: Maxime Ripard、Brendan Higgins、Jani Nikula、Rae Moar、linux-k...@vger.kernel.org、kuni...@googlegroups.com、linux-...@vger.kernel.org
On Thu, 26 Oct 2023 at 16:59, Maxime Ripard <mri...@kernel.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 <mri...@kernel.org>
>
> ---

Looks good to me!

Reviewed-by: David Gow <davi...@google.com>

Thanks,
-- David
全員に返信
投稿者に返信
転送
新着メール 0 件