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

[Maxime Ripard]

Hi David,

On Sat, Apr 15, 2023 at 04:42:27PM +0800, David Gow wrote:
> 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()?

I wasn't aware that it was a solution, but it looks like a good compromise :)

Maxime

Attachment: signature.asc
Description: PGP signature