| Message ID | 20230421040218.2156548-1-davidgow@google.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [v3,1/4] kunit: Always run cleanup from a test kthread | expand |
Hi David,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.3-rc7 next-20230420]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/David-Gow/Documentation-kunit-Note-that-assertions-should-not-be-used-in-cleanup/20230421-120437
patch link: https://lore.kernel.org/r/20230421040218.2156548-1-davidgow%40google.com
patch subject: [PATCH v3 1/4] kunit: Always run cleanup from a test kthread
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230421/202304211445.r8UQGW3F-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/e6f2b343739c4656e2090449ad7eac10db57dde9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review David-Gow/Documentation-kunit-Note-that-assertions-should-not-be-used-in-cleanup/20230421-120437
git checkout e6f2b343739c4656e2090449ad7eac10db57dde9
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash lib/kunit/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304211445.r8UQGW3F-lkp@intel.com/
All warnings (new ones prefixed by >>):
lib/kunit/test.c: In function 'kunit_catch_run_case':
>> lib/kunit/test.c:440:29: warning: unused variable 'suite' [-Wunused-variable]
440 | struct kunit_suite *suite = ctx->suite;
| ^~~~~
vim +/suite +440 lib/kunit/test.c
e6f2b343739c46 David Gow 2023-04-21 434
e6f2b343739c46 David Gow 2023-04-21 435
5f3e06208920ee Brendan Higgins 2019-09-23 436 static void kunit_catch_run_case(void *data)
5f3e06208920ee Brendan Higgins 2019-09-23 437 {
5f3e06208920ee Brendan Higgins 2019-09-23 438 struct kunit_try_catch_context *ctx = data;
5f3e06208920ee Brendan Higgins 2019-09-23 439 struct kunit *test = ctx->test;
5f3e06208920ee Brendan Higgins 2019-09-23 @440 struct kunit_suite *suite = ctx->suite;
5f3e06208920ee Brendan Higgins 2019-09-23 441 int try_exit_code = kunit_try_catch_get_result(&test->try_catch);
5f3e06208920ee Brendan Higgins 2019-09-23 442
5f3e06208920ee Brendan Higgins 2019-09-23 443 if (try_exit_code) {
5f3e06208920ee Brendan Higgins 2019-09-23 444 kunit_set_failure(test);
5f3e06208920ee Brendan Higgins 2019-09-23 445 /*
5f3e06208920ee Brendan Higgins 2019-09-23 446 * Test case could not finish, we have no idea what state it is
5f3e06208920ee Brendan Higgins 2019-09-23 447 * in, so don't do clean up.
5f3e06208920ee Brendan Higgins 2019-09-23 448 */
5f3e06208920ee Brendan Higgins 2019-09-23 449 if (try_exit_code == -ETIMEDOUT) {
5f3e06208920ee Brendan Higgins 2019-09-23 450 kunit_err(test, "test case timed out\n");
5f3e06208920ee Brendan Higgins 2019-09-23 451 /*
5f3e06208920ee Brendan Higgins 2019-09-23 452 * Unknown internal error occurred preventing test case from
5f3e06208920ee Brendan Higgins 2019-09-23 453 * running, so there is nothing to clean up.
5f3e06208920ee Brendan Higgins 2019-09-23 454 */
5f3e06208920ee Brendan Higgins 2019-09-23 455 } else {
5f3e06208920ee Brendan Higgins 2019-09-23 456 kunit_err(test, "internal error occurred preventing test case from running: %d\n",
5f3e06208920ee Brendan Higgins 2019-09-23 457 try_exit_code);
5f3e06208920ee Brendan Higgins 2019-09-23 458 }
5f3e06208920ee Brendan Higgins 2019-09-23 459 return;
5f3e06208920ee Brendan Higgins 2019-09-23 460 }
5f3e06208920ee Brendan Higgins 2019-09-23 461 }
5f3e06208920ee Brendan Higgins 2019-09-23 462
Hi, On Fri, 2023-04-21 at 12:02 +0800, David Gow wrote: > KUnit tests run in a kthread, with the current->kunit_test pointer set > to the test's context. This allows the kunit_get_current_test() and > kunit_fail_current_test() macros to work. Normally, this pointer is > still valid during test shutdown (i.e., the suite->exit function, and > any resource cleanup). However, if the test has exited early (e.g., due > to a failed assertion), the cleanup is done in the parent KUnit thread, > which does not have an active context. > > Instead, in the event test terminates early, run the test exit and > cleanup from a new 'cleanup' kthread, which sets current->kunit_test, > and better isolates the rest of KUnit from issues which arise in test > cleanup. > > If a test cleanup function itself aborts (e.g., due to an assertion > failing), there will be no further attempts to clean up: an error will > be logged and the test failed. For example: > # example_simple_test: test aborted during cleanup. continuing without cleaning up > > This should also make it easier to get access to the KUnit context, > particularly from within resource cleanup functions, which may, for > example, need access to data in test->priv. > > Signed-off-by: David Gow <davidgow@google.com> Great! Looks good to me. Reviewed-by: Benjamin Berg <benjamin.berg@intel.com> > --- > > This is an updated version of / replacement of "kunit: Set the current > KUnit context when cleaning up", which instead creates a new kthread > for cleanup tasks if the original test kthread is aborted. This protects > us from failed assertions during cleanup, if the test exited early. > > Changes since v2: > https://lore.kernel.org/linux-kselftest/20230419085426.1671703-1-davidgow@google.com/ > - Always run cleanup in its own kthread > - Therefore, never attempt to re-run it if it exits > - Thanks, Benjamin. > Changes since v1: > https://lore.kernel.org/linux-kselftest/20230415091401.681395-1-davidgow@google.com/ > - Move cleanup execution to another kthread > - (Thanks, Benjamin, for pointing out the assertion issues) > > --- > lib/kunit/test.c | 55 ++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 48 insertions(+), 7 deletions(-) > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index e2910b261112..2025e51941e6 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -419,10 +419,50 @@ static void kunit_try_run_case(void *data) > * thread will resume control and handle any necessary clean up. > */ > kunit_run_case_internal(test, suite, test_case); > - /* This line may never be reached. */ > +} > + > +static void kunit_try_run_case_cleanup(void *data) > +{ > + struct kunit_try_catch_context *ctx = data; > + struct kunit *test = ctx->test; > + struct kunit_suite *suite = ctx->suite; > + > + current->kunit_test = test; > + > kunit_run_case_cleanup(test, suite); > } > > +static void kunit_catch_run_case_cleanup(void *data) > +{ > + struct kunit_try_catch_context *ctx = data; > + struct kunit *test = ctx->test; > + int try_exit_code = kunit_try_catch_get_result(&test->try_catch); > + > + /* It is always a failure if cleanup aborts. */ > + kunit_set_failure(test); > + > + if (try_exit_code) { > + /* > + * Test case could not finish, we have no idea what state it is > + * in, so don't do clean up. > + */ > + if (try_exit_code == -ETIMEDOUT) { > + kunit_err(test, "test case cleanup timed out\n"); > + /* > + * Unknown internal error occurred preventing test case from > + * running, so there is nothing to clean up. > + */ > + } else { > + kunit_err(test, "internal error occurred during test case cleanup: %d\n", > + try_exit_code); > + } > + return; > + } > + > + kunit_err(test, "test aborted during cleanup. continuing without cleaning up\n"); > +} > + > + > static void kunit_catch_run_case(void *data) > { > struct kunit_try_catch_context *ctx = data; > @@ -448,12 +488,6 @@ static void kunit_catch_run_case(void *data) > } > return; > } > - > - /* > - * Test case was run, but aborted. It is the test case's business as to > - * whether it failed or not, we just need to clean up. > - */ > - kunit_run_case_cleanup(test, suite); > } > > /* > @@ -478,6 +512,13 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite, > context.test_case = test_case; > kunit_try_catch_run(try_catch, &context); > > + /* Now run the cleanup */ > + kunit_try_catch_init(try_catch, > + test, > + kunit_try_run_case_cleanup, > + kunit_catch_run_case_cleanup); > + kunit_try_catch_run(try_catch, &context); > + > /* Propagate the parameter result to the test case. */ > if (test->status == KUNIT_FAILURE) > test_case->status = KUNIT_FAILURE;
On Fri, Apr 21, 2023 at 12:02:15PM +0800, David Gow wrote: > KUnit tests run in a kthread, with the current->kunit_test pointer set > to the test's context. This allows the kunit_get_current_test() and > kunit_fail_current_test() macros to work. Normally, this pointer is > still valid during test shutdown (i.e., the suite->exit function, and > any resource cleanup). However, if the test has exited early (e.g., due > to a failed assertion), the cleanup is done in the parent KUnit thread, > which does not have an active context. > > Instead, in the event test terminates early, run the test exit and > cleanup from a new 'cleanup' kthread, which sets current->kunit_test, > and better isolates the rest of KUnit from issues which arise in test > cleanup. > > If a test cleanup function itself aborts (e.g., due to an assertion > failing), there will be no further attempts to clean up: an error will > be logged and the test failed. For example: > # example_simple_test: test aborted during cleanup. continuing without cleaning up > > This should also make it easier to get access to the KUnit context, > particularly from within resource cleanup functions, which may, for > example, need access to data in test->priv. > > Signed-off-by: David Gow <davidgow@google.com> Thanks for doing this. I've tested it with an action that needs the test priv pointer, and it works as expected Reviewed-by: Maxime Ripard <maxime@cerno.tech> Tested-by: Maxime Ripard <maxime@cerno.tech> Thanks! Maxime
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index e2910b261112..2025e51941e6 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -419,10 +419,50 @@ static void kunit_try_run_case(void *data) * thread will resume control and handle any necessary clean up. */ kunit_run_case_internal(test, suite, test_case); - /* This line may never be reached. */ +} + +static void kunit_try_run_case_cleanup(void *data) +{ + struct kunit_try_catch_context *ctx = data; + struct kunit *test = ctx->test; + struct kunit_suite *suite = ctx->suite; + + current->kunit_test = test; + kunit_run_case_cleanup(test, suite); } +static void kunit_catch_run_case_cleanup(void *data) +{ + struct kunit_try_catch_context *ctx = data; + struct kunit *test = ctx->test; + int try_exit_code = kunit_try_catch_get_result(&test->try_catch); + + /* It is always a failure if cleanup aborts. */ + kunit_set_failure(test); + + if (try_exit_code) { + /* + * Test case could not finish, we have no idea what state it is + * in, so don't do clean up. + */ + if (try_exit_code == -ETIMEDOUT) { + kunit_err(test, "test case cleanup timed out\n"); + /* + * Unknown internal error occurred preventing test case from + * running, so there is nothing to clean up. + */ + } else { + kunit_err(test, "internal error occurred during test case cleanup: %d\n", + try_exit_code); + } + return; + } + + kunit_err(test, "test aborted during cleanup. continuing without cleaning up\n"); +} + + static void kunit_catch_run_case(void *data) { struct kunit_try_catch_context *ctx = data; @@ -448,12 +488,6 @@ static void kunit_catch_run_case(void *data) } return; } - - /* - * Test case was run, but aborted. It is the test case's business as to - * whether it failed or not, we just need to clean up. - */ - kunit_run_case_cleanup(test, suite); } /* @@ -478,6 +512,13 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite, context.test_case = test_case; kunit_try_catch_run(try_catch, &context); + /* Now run the cleanup */ + kunit_try_catch_init(try_catch, + test, + kunit_try_run_case_cleanup, + kunit_catch_run_case_cleanup); + kunit_try_catch_run(try_catch, &context); + /* Propagate the parameter result to the test case. */ if (test->status == KUNIT_FAILURE) test_case->status = KUNIT_FAILURE;
KUnit tests run in a kthread, with the current->kunit_test pointer set to the test's context. This allows the kunit_get_current_test() and kunit_fail_current_test() macros to work. Normally, this pointer is still valid during test shutdown (i.e., the suite->exit function, and any resource cleanup). However, if the test has exited early (e.g., due to a failed assertion), the cleanup is done in the parent KUnit thread, which does not have an active context. Instead, in the event test terminates early, run the test exit and cleanup from a new 'cleanup' kthread, which sets current->kunit_test, and better isolates the rest of KUnit from issues which arise in test cleanup. If a test cleanup function itself aborts (e.g., due to an assertion failing), there will be no further attempts to clean up: an error will be logged and the test failed. For example: # example_simple_test: test aborted during cleanup. continuing without cleaning up This should also make it easier to get access to the KUnit context, particularly from within resource cleanup functions, which may, for example, need access to data in test->priv. Signed-off-by: David Gow <davidgow@google.com> --- This is an updated version of / replacement of "kunit: Set the current KUnit context when cleaning up", which instead creates a new kthread for cleanup tasks if the original test kthread is aborted. This protects us from failed assertions during cleanup, if the test exited early. Changes since v2: https://lore.kernel.org/linux-kselftest/20230419085426.1671703-1-davidgow@google.com/ - Always run cleanup in its own kthread - Therefore, never attempt to re-run it if it exits - Thanks, Benjamin. Changes since v1: https://lore.kernel.org/linux-kselftest/20230415091401.681395-1-davidgow@google.com/ - Move cleanup execution to another kthread - (Thanks, Benjamin, for pointing out the assertion issues) --- lib/kunit/test.c | 55 ++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 7 deletions(-)