2022-03-26 00:36:33

by Daniel Latypov

[permalink] [raw]
Subject: [PATCH v2 2/2] kunit: split resource API impl from test.c into new resource.c

We've split out the declarations from include/kunit/test.h into
resource.h.
This patch splits out the definitions as well for consistency.

A side effect of this is git blame won't properly track history by
default, users need to run
$ git blame -L ,1 -C13 lib/kunit/resource.c

Signed-off-by: Daniel Latypov <[email protected]>
---
lib/kunit/Makefile | 1 +
lib/kunit/resource.c | 126 +++++++++++++++++++++++++++++++++++++++++++
lib/kunit/test.c | 116 +--------------------------------------
3 files changed, 128 insertions(+), 115 deletions(-)
create mode 100644 lib/kunit/resource.c

diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index c49f4ffb6273..29aff6562b42 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -1,6 +1,7 @@
obj-$(CONFIG_KUNIT) += kunit.o

kunit-objs += test.o \
+ resource.o \
string-stream.o \
assert.o \
try-catch.o \
diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c
new file mode 100644
index 000000000000..b8bced246217
--- /dev/null
+++ b/lib/kunit/resource.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit resource API for test managed resources (allocations, etc.).
+ *
+ * Copyright (C) 2022, Google LLC.
+ * Author: Daniel Latypov <[email protected]>
+ */
+
+#include <kunit/resource.h>
+#include <kunit/test.h>
+#include <linux/kref.h>
+
+/*
+ * Used for static resources and when a kunit_resource * has been created by
+ * kunit_alloc_resource(). When an init function is supplied, @data is passed
+ * into the init function; otherwise, we simply set the resource data field to
+ * the data value passed in.
+ */
+int kunit_add_resource(struct kunit *test,
+ kunit_resource_init_t init,
+ kunit_resource_free_t free,
+ struct kunit_resource *res,
+ void *data)
+{
+ int ret = 0;
+ unsigned long flags;
+
+ res->free = free;
+ kref_init(&res->refcount);
+
+ if (init) {
+ ret = init(res, data);
+ if (ret)
+ return ret;
+ } else {
+ res->data = data;
+ }
+
+ spin_lock_irqsave(&test->lock, flags);
+ list_add_tail(&res->node, &test->resources);
+ /* refcount for list is established by kref_init() */
+ spin_unlock_irqrestore(&test->lock, flags);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(kunit_add_resource);
+
+int kunit_add_named_resource(struct kunit *test,
+ kunit_resource_init_t init,
+ kunit_resource_free_t free,
+ struct kunit_resource *res,
+ const char *name,
+ void *data)
+{
+ struct kunit_resource *existing;
+
+ if (!name)
+ return -EINVAL;
+
+ existing = kunit_find_named_resource(test, name);
+ if (existing) {
+ kunit_put_resource(existing);
+ return -EEXIST;
+ }
+
+ res->name = name;
+
+ return kunit_add_resource(test, init, free, res, data);
+}
+EXPORT_SYMBOL_GPL(kunit_add_named_resource);
+
+struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test,
+ kunit_resource_init_t init,
+ kunit_resource_free_t free,
+ gfp_t internal_gfp,
+ void *data)
+{
+ struct kunit_resource *res;
+ int ret;
+
+ res = kzalloc(sizeof(*res), internal_gfp);
+ if (!res)
+ return NULL;
+
+ ret = kunit_add_resource(test, init, free, res, data);
+ if (!ret) {
+ /*
+ * bump refcount for get; kunit_resource_put() should be called
+ * when done.
+ */
+ kunit_get_resource(res);
+ return res;
+ }
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource);
+
+void kunit_remove_resource(struct kunit *test, struct kunit_resource *res)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&test->lock, flags);
+ list_del(&res->node);
+ spin_unlock_irqrestore(&test->lock, flags);
+ kunit_put_resource(res);
+}
+EXPORT_SYMBOL_GPL(kunit_remove_resource);
+
+int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match,
+ void *match_data)
+{
+ struct kunit_resource *res = kunit_find_resource(test, match,
+ match_data);
+
+ if (!res)
+ return -ENOENT;
+
+ kunit_remove_resource(test, res);
+
+ /* We have a reference also via _find(); drop it. */
+ kunit_put_resource(res);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(kunit_destroy_resource);
+
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 3bca3bf5c15b..0f66c13d126e 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -6,10 +6,10 @@
* Author: Brendan Higgins <[email protected]>
*/

+#include <kunit/resource.h>
#include <kunit/test.h>
#include <kunit/test-bug.h>
#include <linux/kernel.h>
-#include <linux/kref.h>
#include <linux/moduleparam.h>
#include <linux/sched/debug.h>
#include <linux/sched.h>
@@ -592,120 +592,6 @@ void __kunit_test_suites_exit(struct kunit_suite **suites)
}
EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);

