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

[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.

Reviewed-by: Benjamin Berg <benjam...@intel.com>
Reviewed-by: Maxime Ripard <max...@cerno.tech>
Tested-by: Maxime Ripard <max...@cerno.tech>
Signed-off-by: David Gow <davi...@google.com>
---

Changes since v1:
https://lore.kernel.org/linux-kselftest/20230421084226.2...@google.com/
- Some small documentation updates (Thanks Daniel)
- Reinstate a typedef for the action function.
- This time, it's called kunit_action_t
- Thanks Maxime!

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 | 92 +++++++++++++++++++++++++++++++++++++
lib/kunit/kunit-test.c | 88 ++++++++++++++++++++++++++++++++++-
lib/kunit/resource.c | 99 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 278 insertions(+), 1 deletion(-)

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

+/* A 'deferred action' function to be used with kunit_add_action. */
+typedef void (kunit_action_t)(void *);
+
+/**
+ * kunit_add_action() - Call a function when 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, as these
+ * functions are called even if the test aborts early due to, e.g., a failed
+ * assertion.
+ *
+ * See also: devm_add_action() for the devres equivalent.
+ *
+ * Returns:
+ * 0 on success, an error if the action could not be deferred.
+ */
+int kunit_add_action(struct kunit *test, kunit_action_t *action, void *ctx);
+
+/**
+ * kunit_add_action_or_reset() - Call a function when 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, as these
+ * functions are called even if the test aborts early due to, e.g., a failed
+ * assertion.
+ *
+ * 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.
+ *
+ * See also: devm_add_action_or_reset() for the devres equivalent.
+ *
+ * Returns:
+ * 0 on success, an error if the action could not be deferred.
+ */
+int kunit_add_action_or_reset(struct kunit *test, kunit_action_t *action,
+ 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.
+ *
+ * See also: devm_remove_action() for the devres equivalent.
+ */
+void kunit_remove_action(struct kunit *test,
+ kunit_action_t *action,
+ 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);
+ *
+ * See also: devm_release_action() for the devres equivalent.
+ */
+void kunit_release_action(struct kunit *test,
+ kunit_action_t *action,
+ 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..f0209252b179 100644
--- a/lib/kunit/resource.c
+++ b/lib/kunit/resource.c
@@ -77,3 +77,102 @@ 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;
+ kunit_action_t *func;
+ 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;
+}
+EXPORT_SYMBOL_GPL(kunit_add_action);
+
+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;
+}
+EXPORT_SYMBOL_GPL(kunit_add_action_or_reset);
+
+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,
+ kunit_action_t *action,
+ 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);
+ }
+}
+EXPORT_SYMBOL_GPL(kunit_remove_action);
+
+void kunit_release_action(struct kunit *test,
+ kunit_action_t *action,
+ 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);
+ }
+}
+EXPORT_SYMBOL_GPL(kunit_release_action);
--
2.40.1.698.g37aff9b760-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.

Reviewed-by: Benjamin Berg <benjam...@intel.com>
Reviewed-by: Maxime Ripard <max...@cerno.tech>
Tested-by: Maxime Ripard <max...@cerno.tech>
Signed-off-by: David Gow <davi...@google.com>
---

Changes since v1:
https://lore.kernel.org/linux-kselftest/20230421084226.2...@google.com/

- Use the kunit_action_t typedef

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..ce6749af374d 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,
+ (kunit_action_t *)kfree,
+ (void *)to_free);
}

static struct kunit_suite *alloc_fake_suite(struct kunit *test,
--
2.40.1.698.g37aff9b760-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_defer_trigger_all() to implement
kunit_kfree().

Reviewed-by: Benjamin Berg <benjam...@intel.com>
Reviewed-by: Maxime Ripard <max...@cerno.tech>
Tested-by: Maxime Ripard <max...@cerno.tech>
Signed-off-by: David Gow <davi...@google.com>
---

Changes since v1:
https://lore.kernel.org/linux-kselftest/20230421084226.2...@google.com/

- Use the kunit_action_t typedef.
- This fixes some spurious checkpatch warnings.

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 3028a1a3fcad..2f23d6efa505 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -324,8 +324,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);

