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

[Maxime Ripard]

On Fri, Apr 14, 2023 at 01:00:26PM +0200, Benjamin Berg wrote:
> Hi,
> 
> On Fri, 2023-04-14 at 12:01 +0200, 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.
> > 
> > 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.
> > 
> > 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.
> 
> The question seems to be what about the typical use-case. I was always
> imagining calling functions like kfree/kfree_skb which often only
> require a single argument.

I guess we can have a look at the devm stuff. I'd expect the scope of
things that will eventually tie their resource to kunit would be
similar. "Straight" allocation/deallocation functions are the obvious
first candidates, but there's a lot of other use cases as well.

I guess my main point is that it assumes that most function to clean
things up will take the resource as its only argument, which isn't
always the case. I guess it's reasonable to optimize for the most
trivial case, but we should strive to keep the boilerplate down as much
as we can in the other case too.

> For arbitrary arguments, a struct and custom free function will be
> needed. At that point, maybe it is fair to assume that API users will
> use the resource API directly, doing the same trick as kunit_add_action
> and storing the arguments together with struct kunit_resource.

kunit_add_resource adds tons of boilerplate as well:

struct test_buffer_priv {
	struct device *dev;
}

struct test_alloc_params {
	struct device *dev;
	void *buffer;
}

static int __alloc_buffer(struct kunit_resource *res, void *ptr)
{
	struct test_alloc_params *params = ptr;
	void *buffer;

	params->buffer = allocate_buffer(params->dev, params->size);
	res->data = params;

	return 0;
}

static void __free_buffer(struct kunit_resource *res)
{
	struct test_alloc_params *params = res->data;

	free_buffer(params->dev, params->buffer);
}

void actual_test(struct kunit_test *test)
{
	struct test_alloc_params *params = test->priv;

	kunit_alloc_resource(test, __alloc_buffer, __free_buffer, GFP_KERNEL, params);
	KUNIT_EXPECT_NOT_NULL(params->buffer);
}

int test_init(struct kunit_test *test)
{
	struct test_alloc_params *params =
		kunit_kmalloc(test, sizeof(*params), GFP_KERNEL);

	test->priv = params;

	params->dev = kunit_allocate_device(...);

	return 0;
}


while we could have something like:


struct test_buffer_priv {
	struct device *dev;
}

static void free_buffer_action(struct kunit *test, void *ptr)
{
	struct test_buffer_priv *priv = test->priv;

	free_buffer(params->dev, ptr);
}

void actual_test(struct kunit_test *test)
{
	struct test_buffer_priv *priv = test->priv;
	void *buffer;

	buffer = allocate_buffer(priv->dev, 42);
	kunit_add_action(test, free_buffer_action, buffer);

	KUNIT_EXPECT_NOT_NULL(buffer);
}

int test_init(struct kunit_test *test)
{
	struct test_buffer_priv *priv =
		kunit_kmalloc(test, sizeof(*priv), GFP_KERNEL);

	test->priv = priv;

	priv->dev = kunit_allocate_device(...);

	return 0;
}

Which is much more compact, more readable, and less error prone.

And sure, kfree and free_skb would need some intermediate function, but
it's like 3 more lines, which can even be shared at the framework or
kunit level, so shouldn't really impact the tests themselves.

> That said, maybe one could add it as a second argument? It is a little
> bit weird API wise, but it would allow simply casting single-argument
> functions in order to ignore "struct kunit *" argument. 

I guess. I'd find it a bit weird to have that one function with the
argument order reversed compared to everything else though.

Maxime

Attachment: signature.asc
Description: PGP signature