Message-ID: <6478bd117b17e004df371bd84342c2e378adf566.camel@sipsolutions.net>
Date:   Fri, 21 Apr 2023 10:52:06 +0200
From:   Benjamin Berg <benjamin@...solutions.net>
To:     David Gow <davidgow@...gle.com>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Rae Moar <rmoar@...gle.com>,
        Daniel Latypov <dlatypov@...gle.com>
Cc:     maxime@...no.tech, Stephen Boyd <sboyd@...nel.org>,
        kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org, Sadiya Kazi <sadiyakazi@...gle.com>
Subject: Re: [PATCH v3 1/4] kunit: Always run cleanup from a test kthread

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@...gle.com>

Great! Looks good to me.

Reviewed-by: Benjamin Berg <benjamin.berg@...el.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;

Powered by blists - more mailing lists