Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
MIME-Version: 1.0
References: <20231204221932.1465004-1-rmoar@google.com> <20231204221932.1465004-6-rmoar@google.com>
 <CABVgOS=JAn49ux6Cg2i1-V_2eNH4Utx_areqg4H1p5xwkT9FMA@mail.gmail.com>
In-Reply-To: <CABVgOS=JAn49ux6Cg2i1-V_2eNH4Utx_areqg4H1p5xwkT9FMA@mail.gmail.com>
From:   Rae Moar <rmoar@google.com>
Date:   Mon, 11 Dec 2023 17:16:36 -0500
Message-ID: <CA+GJov7fUjfKL=O2wC4ZKMvO8OMYJm07h0vGQRDxRF6v4EOpxg@mail.gmail.com>
Subject: Re: [PATCH v3 6/6] Documentation: Add debugfs docs with run after boot
To:     David Gow <davidgow@google.com>
Cc:     shuah@kernel.org, dlatypov@google.com, brendan.higgins@linux.dev,
        sadiyakazi@google.com, keescook@chromium.org, arnd@arndb.de,
        linux-kselftest@vger.kernel.org, linux-arch@vger.kernel.org,
        kunit-dev@googlegroups.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Dec 9, 2023 at 2:58=E2=80=AFAM David Gow <davidgow@google.com> wrot=
e:
>
> On Tue, 5 Dec 2023 at 06:19, Rae Moar <rmoar@google.com> wrote:
> >
> > Expand the documentation on the KUnit debugfs filesystem on the
> > run_manual.rst page.
> >
> > Add section describing how to access results using debugfs.
> >
> > Add section describing how to run tests after boot using debugfs.
> >
> > Signed-off-by: Rae Moar <rmoar@google.com>
> > ---
>
> Looks good to me, some nitpicks below.
>
> The other thing I'd really want to add is some documentation on
> writing init-section suites, which covers the pitfalls better (as
> mentioned in the previous emails). Though that could be a separate
> patch if you want to keep this one debugfs-specific.
>
> Otherwise,
> Reviewed-by: David Gow <davidgow@google.com>
>
> Cheers,
> -- David

Hello!

I have responded to your comments below. I would also be happy to add
documentation to the init-section suites either in this patch series
or in a future one.

Thanks!
-Rae

>
> >
> > Changes since v2:
> > - Add info to documentation about cleaning up data, init tests, and
> >   running tests concurrently
> >
> >  Documentation/dev-tools/kunit/run_manual.rst | 49 ++++++++++++++++++--
> >  1 file changed, 45 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/dev-tools/kunit/run_manual.rst b/Documentati=
on/dev-tools/kunit/run_manual.rst
> > index e7b46421f247..aebb52ba9605 100644
> > --- a/Documentation/dev-tools/kunit/run_manual.rst
> > +++ b/Documentation/dev-tools/kunit/run_manual.rst
> > @@ -49,9 +49,50 @@ loaded.
> >
> >  The results will appear in TAP format in ``dmesg``.
> >
> > +debugfs
> > +=3D=3D=3D=3D=3D=3D=3D
> > +
> > +``debugfs`` is a file system that enables user interaction with the fi=
les to
> > +make kernel information available to user space (See more information =
at
> > +Documentation/filesystems/debugfs.html)
>
> Nit: reference debugfs.rst here, not debugfs.html -- sphinx ought to
> update the link automatically.

Thanks for catching this! I didn't realize Sphinx would update it.

>
> Also, maybe we can make this introduction a _little_ bit more
> KUnit-specific. I'd personally start by saying that KUnit can be
> accessed from userspace via the debugfs filesystem (link), usually
> mounted in /sys/kernel/debug/kunit, etc, if CONFIG_KUNIT_DEBUGFS is
> enabled.

Ok I will add this for the next version.

>
> > +
> > +By default, only the root user has access to the debugfs directory.
> > +
> > +If ``CONFIG_KUNIT_DEBUGFS`` is enabled, you can use KUnit debugfs
> > +filesystem to perform the following actions.
> > +
> > +Retrieve Test Results
> > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > +
> > +You can use debugfs to retrieve KUnit test results. The test results a=
re
> > +accessible from the debugfs filesystem in the following read-only file=
:
> > +
> > +.. code-block :: bash
> > +
> > +       /sys/kernel/debug/kunit/<test_suite>/results
> > +
> > +The test results are available in KTAP format.
>
> Do we want to note that this is a separate KTAP document, and so may
> have different suite numbering from the dmesg log?

Sure! I will add this for the next version.

>
> > +
> > +Run Tests After Kernel Has Booted
> > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > +
> > +You can use the debugfs filesystem to trigger built-in tests to run af=
ter
> > +boot. To run the test suite, you can use the following command to writ=
e to
> > +the ``/sys/kernel/debug/kunit/<test_suite>/run`` file:
> > +
> > +.. code-block :: bash
> > +
> > +       echo "any string" > /sys/kernel/debugfs/kunit/<test_suite>/run
> > +
> > +As a result, the test suite runs and the results are printed to the ke=
rnel
> > +log.
> > +
> > +However, this feature is not available with KUnit tests that use init =
data.
>
> Let's expand this slightly, and mention that this is because the data
> may have already been discarded, and that you can find such tests by
> either looking for the kunit_test_init_section_suites() macro or the
> is_init attribute.

Got it. I will definitely expand this more.

>
> > +
> > +Also, you cannot use this feature to run tests concurrently as there i=
s a
> > +mutex lock around running KUnit tests at the same time.
> > +
>
> Instead of mentioning the mutex, which is an implementation detail,
> just mention that tests will either wait for other tests to complete,
> or fail having timed out.
>

I will definitely switch this out in the next version.

>
>
> >  .. note ::
> >
> > -       If ``CONFIG_KUNIT_DEBUGFS`` is enabled, KUnit test results will
> > -       be accessible from the ``debugfs`` filesystem (if mounted).
> > -       They will be in ``/sys/kernel/debug/kunit/<test_suite>/results`=
`, in
> > -       TAP format.
> > +       For test authors, to use this feature, tests will need to corre=
ctly initialise
> > +       and/or clean up any data, so the test runs correctly a second t=
ime.
> > --
> > 2.43.0.rc2.451.g8631bc7472-goog
> >