linux-kernel - Re: [PATCH v2] 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: <7nhmlcn3bbe3ey5tw43umsahjei3fisyxxp3vhf34lsi5s4wkr@qpp4bsdv4wom>
Date:   Wed, 25 Oct 2023 11:17:19 +0200
From:   Maxime Ripard <mripard@...nel.org>
To:     Rae Moar <rmoar@...gle.com>
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

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

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

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

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

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