[v3,linux-kselftest-test,5/6] kunit: allow kunit to be loaded as a module
diff mbox series

Message ID 1571335639-21675-6-git-send-email-alan.maguire@oracle.com
State New
Headers show
Series
  • kunit: support building core/tests as modules
Related show

Commit Message

Alan Maguire Oct. 17, 2019, 6:07 p.m. UTC
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@oracle.com>
Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 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(-)

Comments

Brendan Higgins Nov. 8, 2019, 2:43 a.m. UTC | #1
On Thu, Oct 17, 2019 at 11:09 AM Alan Maguire <alan.maguire@oracle.com> 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@oracle.com>
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  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.

> +
>  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
>
Brendan Higgins Nov. 8, 2019, 3:02 a.m. UTC | #2
On Thu, Nov 07, 2019 at 06:43:11PM -0800, Brendan Higgins wrote:
> On Thu, Oct 17, 2019 at 11:09 AM Alan Maguire <alan.maguire@oracle.com> 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@oracle.com>
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > ---
> >  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.

Also, I tried applying this and running this on kselftest/test and got
the following build error:

In file included from lib/kunit/try-catch.c:10:
lib/kunit/try-catch.c: In function ‘kunit_test_timeout’:
./include/kunit/test.h:132:19: error: lvalue required as unary ‘&’ operand
  kunit_##symbol = &(symbol)
                   ^
lib/kunit/try-catch.c:57:2: note: in expansion of macro ‘KUNIT_INIT_VAR_SYMBOL’
  KUNIT_INIT_VAR_SYMBOL(NULL, sysctl_hung_task_timeout_secs);
  ^~~~~~~~~~~~~~~~~~~~~

> > +
> >  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
> >
Alan Maguire Nov. 8, 2019, 3:30 p.m. UTC | #3
On Thu, 7 Nov 2019, Brendan Higgins wrote:

> On Thu, Oct 17, 2019 at 11:09 AM Alan Maguire <alan.maguire@oracle.com> 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@oracle.com>
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > ---
> >  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 
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).

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?

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.

Thanks!

Alan

> > +
> >  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
> >
>
Brendan Higgins Nov. 11, 2019, 9:41 p.m. UTC | #4
+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@oracle.com> wrote:
>
> On Thu, 7 Nov 2019, Brendan Higgins wrote:
>
> > On Thu, Oct 17, 2019 at 11:09 AM Alan Maguire <alan.maguire@oracle.com> 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@oracle.com>
> > > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > > ---
> > >  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
> > >
> >
Brendan Higgins Nov. 14, 2019, 9:33 p.m. UTC | #5
On Thu, Nov 14, 2019 at 1:31 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> +kselftest and kunit lists to document this decision.

Sorry for the spam. I accidentally CC'ed the doc list instead of the
kselftest list in my previous email.

> On Wed, Nov 13, 2019 at 11:54 PM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > On Wed, 13 Nov 2019, Stephen Boyd wrote:
> >
> > > Quoting Brendan Higgins (2019-11-11 13:41:38)
> > > > +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@oracle.com> wrote:
> > > > >
> > > > > On Thu, 7 Nov 2019, Brendan Higgins wrote:
> > > > >
> > > > > > On Thu, Oct 17, 2019 at 11:09 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > > > > > > +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?
> > >
> > > One idea would be to make some sort of process flag that says "this is a
> > > kunit task, ignore me with regards to the hung task timeout". Then we
> > > can hardcode the 5 minute kunit timeout. I'm not sure we have any more
> > > flags though.
> > >
> > > Or drop the whole timeout clamping logic, let the hung task timeout kick
> > > in and potentially oops the kernel, but then continue to let the test
> > > run and maybe sometimes get the kunit timeout here. This last option
> > > doesn't sound so bad to me given that this is all a corner case anyway
> > > where we don't expect to actually ever hit this problem so letting the
> > > hung task detector do its job is probably fine. This nicely avoids
> > > having to export this symbol to modules too.
> > >
> >
> > Thanks for suggestions! This latter approach seems fine to me; presumably
> > something has gone wrong if we are tripping the hung task timeout anyway,
> > so having an oops to document that seems fine. Brendan, what do you think?
>
> If Stephen thinks it is fine to drop the clamping logic, I think it is
> fine too. I think it would probably be good to replace it with a
> comment under the TODO that explains that a hung test *can* cause an
> oops if the hung task timeout is less than the kunit timeout value. It
> would probably be good to also select a timeout value that is less
> than the default hung task timeout. We might also want to link to this
> discussion. I fully expect that the timeout logic will get more
> attention at some point in the future.
>
> One more thing: Alan, can you submit the commit that drops the
> clamping logic in its own commit? I would prefer to make sure that it
> is easy to spot in the commit history.
>
> Cheers!

Patch
diff mbox series

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);
+
 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