[PATCH v1 0/3] kunit: Deferred action helpers

[David Gow]

This is v1 of the KUnit deferred actions API, which implements an
equivalent of devm_add_action[1] on top of KUnit managed resources. This
provides a simple way of scheduling a function to run when the test
terminates (whether successfully, or with an error). It's therefore very
useful for freeing resources, or otherwise cleaning up.

The notable changes since RFCv2[2] are:
- Got rid of the 'cancellation token' concept. It was overcomplicated,
and we can add it back if we need to.
- kunit_add_action() therefore now returns 0 on success, and an error
otherwise (like devm_add_action()). Though you may wish to use:
- Added kunit_add_action_or_reset(), which will call the deferred
function if an error occurs. (See devm_add_action_or_reset()). This
also returns an error on failure, which can be asserted safely.
- Got rid of the function pointer typedef. Personally, I liked it, but
it's more typedef-y than most kernel code.
- Got rid of the 'internal_gfp' argument: all internal state is now
allocated with GFP_KERNEL. The main KUnit resource API can be used
instead if this doesn't work for your use-case.

I'd love to hear any further thoughts!

Cheers,
-- David

[1]: https://docs.kernel.org/driver-api/basics.html#c.devm_add_action
[2]: https://patchwork.kernel.org/project/linux-kselftest/list/?series=735720


David Gow (3):
kunit: Add kunit_add_action() to defer a call until test exit
kunit: executor_test: Use kunit_add_action()
kunit: kmalloc_array: Use kunit_add_action()

include/kunit/resource.h | 76 +++++++++++++++++++++++++++++++
include/kunit/test.h | 10 ++++-
lib/kunit/executor_test.c | 11 ++---
lib/kunit/kunit-test.c | 88 +++++++++++++++++++++++++++++++++++-
lib/kunit/resource.c | 95 +++++++++++++++++++++++++++++++++++++++
lib/kunit/test.c | 48 ++++----------------
6 files changed, 279 insertions(+), 49 deletions(-)

--
2.40.0.634.g4ca3ef3211-goog

[David Gow]

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 <davi...@google.com>
---

Note: there is a checkpatch warning:
"function definition argument 'void *' should also have an identifier name"

I think this is a false-positive, as we're defining a function pointer
struct member, not a specific function.

Changes since RFC v2:
https://lore.kernel.org/linux-kselftest/20230331080411.9...@google.com/
- Got rid of internal_gfp
- everything uses GFP_KERNEL now
- This includes kunit_kzalloc() and friends, which still allocate the
returned memory with the provided GFP, but use GFP_KERNEL for
internal bookkeeping data.
- Thanks Maxime & Benjamin!
- Got rid of cancellation tokens.
- kunit_add_action() now returns 0 on success, otherwise an error
- Note that this can quite easily lead to a memory leak, so look at
kunit_add_action_or_reset()
- Thanks Maxime & Benjamin!
- Added kunit_add_action_or_reset
- Matches devm_add_action_or_reset()
- Returns 0 on success.
- Thanks Maxime & Benjamin!
- Got rid of the kunit_defer_func_t typedef.
- I liked it, but it is probably pushing the boundaries of kernel
style.
- Use (void (*)(void *)) instead.

Changes since RFC v1:
https://lore.kernel.org/linux-kselftest/20230325043104.3...@google.com/
- 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 | 76 ++++++++++++++++++++++++++++++++
lib/kunit/kunit-test.c | 88 ++++++++++++++++++++++++++++++++++++-
lib/kunit/resource.c | 95 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 258 insertions(+), 1 deletion(-)

diff --git a/include/kunit/resource.h b/include/kunit/resource.h
index c0d88b318e90..6db28cd43e9b 100644
--- a/include/kunit/resource.h
+++ b/include/kunit/resource.h
@@ -387,4 +387,80 @@ static inline int kunit_destroy_named_resource(struct kunit *test,
*/
void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);

