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
