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
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 <[email protected]>
---
Changes since RFCv2:
https://lore.kernel.org/linux-kselftest/[email protected]/
- 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,
- ¶ms);
+ 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
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 <[email protected]>
>
> 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
>
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
> Reviewed-By: Benjamin Berg <[email protected]>
>
> >
> > 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
> >
>
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 <[email protected]>
Tested-by: Maxime Ripard <[email protected]>
Thanks again,
Maxime
On Mon, 24 Apr 2023 at 22:02, Benjamin Berg <[email protected]> wrote:
>
> 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.
Ah, nice catch! I'll add those to the next version.
Cheers,
-- David
On Tue, 25 Apr 2023 at 23:23, Maxime Ripard <[email protected]> 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 <[email protected]>
> Tested-by: Maxime Ripard <[email protected]>
Cheers,
-- David