+
+/**
+ * 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
+ *
+ * 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:
+ * 0 on success, an error if the action could not be deferred.
+ */
+int kunit_add_action(struct kunit *test, void (*action) (void *), void *ctx);
+
+/**
+ * kunit_add_action_or_reset() - 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
+ *
+ * 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.
+ *
+ * If the action cannot be created (e.g., due to the system being out of memory),
+ * then action(ctx) will be called immediately, and an error will be returned.
+ *
+ * Returns:
+ * 0 on success, an error if the action could not be deferred.
+ */
+int kunit_add_action_or_reset(struct kunit *test, void (*action)(void *),
+ void *ctx);
+
+/**
+ * kunit_remove_action() - Cancel a matching deferred action.
+ * @test: Test case the action is associated with.
+ * @func: The deferred function to cancel.
+ * @ctx: The context passed to the deferred function to trigger.
+ *
+ * Prevent an action deferred via kunit_add_action() from executing when the
+ * test terminates..
+ * If the function/context pair was deferred multiple times, only the most
+ * recent one will be cancelled.
+ */
+void kunit_remove_action(struct kunit *test,
+ void (*action)(void *),
+ void *ctx);
+
+/**
+ * kunit_release_action() - Run a matching action call immediately.
+ * @test: Test case the action is associated with.
+ * @func: The deferred function to trigger.
+ * @ctx: The context passed to the deferred function to trigger.
+ *
+ * Execute a function deferred via kunit_add_action()) immediately, rather than
+ * when the test ends.
+ * If the function/context pair was deferred multiple times, it will only be
+ * executed once here. The most recent deferral will no longer execute when
+ * the test ends.
+ *
+ * kunit_release_action(test, func, ctx);
+ * is equivalent to
+ * func(ctx);
+ * kunit_remove_action(test, func, ctx);
+ */
+void kunit_release_action(struct kunit *test,
+ void (*action)(void *),
+ void *ctx);
#endif /* _KUNIT_RESOURCE_H */
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index 42e44caa1bdd..83d8e90ca7a2 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c
@@ -112,7 +112,7 @@ struct kunit_test_resource_context {
struct kunit test;
bool is_resource_initialized;
int allocate_order[2];
- int free_order[2];
+ int free_order[4];
};

static int fake_resource_init(struct kunit_resource *res, void *context)
@@ -403,6 +403,88 @@ static void kunit_resource_test_named(struct kunit *test)
KUNIT_EXPECT_TRUE(test, list_empty(&test->resources));
}

