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