-/*
- * Used for static resources and when a kunit_resource * has been created by
- * kunit_alloc_resource(). When an init function is supplied, @data is passed
- * into the init function; otherwise, we simply set the resource data field to
- * the data value passed in.
- */
-int kunit_add_resource(struct kunit *test,
- kunit_resource_init_t init,
- kunit_resource_free_t free,
- struct kunit_resource *res,
- void *data)
-{
- int ret = 0;
- unsigned long flags;
-
- res->free = free;
- kref_init(&res->refcount);
-
- if (init) {
- ret = init(res, data);
- if (ret)
- return ret;
- } else {
- res->data = data;
- }
-
- spin_lock_irqsave(&test->lock, flags);
- list_add_tail(&res->node, &test->resources);
- /* refcount for list is established by kref_init() */
- spin_unlock_irqrestore(&test->lock, flags);
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(kunit_add_resource);
-
-int kunit_add_named_resource(struct kunit *test,
- kunit_resource_init_t init,
- kunit_resource_free_t free,
- struct kunit_resource *res,
- const char *name,
- void *data)
-{
- struct kunit_resource *existing;
-
- if (!name)
- return -EINVAL;
-
- existing = kunit_find_named_resource(test, name);
- if (existing) {
- kunit_put_resource(existing);
- return -EEXIST;
- }
-
- res->name = name;
-
- return kunit_add_resource(test, init, free, res, data);
-}
-EXPORT_SYMBOL_GPL(kunit_add_named_resource);
-
-struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test,
- kunit_resource_init_t init,
- kunit_resource_free_t free,
- gfp_t internal_gfp,
- void *data)
-{
- struct kunit_resource *res;
- int ret;
-
- res = kzalloc(sizeof(*res), internal_gfp);
- if (!res)
- return NULL;
-
- ret = kunit_add_resource(test, init, free, res, data);
- if (!ret) {
- /*
- * bump refcount for get; kunit_resource_put() should be called
- * when done.
- */
- kunit_get_resource(res);
- return res;
- }
- return NULL;
-}
-EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource);
-
-void kunit_remove_resource(struct kunit *test, struct kunit_resource *res)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&test->lock, flags);
- list_del(&res->node);
- spin_unlock_irqrestore(&test->lock, flags);
- kunit_put_resource(res);
-}
-EXPORT_SYMBOL_GPL(kunit_remove_resource);
-
-int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match,
- void *match_data)
-{
- struct kunit_resource *res = kunit_find_resource(test, match,
- match_data);
-
- if (!res)
- return -ENOENT;
-
- kunit_remove_resource(test, res);
-
- /* We have a reference also via _find(); drop it. */
- kunit_put_resource(res);
-
- return 0;
-}
-EXPORT_SYMBOL_GPL(kunit_destroy_resource);
-
struct kunit_kmalloc_array_params {
size_t n;
size_t size;
--
2.35.1.1021.g381101b075-goog


2022-03-26 12:48:45

by David Gow

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] kunit: split resource API impl from test.c into new resource.c

On Sat, Mar 26, 2022 at 8:20 AM Daniel Latypov <[email protected]> wrote:
>
> We've split out the declarations from include/kunit/test.h into
> resource.h.
> This patch splits out the definitions as well for consistency.
>
> A side effect of this is git blame won't properly track history by
> default, users need to run
> $ git blame -L ,1 -C13 lib/kunit/resource.c
>
> Signed-off-by: Daniel Latypov <[email protected]>
> ---

Looks good and works fine here. I'm going to try to rebase most of the
other resource system stuff I'm working on on top of these (which will
likely end up moving a bunch of code _again_, but is probably the
least terrible of all the available options).

One nitpick (newline at end of file) below, otherwise this is good.

Reviewed-by: David Gow <[email protected]>

Cheers,
-- David

