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