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