+static void increment_int(void *ctx)
+{
+ int *i = (int *)ctx;
+ (*i)++;
+}
+
+static void kunit_resource_test_action(struct kunit *test)
+{
+ int num_actions = 0;
+
+ kunit_add_action(test, increment_int, &num_actions);
+ KUNIT_EXPECT_EQ(test, num_actions, 0);
+ kunit_cleanup(test);
+ KUNIT_EXPECT_EQ(test, num_actions, 1);
+
+ /* Once we've cleaned up, the action queue is empty. */
+ kunit_cleanup(test);
+ KUNIT_EXPECT_EQ(test, num_actions, 1);
+
+ /* Check the same function can be deferred multiple times. */
+ kunit_add_action(test, increment_int, &num_actions);
+ kunit_add_action(test, increment_int, &num_actions);
+ kunit_cleanup(test);
+ KUNIT_EXPECT_EQ(test, num_actions, 3);
+}
+static void kunit_resource_test_remove_action(struct kunit *test)
+{
+ int num_actions = 0;
+
+ kunit_add_action(test, increment_int, &num_actions);
+ KUNIT_EXPECT_EQ(test, num_actions, 0);
+
+ kunit_remove_action(test, increment_int, &num_actions);
+ kunit_cleanup(test);
+ KUNIT_EXPECT_EQ(test, num_actions, 0);
+}
+static void kunit_resource_test_release_action(struct kunit *test)
+{
+ int num_actions = 0;
+
+ kunit_add_action(test, increment_int, &num_actions);
+ KUNIT_EXPECT_EQ(test, num_actions, 0);
+ /* Runs immediately on trigger. */
+ kunit_release_action(test, increment_int, &num_actions);
+ KUNIT_EXPECT_EQ(test, num_actions, 1);
+
+ /* Doesn't run again on test exit. */
+ kunit_cleanup(test);
+ KUNIT_EXPECT_EQ(test, num_actions, 1);
+}
+static void action_order_1(void *ctx)
+{
+ struct kunit_test_resource_context *res_ctx = (struct kunit_test_resource_context *)ctx;
+
+ KUNIT_RESOURCE_TEST_MARK_ORDER(res_ctx, free_order, 1);
+ kunit_log(KERN_INFO, current->kunit_test, "action_order_1");
+}
+static void action_order_2(void *ctx)
+{
+ struct kunit_test_resource_context *res_ctx = (struct kunit_test_resource_context *)ctx;
+
+ KUNIT_RESOURCE_TEST_MARK_ORDER(res_ctx, free_order, 2);
+ kunit_log(KERN_INFO, current->kunit_test, "action_order_2");
+}
+static void kunit_resource_test_action_ordering(struct kunit *test)
+{
+ struct kunit_test_resource_context *ctx = test->priv;
+
+ kunit_add_action(test, action_order_1, ctx);
+ kunit_add_action(test, action_order_2, ctx);
+ kunit_add_action(test, action_order_1, ctx);
+ kunit_add_action(test, action_order_2, ctx);
+ kunit_remove_action(test, action_order_1, ctx);
+ kunit_release_action(test, action_order_2, ctx);
+ kunit_cleanup(test);
+
+ /* [2 is triggered] [2], [(1 is cancelled)] [1] */
+ KUNIT_EXPECT_EQ(test, ctx->free_order[0], 2);
+ KUNIT_EXPECT_EQ(test, ctx->free_order[1], 2);
+ KUNIT_EXPECT_EQ(test, ctx->free_order[2], 1);
+}
+
static int kunit_resource_test_init(struct kunit *test)
{
struct kunit_test_resource_context *ctx =
@@ -434,6 +516,10 @@ static struct kunit_case kunit_resource_test_cases[] = {
KUNIT_CASE(kunit_resource_test_proper_free_ordering),
KUNIT_CASE(kunit_resource_test_static),
KUNIT_CASE(kunit_resource_test_named),
+ KUNIT_CASE(kunit_resource_test_action),
+ KUNIT_CASE(kunit_resource_test_remove_action),
+ KUNIT_CASE(kunit_resource_test_release_action),
+ KUNIT_CASE(kunit_resource_test_action_ordering),
{}
};

diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c
index c414df922f34..20b08b9341c9 100644
--- a/lib/kunit/resource.c
+++ b/lib/kunit/resource.c
@@ -77,3 +77,98 @@ int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match,
return 0;
}
EXPORT_SYMBOL_GPL(kunit_destroy_resource);
+
+struct kunit_action_ctx {
+ struct kunit_resource res;
+ void (*func)(void *);
+ void *ctx;
+};
+
+static void __kunit_action_free(struct kunit_resource *res)
+{
+ struct kunit_action_ctx *action_ctx = container_of(res, struct kunit_action_ctx, res);
+
+ action_ctx->func(action_ctx->ctx);
+}
+
+
+int kunit_add_action(struct kunit *test, void (*action)(void *), void *ctx)
+{
+ struct kunit_action_ctx *action_ctx;
+
+ KUNIT_ASSERT_NOT_NULL_MSG(test, action, "Tried to action a NULL function!");
+
+ action_ctx = kzalloc(sizeof(*action_ctx), GFP_KERNEL);
+ if (!action_ctx)
+ return -ENOMEM;
+
+ action_ctx->func = action;
+ action_ctx->ctx = ctx;
+
+ action_ctx->res.should_kfree = true;
+ /* As init is NULL, this cannot fail. */
+ __kunit_add_resource(test, NULL, __kunit_action_free, &action_ctx->res, action_ctx);
+
+ return 0;
+}
+
+int kunit_add_action_or_reset(struct kunit *test, void (*action)(void *),
+ void *ctx)
+{
+ int res = kunit_add_action(test, action, ctx);
+
+ if (res)
+ action(ctx);
+ return res;
+}
+
+static bool __kunit_action_match(struct kunit *test,
+ struct kunit_resource *res, void *match_data)
+{
+ struct kunit_action_ctx *match_ctx = (struct kunit_action_ctx *)match_data;
+ struct kunit_action_ctx *res_ctx = container_of(res, struct kunit_action_ctx, res);
+
+ /* Make sure this is a free function. */
+ if (res->free != __kunit_action_free)
+ return false;
+
+ /* Both the function and context data should match. */
+ return (match_ctx->func == res_ctx->func) && (match_ctx->ctx == res_ctx->ctx);
+}
+
+void kunit_remove_action(struct kunit *test,
+ void (*action)(void *),
+ void *ctx)
+{
+ struct kunit_action_ctx match_ctx;
+ struct kunit_resource *res;
+
+ match_ctx.func = action;
+ match_ctx.ctx = ctx;
+
+ res = kunit_find_resource(test, __kunit_action_match, &match_ctx);
+ if (res) {
+ /* Remove the free function so we don't run the action. */
+ res->free = NULL;
+ kunit_remove_resource(test, res);
+ kunit_put_resource(res);
+ }
+}
+
+void kunit_release_action(struct kunit *test,
+ void (*action)(void *),
+ void *ctx)
+{
+ struct kunit_action_ctx match_ctx;
+ struct kunit_resource *res;
+
+ match_ctx.func = action;
+ match_ctx.ctx = ctx;
+
+ res = kunit_find_resource(test, __kunit_action_match, &match_ctx);
+ if (res) {
+ kunit_remove_resource(test, res);
+ /* We have to put() this here, else free won't be called. */
+ kunit_put_resource(res);
+ }
+}
--
2.40.0.634.g4ca3ef3211-goog

