linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Gow <davidgow@google.com>
To: Jeremy Kerr <jk@codeconstruct.com.au>
Cc: Brendan Higgins <brendanhiggins@google.com>,
	Matt Johnston <matt@codeconstruct.com.au>,
	"open list:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module
Date: Fri, 28 Jan 2022 15:41:16 +0800	[thread overview]
Message-ID: <CABVgOSnpyAd-nmxB4VGahCoYep4HXmQ_YQWb=5EgyJpirCNT0w@mail.gmail.com> (raw)
In-Reply-To: <e5fa413ed59083ca63f3479d507b972380da0dcf.camel@codeconstruct.com.au>

On Thu, Jan 6, 2022 at 6:53 PM Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> Hi all,
>
> Happy new year! I'm just picking up this thread again, after having a
> bunch of other things come up at the end of December. I've since
> implemented some of the direct feedback on the patch, but wanted to
> clarify some overall direction too:

Thanks for reaching back out -- and sorry for the delay. I've dusted
everything off post-LCA now, and had another look over this.

I'm CCing both the KUnit mailing list and Daniel Latypov on this
thread so we can avoid (or track) any potential conflicts with things
like:
https://lore.kernel.org/linux-kselftest/20211013191320.2490913-1-dlatypov@google.com/


>
> > One idea I've had in the past is to keep such a list around of "test
> > suites to be run when KUnit is ready". This is partly because it's
> > much nicer to have all the test suites run together as part of a
> > single (K)TAP output, so this could be a way of implementing (at least
> > part of) that.
>
> I had a look at implementing this, but it doesn't seem to win us much
> with the current structure: since we kunit_run_all_tests() before a
> filesystem is available, kunit will always be "ready" (and the tests
> run) before we've had a chance to load modules, which may contain
> further tests.

I think the benefit of something like this isn't really with test
modules so much as with built-in tests which are run manually. The
thunderbolt test, for instance, currently initialises and runs tests
itself[1,2], rather than via the KUnit executor, so that it can ensure
some proportion of the stack is properly initialised. If there's a way
of coalescing those into the same TAP output as other built-in tests,
that'd be useful.

Thinking about it, though, I think it's a separate problem from module
support, so not worth shoehorning in at this stage.
>
> One option would be to defer kunit_run_all_tests() until we think we
> have the full set of tests, but there's no specific point at which we
> know that all required modules are loaded. We could defer this to an
> explicit user-triggered "run the tests now" interface (debugfs?), but
> that might break expectations of the tests automatically executing on
> init.

Yeah, while I do think it'd be neat to have an interface to explicitly
run (or re-run) tests, I think having tests run on module load is
still the most sensible thing, particularly since that's what people
are expecting at the moment (especially with tests which were ported
from standalone modules to KUnit).

There was a plan to allow test suites to be triggered from debugfs
individually at some point, but I think it got derailed as tests
weren't working if run twice, or some builtin-only tests having
problems if run after a userland was brought up.

In any case, I think we should get test suites in modules running on
module load, and leave any debugfs-related shenanigans for a future
patchset.

> Alternatively, I could properly split the TAP output, and just run tests
> whenever they're probed - either from the built-in set or as modules are
> loaded at arbitrary points in the future. However, I'm not sure of what
> the expectations on the consumer-side of the TAP output might be.

At the moment, the KUnit tooling will stop parsing after the first
full TAP output, so if suites are outputting TAP separately, only the
first one will be handled properly by kunit_tool. Of course,
kunit_tool doesn't really handle modules by itself at the moment, so
as long as the output is provided to kunit_tool one at a time, it's
still possible to use it. (And the possibility of adding a way for
kunit_tool to handle multiple TAP outputs in the same file is
something we plan to look into.)

So, I think this isn't a problem for modules at the moment, though
again it could be a bit painful for things like the thunderbolt test
where there are multiple TAP documents from built-ins.

Note that the updated KTAP[3] spec does actually leave the option open
for TAP output to effectively be "streaming" and to not state the
total number of tests in advance, but I don't think that's the ideal
way of handling it here.

> Are there any preferences on the approach here?

So, after all that, I think the original plan makes the most sense,
and if we find any cases which are particularly annoying we can either
change things then, or (better still) adapt the tooling to handle them
better.

Thanks again for looking into this!
-- David

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thunderbolt/test.c#n2730
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thunderbolt/domain.c#n881
[3]: https://www.kernel.org/doc/html/latest/dev-tools/ktap.html

  reply	other threads:[~2022-01-28  7:41 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-02  2:26 [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module Jeremy Kerr
2021-10-02  2:26 ` [PATCH net-next 2/2] mctp: test: defer mdev setup until we've registered Jeremy Kerr
2021-10-02  3:16   ` David Gow
2021-10-03  3:24     ` Jeremy Kerr
2021-10-02  3:10 ` [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module David Gow
2021-10-04  2:21   ` Jeremy Kerr
2021-10-06  8:01     ` David Gow
2021-10-11  2:10       ` Jeremy Kerr
2021-10-12  4:38         ` David Gow
2021-10-12 19:38           ` Brendan Higgins
2021-10-12 20:27           ` Daniel Latypov
2021-10-13 19:16             ` Daniel Latypov
2021-10-12 19:52     ` Brendan Higgins
2021-10-21  6:17       ` Jeremy Kerr
2021-10-21  7:06         ` David Gow
2021-10-25 20:54           ` Brendan Higgins
2022-01-06 10:53           ` Jeremy Kerr
2022-01-28  7:41             ` David Gow [this message]
2022-01-28  7:54               ` David Gow
2021-10-02 13:01 ` David Miller
2021-10-03  2:22   ` Jeremy Kerr

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABVgOSnpyAd-nmxB4VGahCoYep4HXmQ_YQWb=5EgyJpirCNT0w@mail.gmail.com' \
    --to=davidgow@google.com \
    --cc=brendanhiggins@google.com \
    --cc=jk@codeconstruct.com.au \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=matt@codeconstruct.com.au \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).