linux-kernel - Re: [PATCH 1/2] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <l2eeghk7kz4rzsvlvvsj4vayo5s4ctnrizwkjolhaa2p3xdz75@jcczdtol52y7>
Date:   Mon, 11 Sep 2023 13:25:23 +0200
From:   Maxime Ripard <mripard@...nel.org>
To:     Jani Nikula <jani.nikula@...ux.intel.com>
Cc:     Brendan Higgins <brendan.higgins@...ux.dev>,
        David Gow <davidgow@...gle.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        linux-kernel@...r.kernel.org,
        Maíra Canal <mairacanal@...eup.net>,
        dri-devel@...ts.freedesktop.org, linux-kselftest@...r.kernel.org,
        kunit-dev@...glegroups.com
Subject: Re: [PATCH 1/2] kunit: Warn if tests are slow

Hi Jani,

On Mon, Sep 11, 2023 at 01:07:35PM +0300, Jani Nikula wrote:
> On Mon, 11 Sep 2023, 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>
> > ---
> >  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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)