> lib/kunit/Makefile | 1 +
> lib/kunit/resource.c | 126 +++++++++++++++++++++++++++++++++++++++++++
> lib/kunit/test.c | 116 +--------------------------------------
> 3 files changed, 128 insertions(+), 115 deletions(-)
> create mode 100644 lib/kunit/resource.c
>
> diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
> index c49f4ffb6273..29aff6562b42 100644
> --- a/lib/kunit/Makefile
> +++ b/lib/kunit/Makefile
> @@ -1,6 +1,7 @@
> obj-$(CONFIG_KUNIT) += kunit.o
>
> kunit-objs += test.o \
> + resource.o \
> string-stream.o \
> assert.o \
> try-catch.o \
> diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c
> new file mode 100644
> index 000000000000..b8bced246217
> --- /dev/null
> +++ b/lib/kunit/resource.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit resource API for test managed resources (allocations, etc.).
> + *
> + * Copyright (C) 2022, Google LLC.
> + * Author: Daniel Latypov <[email protected]>
> + */
> +
> +#include <kunit/resource.h>
> +#include <kunit/test.h>
> +#include <linux/kref.h>
> +
> +/*
> + * Used for static resources and when a kunit_resource * has been created by
> + * kunit_alloc_resource(). When an init function is supplied, @data is passed
> + * into the init function; otherwise, we simply set the resource data field to
> + * the data value passed in.
> + */
> +int kunit_add_resource(struct kunit *test,
> + kunit_resource_init_t init,
> + kunit_resource_free_t free,
> + struct kunit_resource *res,
> + void *data)
> +{
> + int ret = 0;
> + unsigned long flags;
> +
> + res->free = free;
> + kref_init(&res->refcount);
> +
> + if (init) {
> + ret = init(res, data);
> + if (ret)
> + return ret;
> + } else {
> + res->data = data;
> + }
> +
> + spin_lock_irqsave(&test->lock, flags);
> + list_add_tail(&res->node, &test->resources);
> + /* refcount for list is established by kref_init() */
> + spin_unlock_irqrestore(&test->lock, flags);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kunit_add_resource);
> +
> +int kunit_add_named_resource(struct kunit *test,
> + kunit_resource_init_t init,
> + kunit_resource_free_t free,
> + struct kunit_resource *res,
> + const char *name,
> + void *data)
> +{
> + struct kunit_resource *existing;
> +
> + if (!name)
> + return -EINVAL;
> +
> + existing = kunit_find_named_resource(test, name);
> + if (existing) {
> + kunit_put_resource(existing);
> + return -EEXIST;
> + }
> +
> + res->name = name;
> +
> + return kunit_add_resource(test, init, free, res, data);
> +}
> +EXPORT_SYMBOL_GPL(kunit_add_named_resource);
> +
> +struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test,
> + kunit_resource_init_t init,
> + kunit_resource_free_t free,
> + gfp_t internal_gfp,
> + void *data)
> +{
> + struct kunit_resource *res;
> + int ret;
> +
> + res = kzalloc(sizeof(*res), internal_gfp);
> + if (!res)
> + return NULL;
> +
> + ret = kunit_add_resource(test, init, free, res, data);
> + if (!ret) {
> + /*
> + * bump refcount for get; kunit_resource_put() should be called
> + * when done.
> + */
> + kunit_get_resource(res);
> + return res;
> + }
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource);
> +
> +void kunit_remove_resource(struct kunit *test, struct kunit_resource *res)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&test->lock, flags);
> + list_del(&res->node);
> + spin_unlock_irqrestore(&test->lock, flags);
> + kunit_put_resource(res);
> +}
> +EXPORT_SYMBOL_GPL(kunit_remove_resource);
> +
> +int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match,
> + void *match_data)
> +{
> + struct kunit_resource *res = kunit_find_resource(test, match,
> + match_data);
> +
> + if (!res)
> + return -ENOENT;
> +
> + kunit_remove_resource(test, res);
> +
> + /* We have a reference also via _find(); drop it. */
> + kunit_put_resource(res);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(kunit_destroy_resource);
> +

.git/rebase-apply/patch:151: new blank line at EOF.

+

> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 3bca3bf5c15b..0f66c13d126e 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -6,10 +6,10 @@
> * Author: Brendan Higgins <[email protected]>
> */
>
> +#include <kunit/resource.h>
> #include <kunit/test.h>
> #include <kunit/test-bug.h>
> #include <linux/kernel.h>
> -#include <linux/kref.h>
> #include <linux/moduleparam.h>
> #include <linux/sched/debug.h>
> #include <linux/sched.h>
> @@ -592,120 +592,6 @@ void __kunit_test_suites_exit(struct kunit_suite **suites)
> }
> EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);
>
> -/*
> - * Used for static resources and when a kunit_resource * has been created by
> - * kunit_alloc_resource(). When an init function is supplied, @data is passed
> - * into the init function; otherwise, we simply set the resource data field to
> - * the data value passed in.
> - */
> -int kunit_add_resource(struct kunit *test,
> - kunit_resource_init_t init,
> - kunit_resource_free_t free,
> - struct kunit_resource *res,
> - void *data)
> -{
> - int ret = 0;
> - unsigned long flags;
> -
> - res->free = free;
> - kref_init(&res->refcount);
> -
> - if (init) {
> - ret = init(res, data);
> - if (ret)
> - return ret;
> - } else {
> - res->data = data;
> - }
> -
> - spin_lock_irqsave(&test->lock, flags);
> - list_add_tail(&res->node, &test->resources);
> - /* refcount for list is established by kref_init() */
> - spin_unlock_irqrestore(&test->lock, flags);
> -
> - return ret;
> -}
> -EXPORT_SYMBOL_GPL(kunit_add_resource);
> -
> -int kunit_add_named_resource(struct kunit *test,
> - kunit_resource_init_t init,
> - kunit_resource_free_t free,
> - struct kunit_resource *res,
> - const char *name,
> - void *data)
> -{
> - struct kunit_resource *existing;
> -
> - if (!name)
> - return -EINVAL;
> -
> - existing = kunit_find_named_resource(test, name);
> - if (existing) {
> - kunit_put_resource(existing);
> - return -EEXIST;
> - }
> -
> - res->name = name;
> -
> - return kunit_add_resource(test, init, free, res, data);
> -}
> -EXPORT_SYMBOL_GPL(kunit_add_named_resource);
> -
> -struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test,
> - kunit_resource_init_t init,
> - kunit_resource_free_t free,
> - gfp_t internal_gfp,
> - void *data)
> -{
> - struct kunit_resource *res;
> - int ret;
> -
> - res = kzalloc(sizeof(*res), internal_gfp);
> - if (!res)
> - return NULL;
> -
> - ret = kunit_add_resource(test, init, free, res, data);
> - if (!ret) {
> - /*
> - * bump refcount for get; kunit_resource_put() should be called
> - * when done.
> - */
> - kunit_get_resource(res);
> - return res;
> - }
> - return NULL;
> -}
> -EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource);
> -
> -void kunit_remove_resource(struct kunit *test, struct kunit_resource *res)
> -{
> - unsigned long flags;
> -
> - spin_lock_irqsave(&test->lock, flags);
> - list_del(&res->node);
> - spin_unlock_irqrestore(&test->lock, flags);
> - kunit_put_resource(res);
> -}
> -EXPORT_SYMBOL_GPL(kunit_remove_resource);
> -
> -int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match,
> - void *match_data)
> -{
> - struct kunit_resource *res = kunit_find_resource(test, match,
> - match_data);
> -
> - if (!res)
> - return -ENOENT;
> -
> - kunit_remove_resource(test, res);
> -
> - /* We have a reference also via _find(); drop it. */
> - kunit_put_resource(res);
> -
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(kunit_destroy_resource);
> -
> struct kunit_kmalloc_array_params {
> size_t n;
> size_t size;
> --
> 2.35.1.1021.g381101b075-goog
>

2022-03-28 20:40:59

by Daniel Latypov

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] kunit: split resource API impl from test.c into new resource.c

On Fri, Mar 25, 2022 at 11:27 PM David Gow <[email protected]> wrote:
>
> On Sat, Mar 26, 2022 at 8:20 AM Daniel Latypov <[email protected]> wrote:
> >
> > We've split out the declarations from include/kunit/test.h into
> > resource.h.
> > This patch splits out the definitions as well for consistency.
> >
> > A side effect of this is git blame won't properly track history by
> > default, users need to run
> > $ git blame -L ,1 -C13 lib/kunit/resource.c
> >
> > Signed-off-by: Daniel Latypov <[email protected]>
> > ---
>
> Looks good and works fine here. I'm going to try to rebase most of the
> other resource system stuff I'm working on on top of these (which will
> likely end up moving a bunch of code _again_, but is probably the
> least terrible of all the available options).
>
> One nitpick (newline at end of file) below, otherwise this is good.

Huh, I had thought checkpatch --strict would catch this.
I guess not.

Fixing this and sending out a v3.

>
> Reviewed-by: David Gow <[email protected]>

2022-03-28 23:02:47

by Brendan Higgins

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] kunit: split resource API impl from test.c into new resource.c

On Fri, Mar 25, 2022 at 8:20 PM Daniel Latypov <[email protected]> wrote:
>
> We've split out the declarations from include/kunit/test.h into
> resource.h.
> This patch splits out the definitions as well for consistency.
>
> A side effect of this is git blame won't properly track history by
> default, users need to run
> $ git blame -L ,1 -C13 lib/kunit/resource.c
>
> Signed-off-by: Daniel Latypov <[email protected]>

Reviewed-by: Brendan Higgins <[email protected]>