@@ -336,6 +339,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 f5e4ceffd282..d3fb93a23ccc 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -752,58 +752,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, (kunit_action_t *)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, (kunit_action_t *)kfree, (void *)ptr);
}
EXPORT_SYMBOL_GPL(kunit_kfree);

--
2.40.1.698.g37aff9b760-goog

[David Gow]

Add some basic documentation for kunit_add_action() and related
deferred action functions.

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

This patch is new in v2.

---
Documentation/dev-tools/kunit/usage.rst | 51 +++++++++++++++++++++++++
1 file changed, 51 insertions(+)

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 46957d1cbcbb..c2f0ed648385 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -615,6 +615,57 @@ For example:
KUNIT_ASSERT_STREQ(test, buffer, "");
}

+Registering Cleanup Actions
+---------------------------
+
+If you need to perform some cleanup beyond simple use of ``kunit_kzalloc``,
+you can register a cusom "deferred action", which is a cleanup function
+run when the test exits (whether cleanly, or via a failed assertion).
+
+Actions are simple functions with no return value, and a single ``void*``
+context argument, and forfil the same role as "cleanup" functions in Python
+and Go tests, "defer" statements in languages which support them, and
+(in some cases) destructors in RAII languages.
+
+These are very useful for unregistering things from global lists, closing
+files or other resources, or freeing resources.
+
+For example:
+
+.. code-block:: C
+
+ static void cleanup_device(void *ctx)
+ {
+ struct device *dev = (struct device *)ctx;
+
+ device_unregister(dev);
+ }
+
+ void example_device_test(struct kunit *test)
+ {
+ struct my_device dev;
+
+ device_register(&dev);
+
+ kunit_add_action(test, &cleanup_device, &dev);
+ }
+
+Note that, for functions like device_unregister which only accept a single
+pointer-sized argument, it's possible to directly cast that function to
+a ``kunit_action_t`` rather than writing a wrapper function, for example:
+
+.. code-block:: C
+
+ kunit_add_action(test, (kunit_action_t *)&device_unregister, &dev);
+
+``kunit_add_action`` can fail if, for example, the system is out of memory.
+You can use ``kunit_add_action_or_reset`` instead which runs the action
+immediately if it cannot be deferred.
+
+If you need more control over when the cleanup function is called, you
+can trigger it early using ``kunit_release_action``, or cancel it entirely
+with ``kunit_remove_action``.
+

Testing Static Functions
------------------------
--
2.40.1.698.g37aff9b760-goog

[kernel test robot]

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on lwn/docs-next]
[also build test WARNING on linus/master v6.4-rc2 next-20230518]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/David-Gow/kunit-executor_test-Use-kunit_add_action/20230518-171005
base: git://git.lwn.net/linux.git docs-next
patch link: https://lore.kernel.org/r/20230518083849.2631178-3-davidgow%40google.com
patch subject: [PATCH v2 3/4] kunit: kmalloc_array: Use kunit_add_action()
config: arm64-buildonly-randconfig-r003-20230517
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project b0fb98227c90adf2536c9ad644a74d5e92961111)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/f0c4ca4f24b370c1b4d9d35af1319ef1402e313c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review David-Gow/kunit-executor_test-Use-kunit_add_action/20230518-171005
git checkout f0c4ca4f24b370c1b4d9d35af1319ef1402e313c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash lib/kunit/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <l...@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305182336...@intel.com/

All warnings (new ones prefixed by >>):

>> lib/kunit/test.c:724:38: warning: cast from 'void (*)(const void *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]

if (kunit_add_action_or_reset(test, (kunit_action_t *)kfree, data) != 0)

^~~~~~~~~~~~~~~~~~~~~~~
lib/kunit/test.c:736:29: warning: cast from 'void (*)(const void *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]

kunit_release_action(test, (kunit_action_t *)kfree, (void *)ptr);

^~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.


vim +724 lib/kunit/test.c

714
715 void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t gfp)
716 {
717 void *data;
718
719 data = kmalloc_array(n, size, gfp);
720
721 if (!data)
722 return NULL;
723
> 724 if (kunit_add_action_or_reset(test, (kunit_action_t *)kfree, data) != 0)
725 return NULL;
726
727 return data;
728 }
729 EXPORT_SYMBOL_GPL(kunit_kmalloc_array);
730

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[kernel test robot]

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on lwn/docs-next]
[also build test WARNING on linus/master v6.4-rc2 next-20230518]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/David-Gow/kunit-executor_test-Use-kunit_add_action/20230518-171005
base: git://git.lwn.net/linux.git docs-next

patch link: https://lore.kernel.org/r/20230518083849.2631178-2-davidgow%40google.com
patch subject: [PATCH v2 2/4] kunit: executor_test: Use kunit_add_action()
config: hexagon-randconfig-r045-20230517

compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project b0fb98227c90adf2536c9ad644a74d5e92961111)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross

# https://github.com/intel-lab-lkp/linux/commit/38886419c3c213ab2922bd68236f87fa107d0f72

git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review David-Gow/kunit-executor_test-Use-kunit_add_action/20230518-171005

git checkout 38886419c3c213ab2922bd68236f87fa107d0f72

# save the config file
mkdir build_dir && cp config build_dir/.config

COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash lib/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <l...@intel.com>

| Closes: https://lore.kernel.org/oe-kbuild-all/202305190031...@intel.com/

All warnings (new ones prefixed by >>):

In file included from lib/kunit/executor.c:223:
>> lib/kunit/executor_test.c:138:4: warning: cast from 'void (*)(const void *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]
(kunit_action_t *)kfree,
^~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.


vim +138 lib/kunit/executor_test.c

127
128 /* Use the resource API to register a call to kfree(to_free).
129 * Since we never actually use the resource, it's safe to use on const data.
130 */
131 static void kfree_at_end(struct kunit *test, const void *to_free)
132 {
133 /* kfree() handles NULL already, but avoid allocating a no-op cleanup. */
134 if (IS_ERR_OR_NULL(to_free))
135 return;
136
137 kunit_add_action(test,
> 138 (kunit_action_t *)kfree,
139 (void *)to_free);
140 }
141

[Bagas Sanjaya]

On Thu, May 18, 2023 at 04:38:46PM +0800, David Gow wrote:
> +Registering Cleanup Actions
> +---------------------------
> +
> +If you need to perform some cleanup beyond simple use of ``kunit_kzalloc``,
> +you can register a cusom "deferred action", which is a cleanup function
> +run when the test exits (whether cleanly, or via a failed assertion).
> +
> +Actions are simple functions with no return value, and a single ``void*``
> +context argument, and forfil the same role as "cleanup" functions in Python

"... fulfill the same role ..."?

The rest is LGTM.

--
An old man doll... just what I always wanted! - Clara

[Rae Moar]

On Thu, May 18, 2023 at 4:39 AM David Gow <davi...@google.com> wrote:
>
> Add some basic documentation for kunit_add_action() and related
> deferred action functions.

Hi David!

This looks good to me. Just two typos below. Thanks!

Reviewed-by: Rae Moar <rm...@google.com>

>
> Signed-off-by: David Gow <davi...@google.com>
> ---
>
> This patch is new in v2.
>
> ---
> Documentation/dev-tools/kunit/usage.rst | 51 +++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index 46957d1cbcbb..c2f0ed648385 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -615,6 +615,57 @@ For example:
> KUNIT_ASSERT_STREQ(test, buffer, "");
> }
>
> +Registering Cleanup Actions
> +---------------------------
> +
> +If you need to perform some cleanup beyond simple use of ``kunit_kzalloc``,
> +you can register a cusom "deferred action", which is a cleanup function

Looks like a typo here: "custom"

> +run when the test exits (whether cleanly, or via a failed assertion).
> +
> +Actions are simple functions with no return value, and a single ``void*``
> +context argument, and forfil the same role as "cleanup" functions in Python

Another small typo here as Bagas noted.

[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.

Reviewed-by: Benjamin Berg <benjam...@intel.com>
Reviewed-by: Maxime Ripard <max...@cerno.tech>
Tested-by: Maxime Ripard <max...@cerno.tech>

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

No changes since v2:
https://lore.kernel.org/linux-kselftest/20230518083849.2...@google.com/

Changes since v1:
https://lore.kernel.org/linux-kselftest/20230421084226.2...@google.com/

+static void kunit_resource_test_action_ordering(struct kunit *test)
+{

2.41.0.rc0.172.g3f132b7071-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.

Reviewed-by: Benjamin Berg <benjam...@intel.com>
Reviewed-by: Maxime Ripard <max...@cerno.tech>
Tested-by: Maxime Ripard <max...@cerno.tech>
Signed-off-by: David Gow <davi...@google.com>
---

No changes since v2:
https://lore.kernel.org/linux-kselftest/20230518083849.2...@google.com/

Changes since v1:
https://lore.kernel.org/linux-kselftest/20230421084226.2...@google.com/

- Use the kunit_action_t typedef

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..ce6749af374d 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,
+ (kunit_action_t *)kfree,
+ (void *)to_free);
}

static struct kunit_suite *alloc_fake_suite(struct kunit *test,
--

2.41.0.rc0.172.g3f132b7071-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_defer_trigger_all() to implement
kunit_kfree().

Reviewed-by: Benjamin Berg <benjam...@intel.com>
Reviewed-by: Maxime Ripard <max...@cerno.tech>
Tested-by: Maxime Ripard <max...@cerno.tech>
Signed-off-by: David Gow <davi...@google.com>
---

No changes since v2:
https://lore.kernel.org/linux-kselftest/20230518083849.2...@google.com/

Changes since v1:
https://lore.kernel.org/linux-kselftest/20230421084226.2...@google.com/

- Use the kunit_action_t typedef.
- This fixes some spurious checkpatch warnings.

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

-static void kunit_kmalloc_array_free(struct kunit_resource *res)

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

+ kunit_release_action(test, (kunit_action_t *)kfree, (void *)ptr);
}
EXPORT_SYMBOL_GPL(kunit_kfree);

--
2.41.0.rc0.172.g3f132b7071-goog

[David Gow]

Add some basic documentation for kunit_add_action() and related
deferred action functions.

Reviewed-by: Rae Moar <rm...@google.com>

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

Changes since v2:
https://lore.kernel.org/linux-kselftest/20230518083849.2...@google.com/
- Fix a couple of typos (Thanks Bagas, Rae)
- Add Rae's Reviewed-by.

This patch is new in v2.

---
Documentation/dev-tools/kunit/usage.rst | 51 +++++++++++++++++++++++++
1 file changed, 51 insertions(+)

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst

index 46957d1cbcbb..c27e1646ecd9 100644

--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -615,6 +615,57 @@ For example:
KUNIT_ASSERT_STREQ(test, buffer, "");
}

+Registering Cleanup Actions
+---------------------------
+
+If you need to perform some cleanup beyond simple use of ``kunit_kzalloc``,

+you can register a custom "deferred action", which is a cleanup function

+run when the test exits (whether cleanly, or via a failed assertion).
+
+Actions are simple functions with no return value, and a single ``void*``

+context argument, and fulfill the same role as "cleanup" functions in Python

+and Go tests, "defer" statements in languages which support them, and
+(in some cases) destructors in RAII languages.
+
+These are very useful for unregistering things from global lists, closing
+files or other resources, or freeing resources.
+
+For example:
+
+.. code-block:: C
+
+ static void cleanup_device(void *ctx)
+ {
+ struct device *dev = (struct device *)ctx;
+
+ device_unregister(dev);
+ }
+

+ void example_device_test(struct kunit *test)
+ {

2.41.0.rc0.172.g3f132b7071-goog