Re: [PATCH v3 linux-kselftest-test 5/6] kunit: allow kunit to be loaded as a module

[Brendan Higgins]

+Stephen Boyd - since he is more of an expert on the hung task timer than I am.

On Fri, Nov 8, 2019 at 7:30 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> On Thu, 7 Nov 2019, Brendan Higgins wrote:
>
> > On Thu, Oct 17, 2019 at 11:09 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
> > >
> > > Making kunit itself buildable as a module allows for "always-on"
> > > kunit configuration; specifying CONFIG_KUNIT=m means the module
> > > is built but only used when loaded.  Kunit test modules will load
> > > kunit.ko as an implicit dependency, so simply running
> > > "modprobe my-kunit-tests" will load the tests along with the kunit
> > > module and run them.
> > >
> > > Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> > > Signed-off-by: Knut Omang <knut.omang@xxxxxxxxxx>
> > > ---
> > >  lib/kunit/Kconfig     | 2 +-
> > >  lib/kunit/Makefile    | 4 +++-
> > >  lib/kunit/test.c      | 2 ++
> > >  lib/kunit/try-catch.c | 3 +++
> > >  4 files changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> > > index 9ebd5e6..065aa16 100644
> > > --- a/lib/kunit/Kconfig
> > > +++ b/lib/kunit/Kconfig
> > > @@ -3,7 +3,7 @@
> > >  #
> > >
> > >  menuconfig KUNIT
> > > -       bool "KUnit - Enable support for unit tests"
> > > +       tristate "KUnit - Enable support for unit tests"
> > >         help
> > >           Enables support for kernel unit tests (KUnit), a lightweight unit
> > >           testing and mocking framework for the Linux kernel. These tests are
> > > diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
> > > index 769d940..8e2635a 100644
> > > --- a/lib/kunit/Makefile
> > > +++ b/lib/kunit/Makefile
> > > @@ -1,4 +1,6 @@
> > > -obj-$(CONFIG_KUNIT) +=                 test.o \
> > > +obj-$(CONFIG_KUNIT) +=                 kunit.o
> > > +
> > > +kunit-objs +=                          test.o \
> > >                                         string-stream.o \
> > >                                         assert.o \
> > >                                         try-catch.o
> > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > > index e8b2443..c0ace36 100644
> > > --- a/lib/kunit/test.c
> > > +++ b/lib/kunit/test.c
> > > @@ -523,3 +523,5 @@ void *kunit_find_symbol(const char *sym)
> > >         return ERR_PTR(-ENOENT);
> > >  }
> > >  EXPORT_SYMBOL(kunit_find_symbol);
> > > +
> > > +MODULE_LICENSE("GPL");
> > > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> > > index 1c1e9af..72fc8ed 100644
> > > --- a/lib/kunit/try-catch.c
> > > +++ b/lib/kunit/try-catch.c
> > > @@ -31,6 +31,8 @@ static int kunit_generic_run_threadfn_adapter(void *data)
> > >         complete_and_exit(try_catch->try_completion, 0);
> > >  }
> > >
> > > +KUNIT_VAR_SYMBOL(sysctl_hung_task_timeout_secs, unsigned long);
> >
> > Can you just export sysctl_hung_task_timeout_secs?
> >
> > I don't mean to make you redo all this work for one symbol twice, but
> > I thought we agreed on just exposing this symbol, but in a namespace.
> > It seemed like a good use case for that namespaced exporting thing
> > that Luis was talking about. As I understood it, you would have to
> > export it in the module that defines it, and then use the new
> > MODULE_IMPORT_NS() macro here.
> >
>
> Sure, I can certainly look into that, though I wonder if we should
> consider another possibility - should kunit have its own sysctl table for
> things like configuring timeouts? I can look at adding a patch for that

So on the one hand, yes, I would like to have configurable test
timeouts for KUnit, but that is not what the parameter check is for
here. This is to make sure KUnit times a test case out before the hung
task timer does.

> prior to the module patch so the issues with exporting the hung task
> timeout would go away. Now the reason I suggest this isn't as much a hack
> to solve this specific problem, rather it seems to fit better with the
> longer-term intent expressed by the comment around use of the field (at
> least as I read it, I may be wrong).

Not really. Although I do agree that adding configurability here might
be a good idea, I believe we would need to clamp such a value by
sysctl_hung_task_timeout_secs regardless since we don't want to be
killed by the hung task timer; thus, we still need access to
sysctl_hung_task_timeout_secs either way, and so doing what you are
proposing would be off topic.

> Exporting the symbol does allow us to piggy-back on an existing value, but
> maybe we should support out our own tunable "kunit_timeout_secs" here?
> Doing so would also lay the groundwork for supporting other kunit
> tunables in the future if needed. What do you think?

The goal is not to piggy back on the value as I mentioned above.
Stephen, do you have any thoughts on this? Do you see any other
preferable solution to what Alan is trying to do?

> Many thanks for the review! I've got an updated patchset almost
> ready with the symbol lookup stuff removed; the above is the last issue
> outstanding from my side.

Awesome! No thanks necessary, I appreciate the work you are doing!
There were some other people who mentioned that they wanted this in
the past, so it is a really big help having you do this. I feel bad
that I couldn't get the review back to you faster. :-)

>
> > > +
> > >  static unsigned long kunit_test_timeout(void)
> > >  {
> > >         unsigned long timeout_msecs;
> > > @@ -52,6 +54,7 @@ static unsigned long kunit_test_timeout(void)
> > >          * For more background on this topic, see:
> > >          * https://mike-bland.com/2011/11/01/small-medium-large.html
> > >          */
> > > +       KUNIT_INIT_VAR_SYMBOL(NULL, sysctl_hung_task_timeout_secs);
> > >         if (sysctl_hung_task_timeout_secs) {
> > >                 /*
> > >                  * If sysctl_hung_task is active, just set the timeout to some
> > > --
> > > 1.8.3.1
> > >
> >