* [PATCH] kunit: split resource API from test.h into new resource.h
@ 2022-03-16 2:44 Daniel Latypov
2022-03-16 5:41 ` David Gow
2022-03-23 23:52 ` Brendan Higgins
0 siblings, 2 replies; 8+ messages in thread
From: Daniel Latypov @ 2022-03-16 2:44 UTC (permalink / raw)
To: brendanhiggins, davidgow
Cc: linux-kernel, kunit-dev, linux-kselftest, skhan, Daniel Latypov
Background:
Currently, a reader looking at kunit/test.h will find the file is quite
long, and the first meaty comment is a doc comment about struct
kunit_resource.
Most users will not ever use the KUnit resource API directly.
They'll use kunit_kmalloc() and friends, or decide it's simpler to do
cleanups via labels (it often can be) instead of figuring out how to use
the API.
It's also logically separate from everything else in test.h.
Removing it from the file doesn't cause any compilation errors (since
struct kunit has `struct list_head resources` to store them).
This commit:
Let's move it into a kunit/resource.h file and give it a separate page
in the docs, kunit/api/resource.rst.
We include resource.h at the bottom of test.h since
* don't want to force existing users to add a new include if they use the API
* it accesses `lock` inside `struct kunit` in a inline func
* so we can't just forward declare, and the alternatives require
uninlining the func, adding hepers to lock/unlock, or other more
invasive changes.
Now the first big comment in test.h is about kunit_case, which is a lot
more relevant to what a new user wants to know.
A side effect of this is git blame won't properly track history by
default, users need to run
$ git blame -L ,1 -C17 include/kunit/resource.h
Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
NOTE: this file doesn't split out code from test.c to a new resource.c
file.
I'm primarily concerned with users trying to read the headers, so I
didn't think messing up git blame (w/ default settings) was worth it.
But I can make that change if it feels appropriate (it might also be
messier).
---
Documentation/dev-tools/kunit/api/index.rst | 5 +
.../dev-tools/kunit/api/resource.rst | 13 +
include/kunit/resource.h | 319 ++++++++++++++++++
include/kunit/test.h | 301 +----------------
4 files changed, 339 insertions(+), 299 deletions(-)
create mode 100644 Documentation/dev-tools/kunit/api/resource.rst
create mode 100644 include/kunit/resource.h
diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
index 3006cadcf44a..45ce04823f9f 100644
--- a/Documentation/dev-tools/kunit/api/index.rst
+++ b/Documentation/dev-tools/kunit/api/index.rst
@@ -6,6 +6,7 @@ API Reference
.. toctree::
test
+ resource
This section documents the KUnit kernel testing API. It is divided into the
following sections:
@@ -13,3 +14,7 @@ following sections:
Documentation/dev-tools/kunit/api/test.rst
- documents all of the standard testing API
+
+Documentation/dev-tools/kunit/api/resource.rst
+
+ - documents the KUnit resource API
diff --git a/Documentation/dev-tools/kunit/api/resource.rst b/Documentation/dev-tools/kunit/api/resource.rst
new file mode 100644
index 000000000000..0a94f831259e
--- /dev/null
+++ b/Documentation/dev-tools/kunit/api/resource.rst
@@ -0,0 +1,13 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+============
+Resource API
+============
+
+This file documents the KUnit resource API.
+
+Most users won't need to use this API directly, power users can use it to store
+state on a per-test basis, register custom cleanup actions, and more.
+
+.. kernel-doc:: include/kunit/resource.h
+ :internal:
diff --git a/include/kunit/resource.h b/include/kunit/resource.h
new file mode 100644
index 000000000000..7e044787795a
--- /dev/null
+++ b/include/kunit/resource.h
@@ -0,0 +1,319 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * KUnit resource API for test managed resources (allocations, etc.).
+ *
+ * Copyright (C) 2022, Google LLC.
+ * Author: Daniel Latypov <dlatypov@google.com>
+ */
+
+#ifndef _KUNIT_RESOURCE_H
+#define _KUNIT_RESOURCE_H
+
+#include <kunit/test.h>
+
+#include <linux/kref.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+struct kunit;
+struct kunit_resource;
+
+typedef int (*kunit_resource_init_t)(struct kunit_resource *, void *);
+typedef void (*kunit_resource_free_t)(struct kunit_resource *);
+
+/**
+ * struct kunit_resource - represents a *test managed resource*
+ * @data: for the user to store arbitrary data.
+ * @name: optional name
+ * @free: a user supplied function to free the resource. Populated by
+ * kunit_resource_alloc().
+ *
+ * Represents a *test managed resource*, a resource which will automatically be
+ * cleaned up at the end of a test case.
+ *
+ * Resources are reference counted so if a resource is retrieved via
+ * kunit_alloc_and_get_resource() or kunit_find_resource(), we need
+ * to call kunit_put_resource() to reduce the resource reference count
+ * when finished with it. Note that kunit_alloc_resource() does not require a
+ * kunit_resource_put() because it does not retrieve the resource itself.
+ *
+ * Example:
+ *
+ * .. code-block:: c
+ *
+ * struct kunit_kmalloc_params {
+ * size_t size;
+ * gfp_t gfp;
+ * };
+ *
+ * static int kunit_kmalloc_init(struct kunit_resource *res, void *context)
+ * {
+ * struct kunit_kmalloc_params *params = context;
+ * res->data = kmalloc(params->size, params->gfp);
+ *
+ * if (!res->data)
+ * return -ENOMEM;
+ *
+ * return 0;
+ * }
+ *
+ * static void kunit_kmalloc_free(struct kunit_resource *res)
+ * {
+ * kfree(res->data);
+ * }
+ *
+ * void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp)
+ * {
+ * struct kunit_kmalloc_params params;
+ *
+ * params.size = size;
+ * params.gfp = gfp;
+ *
+ * return kunit_alloc_resource(test, kunit_kmalloc_init,
+ * kunit_kmalloc_free, ¶ms);
+ * }
+ *
+ * Resources can also be named, with lookup/removal done on a name
+ * basis also. kunit_add_named_resource(), kunit_find_named_resource()
+ * and kunit_destroy_named_resource(). Resource names must be
+ * unique within the test instance.
+ */
+struct kunit_resource {
+ void *data;
+ const char *name;
+ kunit_resource_free_t free;
+
+ /* private: internal use only. */
+ struct kref refcount;
+ struct list_head node;
+};
+
+/*
+ * Like kunit_alloc_resource() below, but returns the struct kunit_resource
+ * object that contains the allocation. This is mostly for testing purposes.
+ */
+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 *context);
+
+/**
+ * kunit_get_resource() - Hold resource for use. Should not need to be used
+ * by most users as we automatically get resources
+ * retrieved by kunit_find_resource*().
+ * @res: resource
+ */
+static inline void kunit_get_resource(struct kunit_resource *res)
+{
+ kref_get(&res->refcount);
+}
+
+/*
+ * Called when refcount reaches zero via kunit_put_resource();
+ * should not be called directly.
+ */
+static inline void kunit_release_resource(struct kref *kref)
+{
+ struct kunit_resource *res = container_of(kref, struct kunit_resource,
+ refcount);
+
+ /* If free function is defined, resource was dynamically allocated. */
+ if (res->free) {
+ res->free(res);
+ kfree(res);
+ }
+}
+
+/**
+ * kunit_put_resource() - When caller is done with retrieved resource,
+ * kunit_put_resource() should be called to drop
+ * reference count. The resource list maintains
+ * a reference count on resources, so if no users
+ * are utilizing a resource and it is removed from
+ * the resource list, it will be freed via the
+ * associated free function (if any). Only
+ * needs to be used if we alloc_and_get() or
+ * find() resource.
+ * @res: resource
+ */
+static inline void kunit_put_resource(struct kunit_resource *res)
+{
+ kref_put(&res->refcount, kunit_release_resource);
+}
+
+/**
+ * kunit_add_resource() - Add a *test managed resource*.
+ * @test: The test context object.
+ * @init: a user-supplied function to initialize the result (if needed). If
+ * none is supplied, the resource data value is simply set to @data.
+ * If an init function is supplied, @data is passed to it instead.
+ * @free: a user-supplied function to free the resource (if needed).
+ * @res: The resource.
+ * @data: value to pass to init function or set in resource data field.
+ */
+int kunit_add_resource(struct kunit *test,
+ kunit_resource_init_t init,
+ kunit_resource_free_t free,
+ struct kunit_resource *res,
+ void *data);
+
+/**
+ * kunit_add_named_resource() - Add a named *test managed resource*.
+ * @test: The test context object.
+ * @init: a user-supplied function to initialize the resource data, if needed.
+ * @free: a user-supplied function to free the resource data, if needed.
+ * @res: The resource.
+ * @name: name to be set for resource.
+ * @data: value to pass to init function or set in resource data field.
+ */
+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);
+
+/**
+ * kunit_alloc_resource() - Allocates a *test managed resource*.
+ * @test: The test context object.
+ * @init: a user supplied function to initialize the resource.
+ * @free: a user supplied function to free the resource.
+ * @internal_gfp: gfp to use for internal allocations, if unsure, use GFP_KERNEL
+ * @context: for the user to pass in arbitrary data to the init function.
+ *
+ * Allocates a *test managed resource*, a resource which will automatically be
+ * cleaned up at the end of a test case. See &struct kunit_resource for an
+ * example.
+ *
+ * Note: KUnit needs to allocate memory for a kunit_resource object. You must
+ * specify an @internal_gfp that is compatible with the use context of your
+ * resource.
+ */
+static inline void *kunit_alloc_resource(struct kunit *test,
+ kunit_resource_init_t init,
+ kunit_resource_free_t free,
+ gfp_t internal_gfp,
+ void *context)
+{
+ struct kunit_resource *res;
+
+ res = kzalloc(sizeof(*res), internal_gfp);
+ if (!res)
+ return NULL;
+
+ if (!kunit_add_resource(test, init, free, res, context))
+ return res->data;
+
+ return NULL;
+}
+
+typedef bool (*kunit_resource_match_t)(struct kunit *test,
+ struct kunit_resource *res,
+ void *match_data);
+
+/**
+ * kunit_resource_instance_match() - Match a resource with the same instance.
+ * @test: Test case to which the resource belongs.
+ * @res: The resource.
+ * @match_data: The resource pointer to match against.
+ *
+ * An instance of kunit_resource_match_t that matches a resource whose
+ * allocation matches @match_data.
+ */
+static inline bool kunit_resource_instance_match(struct kunit *test,
+ struct kunit_resource *res,
+ void *match_data)
+{
+ return res->data == match_data;
+}
+
+/**
+ * kunit_resource_name_match() - Match a resource with the same name.
+ * @test: Test case to which the resource belongs.
+ * @res: The resource.
+ * @match_name: The name to match against.
+ */
+static inline bool kunit_resource_name_match(struct kunit *test,
+ struct kunit_resource *res,
+ void *match_name)
+{
+ return res->name && strcmp(res->name, match_name) == 0;
+}
+
+/**
+ * kunit_find_resource() - Find a resource using match function/data.
+ * @test: Test case to which the resource belongs.
+ * @match: match function to be applied to resources/match data.
+ * @match_data: data to be used in matching.
+ */
+static inline struct kunit_resource *
+kunit_find_resource(struct kunit *test,
+ kunit_resource_match_t match,
+ void *match_data)
+{
+ struct kunit_resource *res, *found = NULL;
+ unsigned long flags;
+
+ spin_lock_irqsave(&test->lock, flags);
+
+ list_for_each_entry_reverse(res, &test->resources, node) {
+ if (match(test, res, (void *)match_data)) {
+ found = res;
+ kunit_get_resource(found);
+ break;
+ }
+ }
+
+ spin_unlock_irqrestore(&test->lock, flags);
+
+ return found;
+}
+
+/**
+ * kunit_find_named_resource() - Find a resource using match name.
+ * @test: Test case to which the resource belongs.
+ * @name: match name.
+ */
+static inline struct kunit_resource *
+kunit_find_named_resource(struct kunit *test,
+ const char *name)
+{
+ return kunit_find_resource(test, kunit_resource_name_match,
+ (void *)name);
+}
+
+/**
+ * kunit_destroy_resource() - Find a kunit_resource and destroy it.
+ * @test: Test case to which the resource belongs.
+ * @match: Match function. Returns whether a given resource matches @match_data.
+ * @match_data: Data passed into @match.
+ *
+ * RETURNS:
+ * 0 if kunit_resource is found and freed, -ENOENT if not found.
+ */
+int kunit_destroy_resource(struct kunit *test,
+ kunit_resource_match_t match,
+ void *match_data);
+
+static inline int kunit_destroy_named_resource(struct kunit *test,
+ const char *name)
+{
+ return kunit_destroy_resource(test, kunit_resource_name_match,
+ (void *)name);
+}
+
+/**
+ * kunit_remove_resource() - remove resource from resource list associated with
+ * test.
+ * @test: The test context object.
+ * @res: The resource to be removed.
+ *
+ * Note that the resource will not be immediately freed since it is likely
+ * the caller has a reference to it via alloc_and_get() or find();
+ * in this case a final call to kunit_put_resource() is required.
+ */
+void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
+
+#endif /* _KUNIT_RESOURCE_H */
diff --git a/include/kunit/test.h b/include/kunit/test.h
index b26400731c02..93def5e7c099 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -26,78 +26,6 @@
#include <asm/rwonce.h>
-struct kunit_resource;
-
-typedef int (*kunit_resource_init_t)(struct kunit_resource *, void *);
-typedef void (*kunit_resource_free_t)(struct kunit_resource *);
-
-/**
- * struct kunit_resource - represents a *test managed resource*
- * @data: for the user to store arbitrary data.
- * @name: optional name
- * @free: a user supplied function to free the resource. Populated by
- * kunit_resource_alloc().
- *
- * Represents a *test managed resource*, a resource which will automatically be
- * cleaned up at the end of a test case.
- *
- * Resources are reference counted so if a resource is retrieved via
- * kunit_alloc_and_get_resource() or kunit_find_resource(), we need
- * to call kunit_put_resource() to reduce the resource reference count
- * when finished with it. Note that kunit_alloc_resource() does not require a
- * kunit_resource_put() because it does not retrieve the resource itself.
- *
- * Example:
- *
- * .. code-block:: c
- *
- * struct kunit_kmalloc_params {
- * size_t size;
- * gfp_t gfp;
- * };
- *
- * static int kunit_kmalloc_init(struct kunit_resource *res, void *context)
- * {
- * struct kunit_kmalloc_params *params = context;
- * res->data = kmalloc(params->size, params->gfp);
- *
- * if (!res->data)
- * return -ENOMEM;
- *
- * return 0;
- * }
- *
- * static void kunit_kmalloc_free(struct kunit_resource *res)
- * {
- * kfree(res->data);
- * }
- *
- * void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp)
- * {
- * struct kunit_kmalloc_params params;
- *
- * params.size = size;
- * params.gfp = gfp;
- *
- * return kunit_alloc_resource(test, kunit_kmalloc_init,
- * kunit_kmalloc_free, ¶ms);
- * }
- *
- * Resources can also be named, with lookup/removal done on a name
- * basis also. kunit_add_named_resource(), kunit_find_named_resource()
- * and kunit_destroy_named_resource(). Resource names must be
- * unique within the test instance.
- */
-struct kunit_resource {
- void *data;
- const char *name;
- kunit_resource_free_t free;
-
- /* private: internal use only. */
- struct kref refcount;
- struct list_head node;
-};
-
struct kunit;
/* Size of log associated with test. */
@@ -384,233 +312,6 @@ static inline int kunit_run_all_tests(void)
enum kunit_status kunit_suite_has_succeeded(struct kunit_suite *suite);
-/*
- * Like kunit_alloc_resource() below, but returns the struct kunit_resource
- * object that contains the allocation. This is mostly for testing purposes.
- */
-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 *context);
-
-/**
- * kunit_get_resource() - Hold resource for use. Should not need to be used
- * by most users as we automatically get resources
- * retrieved by kunit_find_resource*().
- * @res: resource
- */
-static inline void kunit_get_resource(struct kunit_resource *res)
-{
- kref_get(&res->refcount);
-}
-
-/*
- * Called when refcount reaches zero via kunit_put_resources();
- * should not be called directly.
- */
-static inline void kunit_release_resource(struct kref *kref)
-{
- struct kunit_resource *res = container_of(kref, struct kunit_resource,
- refcount);
-
- /* If free function is defined, resource was dynamically allocated. */
- if (res->free) {
- res->free(res);
- kfree(res);
- }
-}
-
-/**
- * kunit_put_resource() - When caller is done with retrieved resource,
- * kunit_put_resource() should be called to drop
- * reference count. The resource list maintains
- * a reference count on resources, so if no users
- * are utilizing a resource and it is removed from
- * the resource list, it will be freed via the
- * associated free function (if any). Only
- * needs to be used if we alloc_and_get() or
- * find() resource.
- * @res: resource
- */
-static inline void kunit_put_resource(struct kunit_resource *res)
-{
- kref_put(&res->refcount, kunit_release_resource);
-}
-
-/**
- * kunit_add_resource() - Add a *test managed resource*.
- * @test: The test context object.
- * @init: a user-supplied function to initialize the result (if needed). If
- * none is supplied, the resource data value is simply set to @data.
- * If an init function is supplied, @data is passed to it instead.
- * @free: a user-supplied function to free the resource (if needed).
- * @res: The resource.
- * @data: value to pass to init function or set in resource data field.
- */
-int kunit_add_resource(struct kunit *test,
- kunit_resource_init_t init,
- kunit_resource_free_t free,
- struct kunit_resource *res,
- void *data);
-
-/**
- * kunit_add_named_resource() - Add a named *test managed resource*.
- * @test: The test context object.
- * @init: a user-supplied function to initialize the resource data, if needed.
- * @free: a user-supplied function to free the resource data, if needed.
- * @res: The resource.
- * @name: name to be set for resource.
- * @data: value to pass to init function or set in resource data field.
- */
-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);
-
-/**
- * kunit_alloc_resource() - Allocates a *test managed resource*.
- * @test: The test context object.
- * @init: a user supplied function to initialize the resource.
- * @free: a user supplied function to free the resource.
- * @internal_gfp: gfp to use for internal allocations, if unsure, use GFP_KERNEL
- * @context: for the user to pass in arbitrary data to the init function.
- *
- * Allocates a *test managed resource*, a resource which will automatically be
- * cleaned up at the end of a test case. See &struct kunit_resource for an
- * example.
- *
- * Note: KUnit needs to allocate memory for a kunit_resource object. You must
- * specify an @internal_gfp that is compatible with the use context of your
- * resource.
- */
-static inline void *kunit_alloc_resource(struct kunit *test,
- kunit_resource_init_t init,
- kunit_resource_free_t free,
- gfp_t internal_gfp,
- void *context)
-{
- struct kunit_resource *res;
-
- res = kzalloc(sizeof(*res), internal_gfp);
- if (!res)
- return NULL;
-
- if (!kunit_add_resource(test, init, free, res, context))
- return res->data;
-
- return NULL;
-}
-
-typedef bool (*kunit_resource_match_t)(struct kunit *test,
- struct kunit_resource *res,
- void *match_data);
-
-/**
- * kunit_resource_instance_match() - Match a resource with the same instance.
- * @test: Test case to which the resource belongs.
- * @res: The resource.
- * @match_data: The resource pointer to match against.
- *
- * An instance of kunit_resource_match_t that matches a resource whose
- * allocation matches @match_data.
- */
-static inline bool kunit_resource_instance_match(struct kunit *test,
- struct kunit_resource *res,
- void *match_data)
-{
- return res->data == match_data;
-}
-
-/**
- * kunit_resource_name_match() - Match a resource with the same name.
- * @test: Test case to which the resource belongs.
- * @res: The resource.
- * @match_name: The name to match against.
- */
-static inline bool kunit_resource_name_match(struct kunit *test,
- struct kunit_resource *res,
- void *match_name)
-{
- return res->name && strcmp(res->name, match_name) == 0;
-}
-
-/**
- * kunit_find_resource() - Find a resource using match function/data.
- * @test: Test case to which the resource belongs.
- * @match: match function to be applied to resources/match data.
- * @match_data: data to be used in matching.
- */
-static inline struct kunit_resource *
-kunit_find_resource(struct kunit *test,
- kunit_resource_match_t match,
- void *match_data)
-{
- struct kunit_resource *res, *found = NULL;
- unsigned long flags;
-
- spin_lock_irqsave(&test->lock, flags);
-
- list_for_each_entry_reverse(res, &test->resources, node) {
- if (match(test, res, (void *)match_data)) {
- found = res;
- kunit_get_resource(found);
- break;
- }
- }
-
- spin_unlock_irqrestore(&test->lock, flags);
-
- return found;
-}
-
-/**
- * kunit_find_named_resource() - Find a resource using match name.
- * @test: Test case to which the resource belongs.
- * @name: match name.
- */
-static inline struct kunit_resource *
-kunit_find_named_resource(struct kunit *test,
- const char *name)
-{
- return kunit_find_resource(test, kunit_resource_name_match,
- (void *)name);
-}
-
-/**
- * kunit_destroy_resource() - Find a kunit_resource and destroy it.
- * @test: Test case to which the resource belongs.
- * @match: Match function. Returns whether a given resource matches @match_data.
- * @match_data: Data passed into @match.
- *
- * RETURNS:
- * 0 if kunit_resource is found and freed, -ENOENT if not found.
- */
-int kunit_destroy_resource(struct kunit *test,
- kunit_resource_match_t match,
- void *match_data);
-
-static inline int kunit_destroy_named_resource(struct kunit *test,
- const char *name)
-{
- return kunit_destroy_resource(test, kunit_resource_name_match,
- (void *)name);
-}
-
-/**
- * kunit_remove_resource() - remove resource from resource list associated with
- * test.
- * @test: The test context object.
- * @res: The resource to be removed.
- *
- * Note that the resource will not be immediately freed since it is likely
- * the caller has a reference to it via alloc_and_get() or find();
- * in this case a final call to kunit_put_resource() is required.
- */
-void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
-
/**
* kunit_kmalloc_array() - Like kmalloc_array() except the allocation is *test managed*.
* @test: The test context object.
@@ -1891,4 +1592,6 @@ do { \
return NULL; \
}
+#include <kunit/resource.h>
+
#endif /* _KUNIT_TEST_H */
base-commit: 09688c0166e76ce2fb85e86b9d99be8b0084cdf9
--
2.35.1.723.g4982287a31-goog
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] kunit: split resource API from test.h into new resource.h
2022-03-16 2:44 [PATCH] kunit: split resource API from test.h into new resource.h Daniel Latypov
@ 2022-03-16 5:41 ` David Gow
2022-03-16 16:19 ` Daniel Latypov
2022-03-23 23:52 ` Brendan Higgins
1 sibling, 1 reply; 8+ messages in thread
From: David Gow @ 2022-03-16 5:41 UTC (permalink / raw)
To: Daniel Latypov
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
[-- Attachment #1: Type: text/plain, Size: 3726 bytes --]
On Wed, Mar 16, 2022 at 10:44 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> Background:
> Currently, a reader looking at kunit/test.h will find the file is quite
> long, and the first meaty comment is a doc comment about struct
> kunit_resource.
>
> Most users will not ever use the KUnit resource API directly.
> They'll use kunit_kmalloc() and friends, or decide it's simpler to do
> cleanups via labels (it often can be) instead of figuring out how to use
> the API.
>
A depressing (but probably not untrue) thought. I think that, even if
most people were to use the resource API, having it in test.h makes it
harder, as having the resource functions separate makes it easier to
understand as well.
> It's also logically separate from everything else in test.h.
> Removing it from the file doesn't cause any compilation errors (since
> struct kunit has `struct list_head resources` to store them).
>
> This commit:
> Let's move it into a kunit/resource.h file and give it a separate page
> in the docs, kunit/api/resource.rst.
Yay! This makes a lot of sense to me, as I've wasted a lot of time
scrolling through test.h.
>
> We include resource.h at the bottom of test.h since
> * don't want to force existing users to add a new include if they use the API
> * it accesses `lock` inside `struct kunit` in a inline func
> * so we can't just forward declare, and the alternatives require
> uninlining the func, adding hepers to lock/unlock, or other more
> invasive changes.
I don't like this, but still think it's an improvement on what we have
now. Ultimately, I think adding helpers to lock/unlock or similar and
making users include this separately is probably the right thing to
do, as nesting the headers like this is a bit ugly, but I won't lose
sleep over leaving it till later.
>
> Now the first big comment in test.h is about kunit_case, which is a lot
> more relevant to what a new user wants to know.
>
> A side effect of this is git blame won't properly track history by
> default, users need to run
> $ git blame -L ,1 -C17 include/kunit/resource.h
This is a pain, but is probably worth it. Thanks for including the
command in the commit message, which should mitigate it slightly.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---
This was starting to annoy me, too, as it was a pain to read through
everything in test.h. It'll be a bit of short-term pain,
merge-conflict wise if we have other changes to the resource system
(which I fear is likely), but is worth it.
Reviewed-by: David Gow <davidgow@google.com>
-- David
>
> NOTE: this file doesn't split out code from test.c to a new resource.c
> file.
> I'm primarily concerned with users trying to read the headers, so I
> didn't think messing up git blame (w/ default settings) was worth it.
> But I can make that change if it feels appropriate (it might also be
> messier).
Personally, I think it's probably worth splitting this out as well.
And the sooner we do it, the less history we'll obscure. :-)
But I agree, it's less of an issue as it only directly affects people
working on KUnit itself. Though making it easier for users to find and
read the implementation of these functions could help them understand
API "gotchas", so I think it's worthwhile.
>
> ---
> Documentation/dev-tools/kunit/api/index.rst | 5 +
> .../dev-tools/kunit/api/resource.rst | 13 +
> include/kunit/resource.h | 319 ++++++++++++++++++
> include/kunit/test.h | 301 +----------------
> 4 files changed, 339 insertions(+), 299 deletions(-)
> create mode 100644 Documentation/dev-tools/kunit/api/resource.rst
> create mode 100644 include/kunit/resource.h
>
<...snip...>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kunit: split resource API from test.h into new resource.h
2022-03-16 5:41 ` David Gow
@ 2022-03-16 16:19 ` Daniel Latypov
2022-03-17 8:03 ` David Gow
2022-03-23 23:51 ` Brendan Higgins
0 siblings, 2 replies; 8+ messages in thread
From: Daniel Latypov @ 2022-03-16 16:19 UTC (permalink / raw)
To: David Gow
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
On Wed, Mar 16, 2022 at 12:41 AM David Gow <davidgow@google.com> wrote:
>
> On Wed, Mar 16, 2022 at 10:44 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > Background:
> > Currently, a reader looking at kunit/test.h will find the file is quite
> > long, and the first meaty comment is a doc comment about struct
> > kunit_resource.
> >
> > Most users will not ever use the KUnit resource API directly.
> > They'll use kunit_kmalloc() and friends, or decide it's simpler to do
> > cleanups via labels (it often can be) instead of figuring out how to use
> > the API.
> >
>
> A depressing (but probably not untrue) thought. I think that, even if
I'm not sure it's that depressing.
Without some compiler support (e.g. GCC's `cleanup`), I can see there
being a number of one-off things that don't warrant formalizing into a
resource.
More detail:
It works OK when there's one pointer parameter, e.g. [1], but I feel
like you'd normally need to capture at least one more local variable.
So then you need to define a new struct to hold all the values, which
is where I'd draw the line personally.
[1] https://elixir.bootlin.com/linux/v5.17-rc8/source/lib/kunit/executor_test.c#L182
> most people were to use the resource API, having it in test.h makes it
> harder, as having the resource functions separate makes it easier to
> understand as well.
>
> > It's also logically separate from everything else in test.h.
> > Removing it from the file doesn't cause any compilation errors (since
> > struct kunit has `struct list_head resources` to store them).
> >
> > This commit:
> > Let's move it into a kunit/resource.h file and give it a separate page
> > in the docs, kunit/api/resource.rst.
>
> Yay! This makes a lot of sense to me, as I've wasted a lot of time
> scrolling through test.h.
>
> >
> > We include resource.h at the bottom of test.h since
> > * don't want to force existing users to add a new include if they use the API
> > * it accesses `lock` inside `struct kunit` in a inline func
> > * so we can't just forward declare, and the alternatives require
> > uninlining the func, adding hepers to lock/unlock, or other more
> > invasive changes.
>
> I don't like this, but still think it's an improvement on what we have
> now. Ultimately, I think adding helpers to lock/unlock or similar and
Yes, I can see us maybe needing this in the future.
Right now, outside of test.c, there's only one callsite for each (in
resource.h).
> making users include this separately is probably the right thing to
> do, as nesting the headers like this is a bit ugly, but I won't lose
> sleep over leaving it till later.
Ack, I can add a TODO to indicate we want to clean this up?
It's a bit annoying right now, but it'll only get more annoying in the future.
>
> >
> > Now the first big comment in test.h is about kunit_case, which is a lot
> > more relevant to what a new user wants to know.
> >
> > A side effect of this is git blame won't properly track history by
> > default, users need to run
> > $ git blame -L ,1 -C17 include/kunit/resource.h
>
> This is a pain, but is probably worth it. Thanks for including the
> command in the commit message, which should mitigate it slightly.
>
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
>
> This was starting to annoy me, too, as it was a pain to read through
> everything in test.h. It'll be a bit of short-term pain,
> merge-conflict wise if we have other changes to the resource system
> (which I fear is likely), but is worth it.
>
> Reviewed-by: David Gow <davidgow@google.com>
>
> -- David
>
> >
> > NOTE: this file doesn't split out code from test.c to a new resource.c
> > file.
> > I'm primarily concerned with users trying to read the headers, so I
> > didn't think messing up git blame (w/ default settings) was worth it.
> > But I can make that change if it feels appropriate (it might also be
> > messier).
>
> Personally, I think it's probably worth splitting this out as well.
> And the sooner we do it, the less history we'll obscure. :-)
Yeah, that was my thought.
But if you think this would help users, then I think we have a case to
make this change.
Should I send a v2 with resource.c split out?
Brendan (and any others who have an opinion), what's your preference?
>
> But I agree, it's less of an issue as it only directly affects people
> working on KUnit itself. Though making it easier for users to find and
> read the implementation of these functions could help them understand
> API "gotchas", so I think it's worthwhile.
>
> >
> > ---
> > Documentation/dev-tools/kunit/api/index.rst | 5 +
> > .../dev-tools/kunit/api/resource.rst | 13 +
> > include/kunit/resource.h | 319 ++++++++++++++++++
> > include/kunit/test.h | 301 +----------------
> > 4 files changed, 339 insertions(+), 299 deletions(-)
> > create mode 100644 Documentation/dev-tools/kunit/api/resource.rst
> > create mode 100644 include/kunit/resource.h
> >
> <...snip...>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kunit: split resource API from test.h into new resource.h
2022-03-16 16:19 ` Daniel Latypov
@ 2022-03-17 8:03 ` David Gow
2022-03-23 23:51 ` Brendan Higgins
1 sibling, 0 replies; 8+ messages in thread
From: David Gow @ 2022-03-17 8:03 UTC (permalink / raw)
To: Daniel Latypov
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
[-- Attachment #1: Type: text/plain, Size: 5840 bytes --]
On Thu, Mar 17, 2022 at 12:19 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Wed, Mar 16, 2022 at 12:41 AM David Gow <davidgow@google.com> wrote:
> >
> > On Wed, Mar 16, 2022 at 10:44 AM Daniel Latypov <dlatypov@google.com> wrote:
> > >
> > > Background:
> > > Currently, a reader looking at kunit/test.h will find the file is quite
> > > long, and the first meaty comment is a doc comment about struct
> > > kunit_resource.
> > >
> > > Most users will not ever use the KUnit resource API directly.
> > > They'll use kunit_kmalloc() and friends, or decide it's simpler to do
> > > cleanups via labels (it often can be) instead of figuring out how to use
> > > the API.
> > >
> >
> > A depressing (but probably not untrue) thought. I think that, even if
>
> I'm not sure it's that depressing.
> Without some compiler support (e.g. GCC's `cleanup`), I can see there
> being a number of one-off things that don't warrant formalizing into a
> resource.
True, though I do think that the resources API could use a bit of
polish to reduce the friction involved in figuring out how to use the
API.
(And this patch is a good start!)
> More detail:
> It works OK when there's one pointer parameter, e.g. [1], but I feel
> like you'd normally need to capture at least one more local variable.
> So then you need to define a new struct to hold all the values, which
> is where I'd draw the line personally.
>
> [1] https://elixir.bootlin.com/linux/v5.17-rc8/source/lib/kunit/executor_test.c#L182
>
> > most people were to use the resource API, having it in test.h makes it
> > harder, as having the resource functions separate makes it easier to
> > understand as well.
> >
> > > It's also logically separate from everything else in test.h.
> > > Removing it from the file doesn't cause any compilation errors (since
> > > struct kunit has `struct list_head resources` to store them).
> > >
> > > This commit:
> > > Let's move it into a kunit/resource.h file and give it a separate page
> > > in the docs, kunit/api/resource.rst.
> >
> > Yay! This makes a lot of sense to me, as I've wasted a lot of time
> > scrolling through test.h.
> >
> > >
> > > We include resource.h at the bottom of test.h since
> > > * don't want to force existing users to add a new include if they use the API
> > > * it accesses `lock` inside `struct kunit` in a inline func
> > > * so we can't just forward declare, and the alternatives require
> > > uninlining the func, adding hepers to lock/unlock, or other more
> > > invasive changes.
> >
> > I don't like this, but still think it's an improvement on what we have
> > now. Ultimately, I think adding helpers to lock/unlock or similar and
>
> Yes, I can see us maybe needing this in the future.
> Right now, outside of test.c, there's only one callsite for each (in
> resource.h).
>
> > making users include this separately is probably the right thing to
> > do, as nesting the headers like this is a bit ugly, but I won't lose
> > sleep over leaving it till later.
>
> Ack, I can add a TODO to indicate we want to clean this up?
> It's a bit annoying right now, but it'll only get more annoying in the future.
Yeah, let's get this in largely as-is first, then we can start adding
direct includes of "resource.h" where necessary before making it
required.
> >
> > >
> > > Now the first big comment in test.h is about kunit_case, which is a lot
> > > more relevant to what a new user wants to know.
> > >
> > > A side effect of this is git blame won't properly track history by
> > > default, users need to run
> > > $ git blame -L ,1 -C17 include/kunit/resource.h
> >
> > This is a pain, but is probably worth it. Thanks for including the
> > command in the commit message, which should mitigate it slightly.
> >
> > >
> > > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > > ---
> >
> > This was starting to annoy me, too, as it was a pain to read through
> > everything in test.h. It'll be a bit of short-term pain,
> > merge-conflict wise if we have other changes to the resource system
> > (which I fear is likely), but is worth it.
> >
> > Reviewed-by: David Gow <davidgow@google.com>
> >
> > -- David
> >
> > >
> > > NOTE: this file doesn't split out code from test.c to a new resource.c
> > > file.
> > > I'm primarily concerned with users trying to read the headers, so I
> > > didn't think messing up git blame (w/ default settings) was worth it.
> > > But I can make that change if it feels appropriate (it might also be
> > > messier).
> >
> > Personally, I think it's probably worth splitting this out as well.
> > And the sooner we do it, the less history we'll obscure. :-)
>
> Yeah, that was my thought.
> But if you think this would help users, then I think we have a case to
> make this change.
>
> Should I send a v2 with resource.c split out?
> Brendan (and any others who have an opinion), what's your preference?
I think it's a separate enough thing that this patch could go in
as-is, and resource.c could be split in a separate one if you
preferred. But doing it in a v2 is fine as well.
> >
> > But I agree, it's less of an issue as it only directly affects people
> > working on KUnit itself. Though making it easier for users to find and
> > read the implementation of these functions could help them understand
> > API "gotchas", so I think it's worthwhile.
> >
> > >
> > > ---
> > > Documentation/dev-tools/kunit/api/index.rst | 5 +
> > > .../dev-tools/kunit/api/resource.rst | 13 +
> > > include/kunit/resource.h | 319 ++++++++++++++++++
> > > include/kunit/test.h | 301 +----------------
> > > 4 files changed, 339 insertions(+), 299 deletions(-)
> > > create mode 100644 Documentation/dev-tools/kunit/api/resource.rst
> > > create mode 100644 include/kunit/resource.h
> > >
> > <...snip...>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kunit: split resource API from test.h into new resource.h
2022-03-16 16:19 ` Daniel Latypov
2022-03-17 8:03 ` David Gow
@ 2022-03-23 23:51 ` Brendan Higgins
2022-03-25 0:08 ` Daniel Latypov
1 sibling, 1 reply; 8+ messages in thread
From: Brendan Higgins @ 2022-03-23 23:51 UTC (permalink / raw)
To: Daniel Latypov
Cc: David Gow, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
On Wed, Mar 16, 2022 at 12:19 PM 'Daniel Latypov' via KUnit
Development <kunit-dev@googlegroups.com> wrote:
>
> On Wed, Mar 16, 2022 at 12:41 AM David Gow <davidgow@google.com> wrote:
> >
> > On Wed, Mar 16, 2022 at 10:44 AM Daniel Latypov <dlatypov@google.com> wrote:
> > >
> > > Background:
> > > Currently, a reader looking at kunit/test.h will find the file is quite
> > > long, and the first meaty comment is a doc comment about struct
> > > kunit_resource.
> > >
> > > Most users will not ever use the KUnit resource API directly.
> > > They'll use kunit_kmalloc() and friends, or decide it's simpler to do
> > > cleanups via labels (it often can be) instead of figuring out how to use
> > > the API.
> > >
> >
> > A depressing (but probably not untrue) thought. I think that, even if
>
> I'm not sure it's that depressing.
> Without some compiler support (e.g. GCC's `cleanup`), I can see there
> being a number of one-off things that don't warrant formalizing into a
> resource.
>
> More detail:
> It works OK when there's one pointer parameter, e.g. [1], but I feel
> like you'd normally need to capture at least one more local variable.
> So then you need to define a new struct to hold all the values, which
> is where I'd draw the line personally.
>
> [1] https://elixir.bootlin.com/linux/v5.17-rc8/source/lib/kunit/executor_test.c#L182
>
> > most people were to use the resource API, having it in test.h makes it
> > harder, as having the resource functions separate makes it easier to
> > understand as well.
> >
> > > It's also logically separate from everything else in test.h.
> > > Removing it from the file doesn't cause any compilation errors (since
> > > struct kunit has `struct list_head resources` to store them).
> > >
> > > This commit:
> > > Let's move it into a kunit/resource.h file and give it a separate page
> > > in the docs, kunit/api/resource.rst.
> >
> > Yay! This makes a lot of sense to me, as I've wasted a lot of time
> > scrolling through test.h.
> >
> > >
> > > We include resource.h at the bottom of test.h since
> > > * don't want to force existing users to add a new include if they use the API
> > > * it accesses `lock` inside `struct kunit` in a inline func
> > > * so we can't just forward declare, and the alternatives require
> > > uninlining the func, adding hepers to lock/unlock, or other more
> > > invasive changes.
> >
> > I don't like this, but still think it's an improvement on what we have
> > now. Ultimately, I think adding helpers to lock/unlock or similar and
>
> Yes, I can see us maybe needing this in the future.
> Right now, outside of test.c, there's only one callsite for each (in
> resource.h).
Another alternative workaround is to split even more stuff out into
other header files.
Personally I would prefer not to wrap the lock/unlock functions; I
like seeing the kind of locking that's happening. Plus, such a helper
would be pretty gross:
void kunit_lock(struct kunit *test, unsigned long* flags) {...}
It wouldn't actually clean up the call site, just facilitate breaking
out code into a header.
> > making users include this separately is probably the right thing to
> > do, as nesting the headers like this is a bit ugly, but I won't lose
> > sleep over leaving it till later.
>
> Ack, I can add a TODO to indicate we want to clean this up?
I am fine with this.
> It's a bit annoying right now, but it'll only get more annoying in the future.
>
> >
> > >
> > > Now the first big comment in test.h is about kunit_case, which is a lot
> > > more relevant to what a new user wants to know.
> > >
> > > A side effect of this is git blame won't properly track history by
> > > default, users need to run
> > > $ git blame -L ,1 -C17 include/kunit/resource.h
> >
> > This is a pain, but is probably worth it. Thanks for including the
> > command in the commit message, which should mitigate it slightly.
> >
> > >
> > > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > > ---
> >
> > This was starting to annoy me, too, as it was a pain to read through
> > everything in test.h. It'll be a bit of short-term pain,
> > merge-conflict wise if we have other changes to the resource system
> > (which I fear is likely), but is worth it.
> >
> > Reviewed-by: David Gow <davidgow@google.com>
> >
> > -- David
> >
> > >
> > > NOTE: this file doesn't split out code from test.c to a new resource.c
> > > file.
> > > I'm primarily concerned with users trying to read the headers, so I
> > > didn't think messing up git blame (w/ default settings) was worth it.
> > > But I can make that change if it feels appropriate (it might also be
> > > messier).
> >
> > Personally, I think it's probably worth splitting this out as well.
> > And the sooner we do it, the less history we'll obscure. :-)
>
> Yeah, that was my thought.
> But if you think this would help users, then I think we have a case to
> make this change.
>
> Should I send a v2 with resource.c split out?
> Brendan (and any others who have an opinion), what's your preference?
I personally don't think test.c is so huge that it is a problem to
understand, but I can see it getting there.
If it's going to happen, sooner is probably better.
> >
> > But I agree, it's less of an issue as it only directly affects people
> > working on KUnit itself. Though making it easier for users to find and
> > read the implementation of these functions could help them understand
> > API "gotchas", so I think it's worthwhile.
> >
> > >
> > > ---
> > > Documentation/dev-tools/kunit/api/index.rst | 5 +
> > > .../dev-tools/kunit/api/resource.rst | 13 +
> > > include/kunit/resource.h | 319 ++++++++++++++++++
> > > include/kunit/test.h | 301 +----------------
> > > 4 files changed, 339 insertions(+), 299 deletions(-)
> > > create mode 100644 Documentation/dev-tools/kunit/api/resource.rst
> > > create mode 100644 include/kunit/resource.h
> > >
> > <...snip...>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] kunit: split resource API from test.h into new resource.h
2022-03-23 23:51 ` Brendan Higgins
@ 2022-03-25 0:08 ` Daniel Latypov
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Latypov @ 2022-03-25 0:08 UTC (permalink / raw)
To: Brendan Higgins
Cc: David Gow, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
On Wed, Mar 23, 2022 at 6:51 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
<snip>
>
> Another alternative workaround is to split even more stuff out into
> other header files.
>
> Personally I would prefer not to wrap the lock/unlock functions; I
> like seeing the kind of locking that's happening. Plus, such a helper
> would be pretty gross:
>
> void kunit_lock(struct kunit *test, unsigned long* flags) {...}
That's exactly why I didn't bother to try and wrap it, yeah.
>
> It wouldn't actually clean up the call site, just facilitate breaking
> out code into a header.
>
> > > making users include this separately is probably the right thing to
> > > do, as nesting the headers like this is a bit ugly, but I won't lose
> > > sleep over leaving it till later.
> >
> > Ack, I can add a TODO to indicate we want to clean this up?
>
> I am fine with this.
To clarify, are you saying you're fine w/ the nested header as-is, or
fine with it + a TODO?
>
> > It's a bit annoying right now, but it'll only get more annoying in the future.
> >
> > >
> > > >
> > > > Now the first big comment in test.h is about kunit_case, which is a lot
> > > > more relevant to what a new user wants to know.
> > > >
> > > > A side effect of this is git blame won't properly track history by
> > > > default, users need to run
> > > > $ git blame -L ,1 -C17 include/kunit/resource.h
> > >
> > > This is a pain, but is probably worth it. Thanks for including the
> > > command in the commit message, which should mitigate it slightly.
> > >
> > > >
> > > > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > > > ---
> > >
> > > This was starting to annoy me, too, as it was a pain to read through
> > > everything in test.h. It'll be a bit of short-term pain,
> > > merge-conflict wise if we have other changes to the resource system
> > > (which I fear is likely), but is worth it.
> > >
> > > Reviewed-by: David Gow <davidgow@google.com>
> > >
> > > -- David
> > >
> > > >
> > > > NOTE: this file doesn't split out code from test.c to a new resource.c
> > > > file.
> > > > I'm primarily concerned with users trying to read the headers, so I
> > > > didn't think messing up git blame (w/ default settings) was worth it.
> > > > But I can make that change if it feels appropriate (it might also be
> > > > messier).
> > >
> > > Personally, I think it's probably worth splitting this out as well.
> > > And the sooner we do it, the less history we'll obscure. :-)
> >
> > Yeah, that was my thought.
> > But if you think this would help users, then I think we have a case to
> > make this change.
> >
> > Should I send a v2 with resource.c split out?
> > Brendan (and any others who have an opinion), what's your preference?
>
> I personally don't think test.c is so huge that it is a problem to
> understand, but I can see it getting there.
>
> If it's going to happen, sooner is probably better.
Ack.
I can work on adding a second patch on to this series that splits out
resource.c?
That causes more churn for the other in-flight patches, but we already
have some since David has some changes in test.h.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kunit: split resource API from test.h into new resource.h
2022-03-16 2:44 [PATCH] kunit: split resource API from test.h into new resource.h Daniel Latypov
2022-03-16 5:41 ` David Gow
@ 2022-03-23 23:52 ` Brendan Higgins
1 sibling, 0 replies; 8+ messages in thread
From: Brendan Higgins @ 2022-03-23 23:52 UTC (permalink / raw)
To: Daniel Latypov; +Cc: davidgow, linux-kernel, kunit-dev, linux-kselftest, skhan
On Tue, Mar 15, 2022 at 10:44 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> Background:
> Currently, a reader looking at kunit/test.h will find the file is quite
> long, and the first meaty comment is a doc comment about struct
> kunit_resource.
>
> Most users will not ever use the KUnit resource API directly.
> They'll use kunit_kmalloc() and friends, or decide it's simpler to do
> cleanups via labels (it often can be) instead of figuring out how to use
> the API.
>
> It's also logically separate from everything else in test.h.
> Removing it from the file doesn't cause any compilation errors (since
> struct kunit has `struct list_head resources` to store them).
>
> This commit:
> Let's move it into a kunit/resource.h file and give it a separate page
> in the docs, kunit/api/resource.rst.
>
> We include resource.h at the bottom of test.h since
> * don't want to force existing users to add a new include if they use the API
> * it accesses `lock` inside `struct kunit` in a inline func
> * so we can't just forward declare, and the alternatives require
> uninlining the func, adding hepers to lock/unlock, or other more
> invasive changes.
>
> Now the first big comment in test.h is about kunit_case, which is a lot
> more relevant to what a new user wants to know.
>
> A side effect of this is git blame won't properly track history by
> default, users need to run
> $ git blame -L ,1 -C17 include/kunit/resource.h
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
Aside from the discussion below,
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kunit: split resource API from test.h into new resource.h
@ 2022-03-16 21:41 kernel test robot
0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2022-03-16 21:41 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 15715 bytes --]
CC: llvm(a)lists.linux.dev
CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220316024432.1454366-1-dlatypov@google.com>
References: <20220316024432.1454366-1-dlatypov@google.com>
TO: Daniel Latypov <dlatypov@google.com>
TO: brendanhiggins(a)google.com
TO: davidgow(a)google.com
CC: linux-kernel(a)vger.kernel.org
CC: kunit-dev(a)googlegroups.com
CC: linux-kselftest(a)vger.kernel.org
CC: skhan(a)linuxfoundation.org
CC: Daniel Latypov <dlatypov@google.com>
Hi Daniel,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on 09688c0166e76ce2fb85e86b9d99be8b0084cdf9]
url: https://github.com/0day-ci/linux/commits/Daniel-Latypov/kunit-split-resource-API-from-test-h-into-new-resource-h/20220316-104559
base: 09688c0166e76ce2fb85e86b9d99be8b0084cdf9
:::::: branch date: 19 hours ago
:::::: commit date: 19 hours ago
config: arm-randconfig-c002-20220313 (https://download.01.org/0day-ci/archive/20220317/202203170501.cCagiYT2-lkp(a)intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a6ec1e3d798f8eab43fb3a91028c6ab04e115fcb)
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 arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/0day-ci/linux/commit/132662b59a0f2d3d7b21c94ff6829725a6b606f5
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Daniel-Latypov/kunit-split-resource-API-from-test-h-into-new-resource-h/20220316-104559
git checkout 132662b59a0f2d3d7b21c94ff6829725a6b606f5
# save the config file to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
kernel/rcu/rcuscale.c:453:7: note: Left side of '&&' is true
if (!started &&
^
kernel/rcu/rcuscale.c:454:7: note: Assuming the condition is false
atomic_read(&n_rcu_scale_writer_started) >= nrealwriters)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/rcu/rcuscale.c:453:3: note: Taking false branch
if (!started &&
^
kernel/rcu/rcuscale.c:456:8: note: 'done' is false
if (!done && i >= MIN_MEAS) {
^~~~
kernel/rcu/rcuscale.c:456:7: note: Left side of '&&' is true
if (!done && i >= MIN_MEAS) {
^
kernel/rcu/rcuscale.c:456:16: note: 'i' is < MIN_MEAS
if (!done && i >= MIN_MEAS) {
^
kernel/rcu/rcuscale.c:456:3: note: Taking false branch
if (!done && i >= MIN_MEAS) {
^
kernel/rcu/rcuscale.c:480:7: note: 'done' is false
if (done && !alldone &&
^~~~
kernel/rcu/rcuscale.c:480:12: note: Left side of '&&' is false
if (done && !alldone &&
^
kernel/rcu/rcuscale.c:483:7: note: 'started' is false
if (started && !alldone && i < MAX_MEAS - 1)
^~~~~~~
kernel/rcu/rcuscale.c:483:15: note: Left side of '&&' is false
if (started && !alldone && i < MAX_MEAS - 1)
^
kernel/rcu/rcuscale.c:486:11: note: Assuming the condition is true
} while (!torture_must_stop());
^~~~~~~~~~~~~~~~~~~~
kernel/rcu/rcuscale.c:426:2: note: Loop condition is true. Execution continues on line 427
do {
^
kernel/rcu/rcuscale.c:427:7: note: Assuming 'writer_holdoff' is 0
if (writer_holdoff)
^~~~~~~~~~~~~~
kernel/rcu/rcuscale.c:427:3: note: Taking false branch
if (writer_holdoff)
^
kernel/rcu/rcuscale.c:431:7: note: Assuming 'gp_async' is true
if (gp_async) {
^~~~~~~~
kernel/rcu/rcuscale.c:431:3: note: Taking true branch
if (gp_async) {
^
kernel/rcu/rcuscale.c:433:9: note: 'rhp' is non-null
if (!rhp)
^~~
kernel/rcu/rcuscale.c:433:4: note: Taking false branch
if (!rhp)
^
kernel/rcu/rcuscale.c:435:8: note: 'rhp' is non-null
if (rhp && atomic_read(this_cpu_ptr(&n_async_inflight)) < gp_async_max) {
^~~
kernel/rcu/rcuscale.c:435:8: note: Left side of '&&' is true
kernel/rcu/rcuscale.c:435:27: note: Loop condition is false. Exiting loop
if (rhp && atomic_read(this_cpu_ptr(&n_async_inflight)) < gp_async_max) {
^
include/linux/percpu-defs.h:265:27: note: expanded from macro 'this_cpu_ptr'
#define this_cpu_ptr(ptr) raw_cpu_ptr(ptr)
^
include/linux/percpu-defs.h:264:26: note: expanded from macro 'raw_cpu_ptr'
#define raw_cpu_ptr(ptr) per_cpu_ptr(ptr, 0)
^
include/linux/percpu-defs.h:263:47: note: expanded from macro 'per_cpu_ptr'
#define per_cpu_ptr(ptr, cpu) ({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); })
^
include/linux/percpu-defs.h:259:2: note: expanded from macro 'VERIFY_PERCPU_PTR'
__verify_pcpu_ptr(__p); \
^
include/linux/percpu-defs.h:217:37: note: expanded from macro '__verify_pcpu_ptr'
#define __verify_pcpu_ptr(ptr) \
^
kernel/rcu/rcuscale.c:435:15: note: Assuming the condition is false
if (rhp && atomic_read(this_cpu_ptr(&n_async_inflight)) < gp_async_max) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/rcu/rcuscale.c:435:4: note: Taking false branch
if (rhp && atomic_read(this_cpu_ptr(&n_async_inflight)) < gp_async_max) {
^
kernel/rcu/rcuscale.c:439:15: note: Assuming the condition is false
} else if (!kthread_should_stop()) {
^~~~~~~~~~~~~~~~~~~~~~
kernel/rcu/rcuscale.c:439:11: note: Taking false branch
} else if (!kthread_should_stop()) {
^
kernel/rcu/rcuscale.c:443:5: note: Attempt to free released memory
kfree(rhp); /* Because we are stopping. */
^~~~~~~~~~
Suppressed 4 warnings (4 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
6 warnings generated.
Suppressed 6 warnings (2 in non-user code, 4 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
4 warnings generated.
>> include/kunit/resource.h:125:3: warning: Argument to kfree() is the address of a local stack variable, which is not memory allocated by malloc() [clang-analyzer-unix.Malloc]
kfree(res);
^
lib/kunit/kunit-test.c:333:2: note: Assuming '__left' is not equal to '__right'
KUNIT_EXPECT_EQ(test,
^
include/kunit/test.h:1063:2: note: expanded from macro 'KUNIT_EXPECT_EQ'
KUNIT_BINARY_EQ_ASSERTION(test, KUNIT_EXPECTATION, left, right)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/kunit/test.h:700:2: note: expanded from macro 'KUNIT_BINARY_EQ_ASSERTION'
KUNIT_BINARY_EQ_MSG_ASSERTION(test, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/kunit/test.h:690:2: note: expanded from macro 'KUNIT_BINARY_EQ_MSG_ASSERTION'
KUNIT_BASE_EQ_MSG_ASSERTION(test, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/kunit/test.h:601:2: note: expanded from macro 'KUNIT_BASE_EQ_MSG_ASSERTION'
KUNIT_BASE_BINARY_ASSERTION(test, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/kunit/test.h:580:4: note: expanded from macro 'KUNIT_BASE_BINARY_ASSERTION'
__left op __right, \
~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/kunit/test.h:483:7: note: expanded from macro 'KUNIT_ASSERTION'
pass, \
^~~~
lib/kunit/kunit-test.c:333:2: note: Loop condition is false. Exiting loop
KUNIT_EXPECT_EQ(test,
^
include/kunit/test.h:1063:2: note: expanded from macro 'KUNIT_EXPECT_EQ'
KUNIT_BINARY_EQ_ASSERTION(test, KUNIT_EXPECTATION, left, right)
^
include/kunit/test.h:700:2: note: expanded from macro 'KUNIT_BINARY_EQ_ASSERTION'
KUNIT_BINARY_EQ_MSG_ASSERTION(test, \
^
include/kunit/test.h:690:2: note: expanded from macro 'KUNIT_BINARY_EQ_MSG_ASSERTION'
KUNIT_BASE_EQ_MSG_ASSERTION(test, \
^
include/kunit/test.h:601:2: note: expanded from macro 'KUNIT_BASE_EQ_MSG_ASSERTION'
KUNIT_BASE_BINARY_ASSERTION(test, \
^
include/kunit/test.h:579:2: note: expanded from macro 'KUNIT_BASE_BINARY_ASSERTION'
KUNIT_ASSERTION(test, \
^
include/kunit/test.h:479:74: note: expanded from macro 'KUNIT_ASSERTION'
#define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do { \
^
lib/kunit/kunit-test.c:333:2: note: Loop condition is false. Exiting loop
KUNIT_EXPECT_EQ(test,
^
include/kunit/test.h:1063:2: note: expanded from macro 'KUNIT_EXPECT_EQ'
KUNIT_BINARY_EQ_ASSERTION(test, KUNIT_EXPECTATION, left, right)
^
include/kunit/test.h:700:2: note: expanded from macro 'KUNIT_BINARY_EQ_ASSERTION'
KUNIT_BINARY_EQ_MSG_ASSERTION(test, \
^
include/kunit/test.h:690:2: note: expanded from macro 'KUNIT_BINARY_EQ_MSG_ASSERTION'
KUNIT_BASE_EQ_MSG_ASSERTION(test, \
^
include/kunit/test.h:601:2: note: expanded from macro 'KUNIT_BASE_EQ_MSG_ASSERTION'
KUNIT_BASE_BINARY_ASSERTION(test, \
^
include/kunit/test.h:574:24: note: expanded from macro 'KUNIT_BASE_BINARY_ASSERTION'
...) \
^
lib/kunit/kunit-test.c:337:2: note: Loop condition is false. Exiting loop
KUNIT_EXPECT_PTR_EQ(test, res1.data, (void *)&ctx);
^
include/kunit/test.h:1085:2: note: expanded from macro 'KUNIT_EXPECT_PTR_EQ'
KUNIT_BINARY_PTR_EQ_ASSERTION(test, \
^
include/kunit/test.h:722:2: note: expanded from macro 'KUNIT_BINARY_PTR_EQ_ASSERTION'
KUNIT_BINARY_PTR_EQ_MSG_ASSERTION(test, \
^
include/kunit/test.h:712:2: note: expanded from macro 'KUNIT_BINARY_PTR_EQ_MSG_ASSERTION'
KUNIT_BASE_EQ_MSG_ASSERTION(test, \
^
include/kunit/test.h:601:2: note: expanded from macro 'KUNIT_BASE_EQ_MSG_ASSERTION'
KUNIT_BASE_BINARY_ASSERTION(test, \
^
include/kunit/test.h:579:2: note: expanded from macro 'KUNIT_BASE_BINARY_ASSERTION'
KUNIT_ASSERTION(test, \
^
include/kunit/test.h:479:74: note: expanded from macro 'KUNIT_ASSERTION'
#define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do { \
^
lib/kunit/kunit-test.c:337:2: note: Loop condition is false. Exiting loop
KUNIT_EXPECT_PTR_EQ(test, res1.data, (void *)&ctx);
^
include/kunit/test.h:1085:2: note: expanded from macro 'KUNIT_EXPECT_PTR_EQ'
KUNIT_BINARY_PTR_EQ_ASSERTION(test, \
^
include/kunit/test.h:722:2: note: expanded from macro 'KUNIT_BINARY_PTR_EQ_ASSERTION'
KUNIT_BINARY_PTR_EQ_MSG_ASSERTION(test, \
^
include/kunit/test.h:712:2: note: expanded from macro 'KUNIT_BINARY_PTR_EQ_MSG_ASSERTION'
KUNIT_BASE_EQ_MSG_ASSERTION(test, \
^
include/kunit/test.h:601:2: note: expanded from macro 'KUNIT_BASE_EQ_MSG_ASSERTION'
KUNIT_BASE_BINARY_ASSERTION(test, \
^
include/kunit/test.h:574:24: note: expanded from macro 'KUNIT_BASE_BINARY_ASSERTION'
...) \
vim +125 include/kunit/resource.h
132662b59a0f2d3 Daniel Latypov 2022-03-15 112
132662b59a0f2d3 Daniel Latypov 2022-03-15 113 /*
132662b59a0f2d3 Daniel Latypov 2022-03-15 114 * Called when refcount reaches zero via kunit_put_resource();
132662b59a0f2d3 Daniel Latypov 2022-03-15 115 * should not be called directly.
132662b59a0f2d3 Daniel Latypov 2022-03-15 116 */
132662b59a0f2d3 Daniel Latypov 2022-03-15 117 static inline void kunit_release_resource(struct kref *kref)
132662b59a0f2d3 Daniel Latypov 2022-03-15 118 {
132662b59a0f2d3 Daniel Latypov 2022-03-15 119 struct kunit_resource *res = container_of(kref, struct kunit_resource,
132662b59a0f2d3 Daniel Latypov 2022-03-15 120 refcount);
132662b59a0f2d3 Daniel Latypov 2022-03-15 121
132662b59a0f2d3 Daniel Latypov 2022-03-15 122 /* If free function is defined, resource was dynamically allocated. */
132662b59a0f2d3 Daniel Latypov 2022-03-15 123 if (res->free) {
132662b59a0f2d3 Daniel Latypov 2022-03-15 124 res->free(res);
132662b59a0f2d3 Daniel Latypov 2022-03-15 @125 kfree(res);
132662b59a0f2d3 Daniel Latypov 2022-03-15 126 }
132662b59a0f2d3 Daniel Latypov 2022-03-15 127 }
132662b59a0f2d3 Daniel Latypov 2022-03-15 128
---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-03-25 0:08 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-16 2:44 [PATCH] kunit: split resource API from test.h into new resource.h Daniel Latypov
2022-03-16 5:41 ` David Gow
2022-03-16 16:19 ` Daniel Latypov
2022-03-17 8:03 ` David Gow
2022-03-23 23:51 ` Brendan Higgins
2022-03-25 0:08 ` Daniel Latypov
2022-03-23 23:52 ` Brendan Higgins
2022-03-16 21:41 kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.