Linux-kselftest Archive on lore.kernel.org
 help / color / Atom feed
From: Alan Maguire <alan.maguire@oracle.com>
To: Brendan Higgins <brendanhiggins@google.com>
Cc: Alan Maguire <alan.maguire@oracle.com>,
	"open list:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	KUnit Development <kunit-dev@googlegroups.com>,
	Kees Cook <keescook@chromium.org>,
	Iurii Zaikin <yzaikin@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	catalin.marinas@arm.com, joe.lawrence@redhat.com,
	penguin-kernel@i-love.sakura.ne.jp, schowdary@nvidia.com,
	urezki@gmail.com, andriy.shevchenko@linux.intel.com,
	Jonathan Corbet <corbet@lwn.net>,
	"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
	Knut Omang <knut.omang@oracle.com>
Subject: Re: [PATCH v3 linux-kselftest-test 5/6] kunit: allow kunit to be loaded as a module
Date: Fri, 8 Nov 2019 15:30:15 +0000 (GMT)
Message-ID: <alpine.LRH.2.20.1911081520550.24027@dhcp-10-175-178-67.vpn.oracle.com> (raw)
In-Reply-To: <CAFd5g46s4eY4qEB5UZPeOKNdZXm4+sA9N=4g8gDYAhyhMahZKw@mail.gmail.com>

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

  parent reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-17 18:07 [PATCH v3 linux-kselftest-test 0/6] kunit: support building core/tests as modules Alan Maguire
2019-10-17 18:07 ` [PATCH v3 linux-kselftest-test 1/6] kunit: move string-stream.h to lib/kunit/string-stream-impl.h Alan Maguire
2019-11-08  1:01   ` Brendan Higgins
2019-10-17 18:07 ` [PATCH v3 linux-kselftest-test 2/6] kunit: hide unexported try-catch interface in try-catch-impl.h Alan Maguire
2019-11-08  1:07   ` Brendan Higgins
2019-10-17 18:07 ` [PATCH v3 linux-kselftest-test 3/6] kunit: add kunit_find_symbol() function for symbol lookup Alan Maguire
2019-11-08  1:24   ` Brendan Higgins
2019-10-17 18:07 ` [PATCH v3 linux-kselftest-test 4/6] kunit: allow kunit tests to be loaded as a module Alan Maguire
2019-11-08  1:40   ` Brendan Higgins
2019-10-17 18:07 ` [PATCH v3 linux-kselftest-test 5/6] kunit: allow kunit " Alan Maguire
2019-11-08  2:43   ` Brendan Higgins
2019-11-08  3:02     ` Brendan Higgins
2019-11-08 15:30     ` Alan Maguire [this message]
2019-11-11 21:41       ` Brendan Higgins
2019-10-17 18:07 ` [PATCH v3 linux-kselftest-test 6/6] kunit: update documentation to describe module-based build Alan Maguire

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LRH.2.20.1911081520550.24027@dhcp-10-175-178-67.vpn.oracle.com \
    --to=alan.maguire@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=brendanhiggins@google.com \
    --cc=catalin.marinas@arm.com \
    --cc=corbet@lwn.net \
    --cc=joe.lawrence@redhat.com \
    --cc=keescook@chromium.org \
    --cc=knut.omang@oracle.com \
    --cc=kunit-dev@googlegroups.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    --cc=schowdary@nvidia.com \
    --cc=urezki@gmail.com \
    --cc=yamada.masahiro@socionext.com \
    --cc=yzaikin@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-kselftest Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kselftest/0 linux-kselftest/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kselftest linux-kselftest/ https://lore.kernel.org/linux-kselftest \
		linux-kselftest@vger.kernel.org
	public-inbox-index linux-kselftest

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kselftest


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git