[David Gow]

Now we have the kunit_add_action() function, we can use it to implement
kfree_at_end() and free_subsuite_at_end() without the need for extra
helper functions.

Signed-off-by: David Gow <davi...@google.com>
---

Changes since RFCv2:
https://lore.kernel.org/linux-kselftest/20230331080411.9...@google.com/
- Don't use the no-longer-extant kunit_defer_func_t typedef.
- Don't pass a GFP pointer in.

---
lib/kunit/executor_test.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
index 0cea31c27b23..fd3ba4d74a66 100644
--- a/lib/kunit/executor_test.c
+++ b/lib/kunit/executor_test.c
@@ -125,11 +125,6 @@ kunit_test_suites(&executor_test_suite);

/* Test helpers */

-static void kfree_res_free(struct kunit_resource *res)
-{
- kfree(res->data);
-}
-
/* Use the resource API to register a call to kfree(to_free).
* Since we never actually use the resource, it's safe to use on const data.
*/
@@ -138,8 +133,10 @@ static void kfree_at_end(struct kunit *test, const void *to_free)
/* kfree() handles NULL already, but avoid allocating a no-op cleanup. */
if (IS_ERR_OR_NULL(to_free))
return;
- kunit_alloc_resource(test, NULL, kfree_res_free, GFP_KERNEL,
- (void *)to_free);
+
+ kunit_add_action(test,
+ (void (*)(void *))kfree,
+ (void *)to_free);
}

static struct kunit_suite *alloc_fake_suite(struct kunit *test,
--
2.40.0.634.g4ca3ef3211-goog

[David Gow]

The kunit_add_action() function is much simpler and cleaner to use that
the full KUnit resource API for simple things like the
kunit_kmalloc_array() functionality.

Replacing it allows us to get rid of a number of helper functions, and
leaves us with no uses of kunit_alloc_resource(), which has some
usability problems and is going to have its behaviour modified in an
upcoming patch.

Note that we need to use kunit_release_action() to implement kunit_kfree().

Signed-off-by: David Gow <davi...@google.com>
---

Changes since RFCv2:
https://lore.kernel.org/linux-kselftest/20230331080411.9...@google.com/

- Update to match changes in the the action API.
- Always allocate the action context with GFP_KERNEL.
- Update documentation to note that this will cause GFP_KERNEL
allocations, regardless of the gfp argument passed in.

---
include/kunit/test.h | 10 +++++++--
lib/kunit/test.c | 48 +++++++++-----------------------------------
2 files changed, 17 insertions(+), 41 deletions(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 57b309c6ca27..3e8e98d0d8b1 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -321,8 +321,11 @@ enum kunit_status kunit_suite_has_succeeded(struct kunit_suite *suite);
* @gfp: flags passed to underlying kmalloc().
*
* Just like `kmalloc_array(...)`, except the allocation is managed by the test case
- * and is automatically cleaned up after the test case concludes. See &struct
- * kunit_resource for more information.
+ * and is automatically cleaned up after the test case concludes. See kunit_add_action()
+ * for more information.
+ *
+ * Note that some internal context data is also allocated with GFP_KERNEL,
+ * regardless of the gfp passed in.
*/
void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t gfp);

@@ -333,6 +336,9 @@ void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t gfp);
* @gfp: flags passed to underlying kmalloc().
*
* See kmalloc() and kunit_kmalloc_array() for more information.
+ *
+ * Note that some internal context data is also allocated with GFP_KERNEL,
+ * regardless of the gfp passed in.
*/
static inline void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp)
{
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index e2910b261112..6aafe2138766 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -712,58 +712,28 @@ static struct notifier_block kunit_mod_nb = {
};
#endif

-struct kunit_kmalloc_array_params {
- size_t n;
- size_t size;
- gfp_t gfp;
-};
-
-static int kunit_kmalloc_array_init(struct kunit_resource *res, void *context)
+void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t gfp)
{
- struct kunit_kmalloc_array_params *params = context;
+ void *data;

- res->data = kmalloc_array(params->n, params->size, params->gfp);
- if (!res->data)
- return -ENOMEM;
+ data = kmalloc_array(n, size, gfp);

- return 0;
-}
+ if (!data)
+ return NULL;

-static void kunit_kmalloc_array_free(struct kunit_resource *res)

-{
- kfree(res->data);
-}

+ if (kunit_add_action_or_reset(test, (void (*)(void *))kfree, data) != 0)
+ return NULL;

-void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t gfp)
-{
- struct kunit_kmalloc_array_params params = {
- .size = size,
- .n = n,
- .gfp = gfp
- };
-
- return kunit_alloc_resource(test,
- kunit_kmalloc_array_init,
- kunit_kmalloc_array_free,
- gfp,
- &params);
+ return data;
}
EXPORT_SYMBOL_GPL(kunit_kmalloc_array);

-static inline bool kunit_kfree_match(struct kunit *test,
- struct kunit_resource *res, void *match_data)
-{
- /* Only match resources allocated with kunit_kmalloc() and friends. */
- return res->free == kunit_kmalloc_array_free && res->data == match_data;
-}
-
void kunit_kfree(struct kunit *test, const void *ptr)
{
if (!ptr)
return;

- if (kunit_destroy_resource(test, kunit_kfree_match, (void *)ptr))
- KUNIT_FAIL(test, "kunit_kfree: %px already freed or not allocated by kunit", ptr);
+ kunit_release_action(test, (void (*)(void *))kfree, (void *)ptr);
}
EXPORT_SYMBOL_GPL(kunit_kfree);

--
2.40.0.634.g4ca3ef3211-goog

[Benjamin Berg]

On Fri, 2023-04-21 at 16:42 +0800, 'David Gow' via KUnit Development wrote:
> This is v1 of the KUnit deferred actions API, which implements an
> equivalent of devm_add_action[1] on top of KUnit managed resources. This
> provides a simple way of scheduling a function to run when the test
> terminates (whether successfully, or with an error). It's therefore very
> useful for freeing resources, or otherwise cleaning up.
>
> The notable changes since RFCv2[2] are:
> - Got rid of the 'cancellation token' concept. It was overcomplicated,
>   and we can add it back if we need to.
> - kunit_add_action() therefore now returns 0 on success, and an error
>   otherwise (like devm_add_action()). Though you may wish to use:
> - Added kunit_add_action_or_reset(), which will call the deferred
>   function if an error occurs. (See devm_add_action_or_reset()). This
>   also returns an error on failure, which can be asserted safely.
> - Got rid of the function pointer typedef. Personally, I liked it, but
>   it's more typedef-y than most kernel code.
> - Got rid of the 'internal_gfp' argument: all internal state is now
>   allocated with GFP_KERNEL. The main KUnit resource API can be used
>   instead if this doesn't work for your use-case.
>
> I'd love to hear any further thoughts!

I am happy with it as-is.

Reviewed-By: Benjamin Berg <benjam...@intel.com>

[Benjamin Berg]

Hi David,

On Mon, 2023-04-24 at 14:32 +0200, Benjamin Berg wrote:
> On Fri, 2023-04-21 at 16:42 +0800, 'David Gow' via KUnit Development wrote:
> > This is v1 of the KUnit deferred actions API, which implements an
> > equivalent of devm_add_action[1] on top of KUnit managed resources. This
> > provides a simple way of scheduling a function to run when the test
> > terminates (whether successfully, or with an error). It's therefore very
> > useful for freeing resources, or otherwise cleaning up.
> >
> > The notable changes since RFCv2[2] are:
> > - Got rid of the 'cancellation token' concept. It was overcomplicated,
> >   and we can add it back if we need to.
> > - kunit_add_action() therefore now returns 0 on success, and an error
> >   otherwise (like devm_add_action()). Though you may wish to use:
> > - Added kunit_add_action_or_reset(), which will call the deferred
> >   function if an error occurs. (See devm_add_action_or_reset()). This
> >   also returns an error on failure, which can be asserted safely.
> > - Got rid of the function pointer typedef. Personally, I liked it, but
> >   it's more typedef-y than most kernel code.
> > - Got rid of the 'internal_gfp' argument: all internal state is now
> >   allocated with GFP_KERNEL. The main KUnit resource API can be used
> >   instead if this doesn't work for your use-case.
> >
> > I'd love to hear any further thoughts!
>
> I am happy with it as-is.

