Re: [Intel-gfx] KUnit issues - Was: [igt-dev] [PATCH RFC v2 8/8] drm/i915: check if current->mm is not NULL
On Fri, Nov 4, 2022 at 12:20 PM Daniel Latypov wrote: > rm...@google.com was working on them and should hopefully be able to > send them out real soon. > You should get CC'd on those. > > I think the follow-up work is just crafting an example parser input > file and iterating until > $ ./tools/testing/kunit/kunit.py parse < /tmp/example_input > produces our desired results. I briefly played around with this on top of https://lore.kernel.org/linux-kselftest/20221104194705.3245738-1-rm...@google.com/ But instead, I tried to make it so kunit.py parse could be just given a directory as input so we don't have to do `find | xargs cat`. I can't get module tests to work locally, so I'll replicate the setup with some regular files. Say we have /tmp/results with two files: file1 contains # Subtest: suite1 KTAP version 1 1..1 ok 1 test ok 1 suite1 and file2 contains # Subtest: suite2 KTAP version 1 1..1 ok 1 test ok 1 suite2 I think this should match what module output in debugfs would look like. A key thing to note: the top level test counter resets between modules, iirc. The patch below makes this almost work $ ./tools/testing/kunit/kunit.py parse /tmp/results/ suite2 (1 subtest) [PASSED] test = [PASSED] suite2 == suite1 (1 subtest) [PASSED] test [ERROR] Test: suite1: Expected test number 2 but found 1 = [PASSED] suite1 == Testing complete. Ran 2 tests: passed: 2, errors: 1 But there's a few issues a. the error about multiple top level tests with "test number 1" b. how do we make this handle kernel output with prefixes (timestamps) c. and what happens if files each have different length prefixes? diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 4d4663fb578b..c5b2eb416c2d 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -189,6 +189,14 @@ def _map_to_overall_status(test_status: kunit_parser.TestStatus) -> KunitStatus: return KunitStatus.SUCCESS return KunitStatus.TEST_FAILURE +def _kernel_output_from_dir(dir: str) -> Iterable[str]: + yield 'KTAP version 1' + for root, dirs, files in os.walk(dir): + for sub in files: + with open(os.path.join(root, sub), errors='backslashreplace') as f: + for line in f: + yield line + def parse_tests(request: KunitParseRequest, metadata: kunit_json.Metadata, input_data: Iterable[str]) -> Tuple[KunitResult, kunit_parser.Test]: parse_start = time.time() @@ -496,6 +504,8 @@ def main(argv): if cli_args.file is None: sys.stdin.reconfigure(errors='backslashreplace') # pytype: disable=attribute-error kunit_output = sys.stdin + elif os.path.isdir(cli_args.file): + kunit_output = _kernel_output_from_dir(cli_args.file) else: with open(cli_args.file, 'r', errors='backslashreplace') as f: kunit_output = f.read().splitlines()
Re: [Intel-gfx] KUnit issues - Was: [igt-dev] [PATCH RFC v2 8/8] drm/i915: check if current->mm is not NULL
On Mon, Nov 7, 2022 at 10:38 AM Michał Winiarski wrote: > > On Thu, Nov 03, 2022 at 04:23:02PM +0100, Mauro Carvalho Chehab wrote: > > Hi, > > > > I'm facing a couple of issues when testing KUnit with the i915 driver. > > > > The DRM subsystem and the i915 driver has, for a long time, his own > > way to do unit tests, which seems to be added before KUnit. > > > > I'm now checking if it is worth start using KUnit at i915. So, I wrote > > a RFC with some patches adding support for the tests we have to be > > reported using Kernel TAP and KUnit. > > > > There are basically 3 groups of tests there: > > > > - mock tests - check i915 hardware-independent logic; > > - live tests - run some hardware-specific tests; > > - perf tests - check perf support - also hardware-dependent. > > > > As they depend on i915 driver, they run only on x86, with PCI > > stack enabled, but the mock tests run nicely via qemu. > > > > The live and perf tests require a real hardware. As we run them > > together with our CI, which, among other things, test module > > unload/reload and test loading i915 driver with different > > modprobe parameters, the KUnit tests should be able to run as > > a module. > > Note that KUnit tests that are doing more of a functional/integration > testing (on "live" hardware) rather than unit testing (where hardware > interactions are mocked) are not very common. > Do we have other KUnit tests like this merged? I don't think we have other tests like this. > Some of the "live tests" are not even that, being more of a pure > hardware tests (e.g. live_workarounds, which is checking whether values > in MMIO regs stick over various HW state transitions). > > I'm wondering, is KUnit the right tool for this job? The main focus of KUnit is for hw-independent tests. So in theory: no. But I can imagine it could be easier to write the validation via KUNIT_EXPECT_EQ and friends as opposed to writing your own kernel module w/ its own set of macros, etc. So my first thought is: "if it works, then you can try using it." (Might want to take steps like make sure they don't get enabled by CONFIG_KUNIT_ALL_TESTS=y). Talking with David, he seems to have echoed my thoughts. David also suggested that maybe the test could use a fake of the hw by default, but have an option to run against real hw when available. I think that sounds like a good chunk of work, so I don't know if you need to worry about that. Daniel
Re: [Intel-gfx] KUnit issues - Was: [igt-dev] [PATCH RFC v2 8/8] drm/i915: check if current->mm is not NULL
On Thu, Nov 3, 2022 at 8:23 AM Mauro Carvalho Chehab wrote: > > Hi, > > I'm facing a couple of issues when testing KUnit with the i915 driver. > > The DRM subsystem and the i915 driver has, for a long time, his own > way to do unit tests, which seems to be added before KUnit. > > I'm now checking if it is worth start using KUnit at i915. So, I wrote > a RFC with some patches adding support for the tests we have to be > reported using Kernel TAP and KUnit. > > There are basically 3 groups of tests there: > > - mock tests - check i915 hardware-independent logic; > - live tests - run some hardware-specific tests; > - perf tests - check perf support - also hardware-dependent. > > As they depend on i915 driver, they run only on x86, with PCI > stack enabled, but the mock tests run nicely via qemu. > > The live and perf tests require a real hardware. As we run them > together with our CI, which, among other things, test module > unload/reload and test loading i915 driver with different > modprobe parameters, the KUnit tests should be able to run as > a module. > > While testing KUnit, I noticed a couple of issues: > > 1. kunit.py parser is currently broken when used with modules > > the parser expects "TAP version xx" output, but this won't > happen when loading the kunit test driver. > > Are there any plans or patches fixing this issue? Partially. Note: we need a header to look for so we can strip prefixes (like timestamps). But there is a patch in the works to add a TAP header for each subtest, hopefully in time for 6.2. This is to match the KTAP spec: https://kernel.org/doc/html/latest/dev-tools/ktap.html That should fix it so you can parse one suite's results at a time. I'm pretty sure it won't fix the case where there's multiple suites and/or you're trying to parse all test results at once via $ find /sys/kernel/debug/kunit/ -type f | xargs cat | ./tools/testing/kunit/kunit.py parse I think that in-kernel code change + some more python changes could make the above command work, but no one has actively started looking at that just yet. Hopefully we can pick this up and also get it done for 6.2 (unless I'm underestimating how complicated this is). > > 2. current->mm is not initialized > > Some tests do mmap(). They need the mm user context to be initialized, > but this is not happening right now. > > Are there a way to properly initialize it for KUnit? Right, this is a consequence of how early built-in KUnit tests are run after boot. I think for now, the answer is to make the test module-only. I know David had some ideas here, but I can't speak to them. > > 3. there's no test filters for modules > > In order to be able to do proper CI automation, it is needed to > be able to control what tests will run or not. That's specially > interesting at development time where some tests may not apply > or not run properly on new hardware. > > Are there any plans to add support for it at kunit_test_suites() > when the driver is built as module? Ideally, the best would be to > export a per-module filter_glob parameter on such cases. I think this is a good idea and is doable. (I think I said as much on the other thread). The thinking before was that people would make group tests together in modules. But if you want to share a single module for many tests, this becomes more useful. This has some potential merge conflicts w/ other pending work. I was also prototyping the ability to tell KUnit "run tests #2 - #5", so that also touches the filtering code very heavily. (The goal there is to have kunit.py able to shard up tests and boot multiple kernels concurrently.) > > 4. there are actually 3 levels of tests on i915: > - Level 1: mock, live, perf > - Level 2: test group (mmap, fences, ...) > - Level 3: unit tests > > Currently, KUnit seems to have just two levels (test suite and tests). > Are there a way to add test groups there? Parameterized tests are the closest we have to a third-level of tests. But other than that, the answer is no. I'd need to get more familiar with the existing tests, but I'm pretty sure parameters won't work for you. And I don't know if this will get done. Note: the kunit_parser.py code should be able to handle arbitrary levels of tests in the output. This restriction is purely in the in-kernel code. I had brought up the idea of more layers of tests before. It would also be useful for a) sharing expensive setup between multiple tests b) allowing more granular scope for cleanups (kunit_kmalloc and others) c) more flexibility in dynamically generating subtests than parameterized testing There's some precedent in other unit testing frameworks, for example: https://pkg.go.dev/testing#T.Run Daniel
Re: [Intel-gfx] KUnit issues - Was: [igt-dev] [PATCH RFC v2 8/8] drm/i915: check if current->mm is not NULL
On Fri, Nov 4, 2022 at 12:50 AM Mauro Carvalho Chehab
wrote:
>
> On Thu, 3 Nov 2022 15:43:26 -0700
> Daniel Latypov wrote:
>
> > On Thu, Nov 3, 2022 at 8:23 AM Mauro Carvalho Chehab
> > wrote:
> > >
> > > Hi,
> > >
> > > I'm facing a couple of issues when testing KUnit with the i915 driver.
> > >
> > > The DRM subsystem and the i915 driver has, for a long time, his own
> > > way to do unit tests, which seems to be added before KUnit.
> > >
> > > I'm now checking if it is worth start using KUnit at i915. So, I wrote
> > > a RFC with some patches adding support for the tests we have to be
> > > reported using Kernel TAP and KUnit.
> > >
> > > There are basically 3 groups of tests there:
> > >
> > > - mock tests - check i915 hardware-independent logic;
> > > - live tests - run some hardware-specific tests;
> > > - perf tests - check perf support - also hardware-dependent.
> > >
> > > As they depend on i915 driver, they run only on x86, with PCI
> > > stack enabled, but the mock tests run nicely via qemu.
> > >
> > > The live and perf tests require a real hardware. As we run them
> > > together with our CI, which, among other things, test module
> > > unload/reload and test loading i915 driver with different
> > > modprobe parameters, the KUnit tests should be able to run as
> > > a module.
> > >
> > > While testing KUnit, I noticed a couple of issues:
> > >
> > > 1. kunit.py parser is currently broken when used with modules
> > >
> > > the parser expects "TAP version xx" output, but this won't
> > > happen when loading the kunit test driver.
> > >
> > > Are there any plans or patches fixing this issue?
> >
> > Partially.
> > Note: we need a header to look for so we can strip prefixes (like
> > timestamps).
> >
> > But there is a patch in the works to add a TAP header for each
> > subtest, hopefully in time for 6.2.
>
> Good to know.
>
> > This is to match the KTAP spec:
> > https://kernel.org/doc/html/latest/dev-tools/ktap.html
>
> I see.
>
> > That should fix it so you can parse one suite's results at a time.
> > I'm pretty sure it won't fix the case where there's multiple suites
> > and/or you're trying to parse all test results at once via
> >
> > $ find /sys/kernel/debug/kunit/ -type f | xargs cat |
> > ./tools/testing/kunit/kunit.py parse
>
> Could you point me to the changeset? perhaps I can write a followup
> patch addressing this case.
rm...@google.com was working on them and should hopefully be able to
send them out real soon.
You should get CC'd on those.
I think the follow-up work is just crafting an example parser input
file and iterating until
$ ./tools/testing/kunit/kunit.py parse < /tmp/example_input
produces our desired results.
>
> > I think that in-kernel code change + some more python changes could
> > make the above command work, but no one has actively started looking
> > at that just yet.
> > Hopefully we can pick this up and also get it done for 6.2 (unless I'm
> > underestimating how complicated this is).
> >
> > >
> > > 2. current->mm is not initialized
> > >
> > > Some tests do mmap(). They need the mm user context to be initialized,
> > > but this is not happening right now.
> > >
> > > Are there a way to properly initialize it for KUnit?
> >
> > Right, this is a consequence of how early built-in KUnit tests are run
> > after boot.
> > I think for now, the answer is to make the test module-only.
> >
> > I know David had some ideas here, but I can't speak to them.
>
> This is happening when test-i915 is built as module as well.
Oh, I didn't expect that at all.
>
> I suspect that the function which initializes it is mm_alloc() inside
> kernel/fork.c:
>
> struct mm_struct *mm_alloc(void)
> {
> struct mm_struct *mm;
>
> mm = allocate_mm();
> if (!mm)
> return NULL;
>
> memset(mm, 0, sizeof(*mm));
> return mm_init(mm, current, current_user_ns());
> }
>
> As modprobing a test won't fork until all tests run, this never runs.
>
> It seems that the normal usage is at fs/exec.c:
>
> fs/exec.c: bprm->mm = mm = mm_alloc();
>
> but other places also call it:
>
> arch/arm/mach-rpc/ecard.c: struct mm_struct * mm = mm_alloc();
> driver
