Hi Jani,

On Mon, Sep 11, 2023 at 01:07:35PM +0300, Jani Nikula wrote:

On Mon, 11 Sep 2023, Maxime Ripard mripard@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 mripard@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);
  

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.

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

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

...

---

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);
  

I would consider moving this if statement into a separate function.

Ack.

I'll send a v2 with your suggestions

Thanks! Maxime

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 davidgow@google.com Signed-off-by: Maxime Ripard mripard@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

---

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