Oh, wait. Nothing big, but I just noticed that the new API functions
seem to not yet be exported using EXPORT_SYMBOL_GPL.

Benjamin

[Maxime Ripard]

Hi,

On Fri, Apr 21, 2023 at 04:42:23PM +0800, David Gow wrote:
> This is v1 of the KUnit deferred actions API, which implements an
> equivalent of devm_add_action[1] on top of KUnit managed resources. This
> provides a simple way of scheduling a function to run when the test
> terminates (whether successfully, or with an error). It's therefore very
> useful for freeing resources, or otherwise cleaning up.
>
> The notable changes since RFCv2[2] are:
> - Got rid of the 'cancellation token' concept. It was overcomplicated,
> and we can add it back if we need to.
> - kunit_add_action() therefore now returns 0 on success, and an error
> otherwise (like devm_add_action()). Though you may wish to use:
> - Added kunit_add_action_or_reset(), which will call the deferred
> function if an error occurs. (See devm_add_action_or_reset()). This
> also returns an error on failure, which can be asserted safely.
> - Got rid of the function pointer typedef. Personally, I liked it, but
> it's more typedef-y than most kernel code.
> - Got rid of the 'internal_gfp' argument: all internal state is now
> allocated with GFP_KERNEL. The main KUnit resource API can be used
> instead if this doesn't work for your use-case.
>
> I'd love to hear any further thoughts!

I've converted the KMS kunit tests to use that API when relevant, and
it works like a charm and is super usable, thanks so much.

One improvement we could do as a second step is to provide a
kunit_action_t type or something to make casting kfree-like functions
easier, but it's already great overall.

Reviewed-by: Maxime Ripard <max...@cerno.tech>
Tested-by: Maxime Ripard <max...@cerno.tech>

Thanks again,
Maxime

[Daniel Latypov]

On Fri, Apr 21, 2023 at 1:42 AM David Gow <davi...@google.com> 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.

Apologies for the delayed bikeshedding.

I think mimicking the devres API is a better idea than kunit_defer()
and friends.
But I can't help but think this still isn't the best name.
I personally would have no idea what `kunit_release_action()` does
without looking it up.

I feel like `kunit_add_cleanup()` probably works better for a unit
test framework.
I think `kunit_remove_cleanup()` is fine and `kunit_release_cleanup()`
is questionably ok.
Instead of `release`, maybe it should be `kunit_trigger_cleanup()` or
more verbosely, something like `kunit_early_trigger_cleanup()`.

I tried to look for equivalents in other languages/frameworks:
* Rust and C++ rely on RAII, don't think they have equivalents in testing libs
* Python has `self.addCleanup()`,
https://docs.python.org/3/library/unittest.html#unittest.TestCase.addCleanup
* Go has `t.Cleanup()`, https://pkg.go.dev/testing#T.Cleanup
* Looking at Zig since it also has a `defer`, I guess they just use
that, I don't see anything in
https://ziglang.org/documentation/master/std/#A;std:testing
* I know nothing about JUnit, but a quick search seems like they rely
on @After and @AfterClass annotations,
https://junit.org/junit4/javadoc/4.12/org/junit/After.html
* I know even less about HUnit, but it looks like it relies on
wrapping things via the IO monad,
https://hackage.haskell.org/package/HUnit-1.6.2.0/docs/Test-HUnit-Base.html#t:AssertionPredicate
* Since we were inspired by TAP, I tried to look at Perl, but didn't
immediately see anything that looked equivalent,
https://metacpan.org/pod/Test::Most

>
> 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 <davi...@google.com>
> ---

<snip>

