Re: [RFC PATCH v2 1/3] kunit: Add kunit_add_action() to defer a call until test exit

[David Gow]

On Fri, 14 Apr 2023 at 18:02, <maxime@xxxxxxxxxx> wrote:
>
> Hi David,
>
> On Fri, Mar 31, 2023 at 04:04:09PM +0800, David Gow wrote:
> > Many uses of the KUnit resource system are intended to simply defer
> > calling a function until the test exits (be it due to success or
> > failure). The existing kunit_alloc_resource() function is often used for
> > this, but was awkward to use (requiring passing NULL init functions, etc),
> > and returned a resource without incrementing its reference count, which
> > -- while okay for this use-case -- could cause problems in others.
> >
> > Instead, introduce a simple kunit_add_action() API: a simple function
> > (returning nothing, accepting a single void* argument) can be scheduled
> > to be called when the test exits. Deferred actions are called in the
> > opposite order to that which they were registered.
> >
> > This mimics the devres API, devm_add_action(), and also provides
> > kunit_remove_action(), to cancel a deferred action, and
> > kunit_release_action() to trigger one early.
> >
> > This is implemented as a resource under the hood, so the ordering
> > between resource cleanup and deferred functions is maintained.
> >
> > Signed-off-by: David Gow <davidgow@xxxxxxxxxx>
> > ---
> >
> > Changes since RFC v1:
> > https://lore.kernel.org/linux-kselftest/20230325043104.3761770-2-davidgow@xxxxxxxxxx/
> > - Rename functions to better match the devm_* APIs. (Thanks Maxime)
> > - Embed the kunit_resource in struct kunit_action_ctx to avoid an extra
> >   allocation (Thanks Benjamin)
> > - Use 'struct kunit_action_ctx' as the type for cancellation tokens
> >   (Thanks Benjamin)
> > - Add tests.
> >
> > ---
> >  include/kunit/resource.h |  89 ++++++++++++++++++++++++++++
> >  lib/kunit/kunit-test.c   | 123 ++++++++++++++++++++++++++++++++++++++-
> >  lib/kunit/resource.c     |  99 +++++++++++++++++++++++++++++++
> >  3 files changed, 310 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/kunit/resource.h b/include/kunit/resource.h
> > index c0d88b318e90..15efd8924666 100644
> > --- a/include/kunit/resource.h
> > +++ b/include/kunit/resource.h
> > @@ -387,4 +387,93 @@ static inline int kunit_destroy_named_resource(struct kunit *test,
> >   */
> >  void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
> >
> > +typedef void (*kunit_defer_function_t)(void *ctx);
> > +
> > +/* An opaque token to a deferred action. */
> > +struct kunit_action_ctx;
> > +
> > +/**
> > + * kunit_add_action() - Defer an 'action' (function call) until the test ends.
> > + * @test: Test case to associate the action with.
> > + * @func: The function to run on test exit
> > + * @ctx: Data passed into @func
> > + * @internal_gfp: gfp to use for internal allocations, if unsure, use GFP_KERNEL
> > + *
> > + * Defer the execution of a function until the test exits, either normally or
> > + * due to a failure.  @ctx is passed as additional context. All functions
> > + * registered with kunit_add_action() will execute in the opposite order to that
> > + * they were registered in.
> > + *
> > + * This is useful for cleaning up allocated memory and resources.
> > + *
> > + * Returns:
> > + *   An opaque "cancellation token", or NULL on error. Pass this token to
> > + *   kunit_remove_action_token() in order to cancel the deferred execution of
> > + *   func().
> > + */
> > +struct kunit_action_ctx *kunit_add_action(struct kunit *test, kunit_defer_function_t func,
> > +                   void *ctx, gfp_t internal_gfp);
>
> I've tried to leverage kunit_add_action() today, and I'm wondering if
> passing the struct kunit pointer to the deferred function would help.
>

I'm tempted, but it does make the case where we just want to cast,
e.g., kfree() directly to an action pointer more difficult. Not that
that's a deal-blocker, but it was convenient...

> The code I'm struggling with is something like:
>
> > static int test_init(struct kunit *test)
> > {
> >       priv = kunit_kzalloc(sizeof(*priv), GFP_KERNEL);
> >       KUNIT_ASSERT_NOT_NULL(test, priv);
> >       test->priv = priv;
> >
> >       priv->dev = alloc_device();
> >
> >       return 0;
> > }
>
> and then in the test itself:
>
> > static void actual_test(struct kunit *test)
> > {
> >       struct test_priv *priv = test->priv;
> >
> >       id = allocate_buffer(priv->dev);
> >
> >       KUNIT_EXPECT_EQ(test, id, 42);
> >
> >       free_buffer(priv->dev, id);
> > }
>
> I'd like to turn free_buffer an action registered right after allocate
> buffer. However, since it takes several arguments and kunit_add_action
> expects a single pointer, we would need to create a structure for it,
> allocate it, fill it, and then free it when the action has ran.

The general case of wanting multiple arguments to an action is a bit
complicated. My plan was to initially support just the one argument,
and deal with more complicated cases later. Ideas included:
- using a struct like you suggest, possibly with some macro magic to
make it easier,
- having a bunch of very similar implementations of
kunit_add_action{2,3,4,..}(), which accept 2,3,4,... arguments,
- something horrible and architecture-specific with manually writing
out arguments to the stack (or registers)

None of those sounded particularly pleasant, though. My suspicion is
that the "right" way of doing this is to maybe have one or two helpers
for common cases (e.g., 2 arguments), and just suggest people create a
structure for anything more complicated, but I'd love a nicer
solution.

>
> It creates a lot of boilerplate, while if we were passing the pointer to
> struct kunit we could access the context of the test as well, and things
> would be much simpler.

For the test context specifically, can you just use kunit_get_current_test()?

There might be an issue with using it during the cleanup process after
a failed assertion (as if the test is aborted early, the cleanup runs
in a different thread), but if so, this should fix it:
---
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index e2910b261112..2d7cad249863 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -392,10 +392,21 @@ static void kunit_case_internal_cleanup(struct
kunit *test)
static void kunit_run_case_cleanup(struct kunit *test,
                                  struct kunit_suite *suite)
{
+       /*
+        * If we're no-longer running from within the test kthread()
because it failed
+        * or timed out, we still need the context to be okay when
running exit and
+        * cleanup functions.
+        */
+       struct kunit *old_current = current->kunit_test;
+
+       current->kunit_test = test;
       if (suite->exit)
               suite->exit(test);

       kunit_case_internal_cleanup(test);
+
+       /* Restore the thread's previous test context (probably NULL
or test). */
+       current->kunit_test = old_current;
}

struct kunit_try_catch_context {
---

I'll look into tidying that up and sending it through next week, anyway.

Cheers,
-- David

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature