On Thu, Sep 14, 2023 at 5:06 AM David Gow <davidgow@xxxxxxxxxx> wrote:
>
> On Sat, 9 Sept 2023 at 05:31, Rae Moar <rmoar@xxxxxxxxxx> wrote:
> >
> > Add functionality to run built-in tests after boot by writing to a
> > debugfs file.
> >
> > Add a new debugfs file labeled "run" for each test suite to use for
> > this purpose.
> >
> > As an example, write to the file using the following:
> >
> > echo "any string" > /sys/kernel/debugfs/kunit/<testsuite>/run
> >
> > This will trigger the test suite to run and will print results to the
> > kernel log.
> >
> > Note that what you "write" to the debugfs file will not be saved.
> >
> > To guard against running tests concurrently with this feature, add a
> > mutex lock around running kunit. This supports the current practice of
> > not allowing tests to be run concurrently on the same kernel.
> >
> > This functionality may not work for all tests.
> >
> > This new functionality could be used to design a parameter
> > injection feature in the future.
> >
> > Signed-off-by: Rae Moar <rmoar@xxxxxxxxxx>
> > ---
>
> This is looking pretty good, but I have a few nitpicks below and one big issue.
>
> The big issue is that this doesn't seem to exclude test suites created
> with kunit_test_init_section_suite{,s}(). The init section versions of
> the suite declarations, by definition, won't work if run after the
> kernel has finished booting. At the moment, these macros just pass
> through to the normal versions (because we've not been able to run
> after boot until now), but we'll need to implement it (maybe as a
> separate linker section, maybe as an attribute, etc) now. I expect
> that the correct solution here would be to not create the 'run'
> debugfs file for these tests. But I could be convinced to have it
> exist, but to just say "this test cannot be run after boot" if you've
> got a good argument. In any case, grep 'test.h' for "NOTE TO KUNIT
> DEVS" and you'll see the details.
>
> My one other not-totally-related thought (and this extends to module
> loading, too, so is possibly more useful as a separate patch) is that
> we're continually incrementing the test number still. This doesn't
> matter if we read the results from debugfs though, and it may make
> more sense to have this continue to increment (and thus treat all of
> dmesg as one long KTAP document). We could always add a reset option
> to debugfs in a follow-up patch if we want. But that's not something
> I'd hold this up with.
>
Hello!
Sorry for the delay in this response. I was working on other items but
I have started working on the next version of this patch.
Thanks for bringing my attention to the init tests. I am currently
working on a draft to remove the run files for these tests. However,
if that does not work, I will resort to outputting the message you
detailed above: "this test cannot be run after boot".
I am currently fine with the test number incrementing. However, I
would also be fine to implement a reset to ensure all the re-run
results have the test number of 1.
> >
> > Changes since v1:
> > - Removed second patch as this problem has been fixed
> > - Added Documentation patch
> > - Made changes to work with new dynamically-extending log feature
> >
> > Note that these patches now rely on (and are rebased on) the patch series:
> > https://lore.kernel.org/all/20230828104111.2394344-1-rf@xxxxxxxxxxxxxxxxxxxxx/
> >
> > lib/kunit/debugfs.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
> > lib/kunit/test.c | 13 +++++++++
> > 2 files changed, 79 insertions(+)
> >
> > diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c
> > index 270d185737e6..8c0a970321ce 100644
> > --- a/lib/kunit/debugfs.c
> > +++ b/lib/kunit/debugfs.c
> > @@ -8,12 +8,14 @@
> > #include <linux/module.h>
> >
> > #include <kunit/test.h>
> > +#include <kunit/test-bug.h>
> >
> > #include "string-stream.h"
> > #include "debugfs.h"
> >
> > #define KUNIT_DEBUGFS_ROOT "kunit"
> > #define KUNIT_DEBUGFS_RESULTS "results"
> > +#define KUNIT_DEBUGFS_RUN "run"
> >
> > /*
> > * Create a debugfs representation of test suites:
> > @@ -21,6 +23,8 @@
> > * Path Semantics
> > * /sys/kernel/debug/kunit/<testsuite>/results Show results of last run for
> > * testsuite
> > + * /sys/kernel/debug/kunit/<testsuite>/run Write to this file to trigger
> > + * testsuite to run
> > *
> > */
> >
> > @@ -99,6 +103,51 @@ static int debugfs_results_open(struct inode *inode, struct file *file)
> > return single_open(file, debugfs_print_results, suite);
> > }
> >
> > +/*
> > + * Print a usage message to the debugfs "run" file
> > + * (/sys/kernel/debug/kunit/<testsuite>/run) if opened.
> > + */
> > +static int debugfs_print_run(struct seq_file *seq, void *v)
> > +{
> > + struct kunit_suite *suite = (struct kunit_suite *)seq->private;
> > +
> > + seq_puts(seq, "Write to this file to trigger the test suite to run.\n");
> > + seq_printf(seq, "usage: echo \"any string\" > /sys/kernel/debugfs/kunit/%s/run\n",
> > + suite->name);
> > + return 0;
> > +}
> > +
> > +/*
> > + * The debugfs "run" file (/sys/kernel/debug/kunit/<testsuite>/run)
> > + * contains no information. Write to the file to trigger the test suite
> > + * to run.
> > + */
> > +static int debugfs_run_open(struct inode *inode, struct file *file)
> > +{
> > + struct kunit_suite *suite;
> > +
> > + suite = (struct kunit_suite *)inode->i_private;
> > +
> > + return single_open(file, debugfs_print_run, suite);
> > +}
> > +
> > +/*
> > + * Trigger a test suite to run by writing to the suite's "run" debugfs
> > + * file found at: /sys/kernel/debug/kunit/<testsuite>/run
> > + *
> > + * Note: what is written to this file will not be saved.
> > + */
> > +static ssize_t debugfs_run(struct file *file,
> > + const char __user *buf, size_t count, loff_t *ppos)
> > +{
> > + struct inode *f_inode = file->f_inode;
> > + struct kunit_suite *suite = (struct kunit_suite *) f_inode->i_private;
> > +
> > + __kunit_test_suites_init(&suite, 1);
> > +
> > + return count;
> > +}
> > +
> > static const struct file_operations debugfs_results_fops = {
> > .open = debugfs_results_open,
> > .read = seq_read,
> > @@ -106,10 +155,23 @@ static const struct file_operations debugfs_results_fops = {
> > .release = debugfs_release,
> > };
> >
> > +static const struct file_operations debugfs_run_fops = {
> > + .open = debugfs_run_open,
> > + .read = seq_read,
> > + .write = debugfs_run,
> > + .llseek = seq_lseek,
> > + .release = debugfs_release,
> > +};
> > +
> > void kunit_debugfs_create_suite(struct kunit_suite *suite)
> > {
> > struct kunit_case *test_case;
> >
> > + if (suite->log) {
> > + /* Clear the suite log that's leftover from a previous run. */
> > + string_stream_clear(suite->log);
> > + return;
> > + }
>
> Can we just move this to kunit_init_suite() in test.c. It doesn't feel
> quite debugfs-y enough, and the return here tripped me up for a little
> too long.
>
> Ideally, we'd then split up kunit_init_suite() into a one-time
> initialisation (which calls kunit_debugfs_create_suite()), and a reset
> function (which resets the state of the suite back to the beginning).
> We then only call init once, but reset on every execution.
I definitely think you are right here to move this into test.c. I will
try to put this into a reset function.
> > /* Allocate logs before creating debugfs representation. */
> > suite->log = alloc_string_stream(GFP_KERNEL);
> > string_stream_set_append_newlines(suite->log, true);
> > @@ -124,6 +186,10 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite)
> > debugfs_create_file(KUNIT_DEBUGFS_RESULTS, S_IFREG | 0444,
> > suite->debugfs,
> > suite, &debugfs_results_fops);
> > +
> > + debugfs_create_file(KUNIT_DEBUGFS_RUN, S_IFREG | 0644,
> > + suite->debugfs,
> > + suite, &debugfs_run_fops);
> > }
> >
> > void kunit_debugfs_destroy_suite(struct kunit_suite *suite)
> > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > index 651cbda9f250..d376b886d72d 100644
> > --- a/lib/kunit/test.c
> > +++ b/lib/kunit/test.c
> > @@ -13,6 +13,7 @@
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/moduleparam.h>
> > +#include <linux/mutex.h>
> > #include <linux/panic.h>
> > #include <linux/sched/debug.h>
> > #include <linux/sched.h>
> > @@ -22,6 +23,8 @@
> > #include "string-stream.h"
> > #include "try-catch-impl.h"
> >
> > +static struct mutex kunit_run_lock;
> > +
>
> Should we use DEFINE_MUTEX() here rather than initialising it at runtime?
I will try to implement this using DEFINE_MUTEX.
>
> > /*
> > * Hook to fail the current test and print an error message to the log.
> > */
> > @@ -668,6 +671,11 @@ int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_
> > return 0;
> > }
> >
> > + /* Use mutex lock to guard against running tests concurrently. */
> > + if (mutex_lock_interruptible(&kunit_run_lock)) {
> > + pr_err("kunit: test interrupted\n");
> > + return -EINTR;
> > + }
> > static_branch_inc(&kunit_running);
> >
> > for (i = 0; i < num_suites; i++) {
> > @@ -676,6 +684,7 @@ int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_
> > }
> >
> > static_branch_dec(&kunit_running);
> > + mutex_unlock(&kunit_run_lock);
> > return 0;
> > }
> > EXPORT_SYMBOL_GPL(__kunit_test_suites_init);
> > @@ -836,6 +845,10 @@ static int __init kunit_init(void)
> > kunit_install_hooks();
> >
> > kunit_debugfs_init();
> > +
> > + /* Initialize lock to guard against running tests concurrently. */
> > + mutex_init(&kunit_run_lock);
> > +
>
> As I understand it, we can just use DEFINE_MUTEX() above.
>
>
> > #ifdef CONFIG_MODULES
> > return register_module_notifier(&kunit_mod_nb);
> > #else
> >
> > base-commit: b754593274e04fc840482a658b29791bc8f8b933
> > --
> > 2.42.0.283.g2d96d420d3-goog
> >