> diff --git a/include/kunit/resource.h b/include/kunit/resource.h
> index c0d88b318e90..6db28cd43e9b 100644
> --- a/include/kunit/resource.h
> +++ b/include/kunit/resource.h
> @@ -387,4 +387,80 @@ static inline int kunit_destroy_named_resource(struct kunit *test,
> */
> void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
>
> +
> +/**
> + * 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
> + *
> + * 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.

Re renaming to kunit_add_cleanup(), I think this makes writing the
comment easier.

E.g.
- kunit_add_action() - Defer an 'action' (function call) until the test ends.
+ kunit_add_cleanup() - Call a function when the test ends.
+ ...
+ This is useful for cleaning up allocated memory and resources.

Daniel

[David Gow]

Ah, nice catch! I'll add those to the next version.

Cheers,
-- David

[David Gow]

On Tue, 25 Apr 2023 at 23:23, Maxime Ripard <max...@cerno.tech> wrote:
>
> Hi,
>
> On Fri, Apr 21, 2023 at 04:42:23PM +0800, David Gow wrote:
> > This is v1 of the KUnit deferred actions API, which implements an
> > equivalent of devm_add_action[1] on top of KUnit managed resources. This
> > provides a simple way of scheduling a function to run when the test
> > terminates (whether successfully, or with an error). It's therefore very
> > useful for freeing resources, or otherwise cleaning up.
> >
> > The notable changes since RFCv2[2] are:
> > - Got rid of the 'cancellation token' concept. It was overcomplicated,
> > and we can add it back if we need to.
> > - kunit_add_action() therefore now returns 0 on success, and an error
> > otherwise (like devm_add_action()). Though you may wish to use:
> > - Added kunit_add_action_or_reset(), which will call the deferred
> > function if an error occurs. (See devm_add_action_or_reset()). This
> > also returns an error on failure, which can be asserted safely.
> > - Got rid of the function pointer typedef. Personally, I liked it, but
> > it's more typedef-y than most kernel code.
> > - Got rid of the 'internal_gfp' argument: all internal state is now
> > allocated with GFP_KERNEL. The main KUnit resource API can be used
> > instead if this doesn't work for your use-case.
> >
> > I'd love to hear any further thoughts!
>
> I've converted the KMS kunit tests to use that API when relevant, and
> it works like a charm and is super usable, thanks so much.

Nice! I'm glad it's working well.

>
> One improvement we could do as a second step is to provide a
> kunit_action_t type or something to make casting kfree-like functions
> easier, but it's already great overall.

I had that in an earlier version and got rid of it to better match
what devm_* was doing, but I personally agree that it's nice to have.
I'll add it back in the next version.

> Reviewed-by: Maxime Ripard <max...@cerno.tech>
> Tested-by: Maxime Ripard <max...@cerno.tech>

Cheers,
-- David

[David Gow]

Hmm... While personally I prefer 'defer' or 'cleanup' to 'action' in
isolation, I think the benefits of matching the devm_ API probably
exceed the benefits of a slightly better name here.

I'm less convinced by the _release_action() and _remove_action()
names: I definitely think 'trigger' is more obvious here. I hope that,
with some extra documentation, we can nevertheless make this
consistent with devm_*, but it's definitely suboptimal.

>
> I tried to look for equivalents in other languages/frameworks:
> * Rust and C++ rely on RAII, don't think they have equivalents in testing libs
> * Python has `self.addCleanup()`,
> https://docs.python.org/3/library/unittest.html#unittest.TestCase.addCleanup
> * Go has `t.Cleanup()`, https://pkg.go.dev/testing#T.Cleanup
> * Looking at Zig since it also has a `defer`, I guess they just use
> that, I don't see anything in
> https://ziglang.org/documentation/master/std/#A;std:testing
> * I know nothing about JUnit, but a quick search seems like they rely
> on @After and @AfterClass annotations,
> https://junit.org/junit4/javadoc/4.12/org/junit/After.html
> * I know even less about HUnit, but it looks like it relies on
> wrapping things via the IO monad,
> https://hackage.haskell.org/package/HUnit-1.6.2.0/docs/Test-HUnit-Base.html#t:AssertionPredicate
> * Since we were inspired by TAP, I tried to look at Perl, but didn't
> immediately see anything that looked equivalent,
> https://metacpan.org/pod/Test::Most
>

Thanks for putting that together. It looks like cleanup is the winner
here, followed maybe by defer. I'd been using 'cleanup' to refer to
the sum total of all deferred functions, resource free functions, and
the test 'exit()' function (i.e., everything which runs after a failed
assertion), so I don't want to totally confuse the issue.

Regardless, it's probably worth at least having a mention in the
documentation that these are referred to as a cleanup in
Python/Go/etc, and are vaguely equivalent to 'defer' in Go and Zig.

Good point. I think we can probably use the better description here
even if we don't rename the function.

Cheers,
-- David

> Daniel