Add in: * kunit_kmalloc_array() and wire up kunit_kmalloc() to be a special case of it. * kunit_kcalloc() for symmetry with kunit_kzalloc()
This should using KUnit more natural by making it more similar to the existing *alloc() APIs.
And while we shouldn't necessarily be writing unit tests where overflow should be a concern, it can't hurt to be safe.
Signed-off-by: Daniel Latypov dlatypov@google.com --- include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++---- lib/kunit/test.c | 22 ++++++++++++---------- 2 files changed, 44 insertions(+), 14 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 49601c4b98b8..7fa0de4af977 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -577,16 +577,30 @@ static inline int kunit_destroy_named_resource(struct kunit *test, void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
/** - * kunit_kmalloc() - Like kmalloc() except the allocation is *test managed*. + * kunit_kmalloc_array() - Like kmalloc_array() except the allocation is *test managed*. * @test: The test context object. + * @n: number of elements. * @size: The size in bytes of the desired memory. * @gfp: flags passed to underlying kmalloc(). * - * Just like `kmalloc(...)`, except the allocation is managed by the test case + * 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. */ -void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp); +void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t flags); + +/** + * kunit_kmalloc() - Like kmalloc() except the allocation is *test managed*. + * @test: The test context object. + * @size: The size in bytes of the desired memory. + * @gfp: flags passed to underlying kmalloc(). + * + * See kmalloc() and kunit_kmalloc_array() for more information. + */ +static inline void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp) +{ + return kunit_kmalloc_array(test, 1, size, gfp); +}
/** * kunit_kfree() - Like kfree except for allocations managed by KUnit. @@ -601,13 +615,27 @@ void kunit_kfree(struct kunit *test, const void *ptr); * @size: The size in bytes of the desired memory. * @gfp: flags passed to underlying kmalloc(). * - * See kzalloc() and kunit_kmalloc() for more information. + * See kzalloc() and kunit_kmalloc_array() for more information. */ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp) { return kunit_kmalloc(test, size, gfp | __GFP_ZERO); }
+/** + * kunit_kzalloc() - Just like kunit_kmalloc_array(), but zeroes the allocation. + * @test: The test context object. + * @n: number of elements. + * @size: The size in bytes of the desired memory. + * @gfp: flags passed to underlying kmalloc(). + * + * See kcalloc() and kunit_kmalloc_array() for more information. + */ +static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp_t flags) +{ + return kunit_kmalloc_array(test, n, size, flags | __GFP_ZERO); +} + void kunit_cleanup(struct kunit *test);
void kunit_log_append(char *log, const char *fmt, ...); diff --git a/lib/kunit/test.c b/lib/kunit/test.c index ec9494e914ef..052fccf69eef 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -540,41 +540,43 @@ int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match, } EXPORT_SYMBOL_GPL(kunit_destroy_resource);
-struct kunit_kmalloc_params { +struct kunit_kmalloc_array_params { + size_t n; size_t size; gfp_t gfp; };
-static int kunit_kmalloc_init(struct kunit_resource *res, void *context) +static int kunit_kmalloc_array_init(struct kunit_resource *res, void *context) { - struct kunit_kmalloc_params *params = context; + struct kunit_kmalloc_array_params *params = context;
- res->data = kmalloc(params->size, params->gfp); + res->data = kmalloc_array(params->n, params->size, params->gfp); if (!res->data) return -ENOMEM;
return 0; }
-static void kunit_kmalloc_free(struct kunit_resource *res) +static void kunit_kmalloc_array_free(struct kunit_resource *res) { kfree(res->data); }
-void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp) +void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t gfp) { - struct kunit_kmalloc_params params = { + struct kunit_kmalloc_array_params params = { .size = size, + .n = n, .gfp = gfp };
return kunit_alloc_resource(test, - kunit_kmalloc_init, - kunit_kmalloc_free, + kunit_kmalloc_array_init, + kunit_kmalloc_array_free, gfp, ¶ms); } -EXPORT_SYMBOL_GPL(kunit_kmalloc); +EXPORT_SYMBOL_GPL(kunit_kmalloc_array);
void kunit_kfree(struct kunit *test, const void *ptr) {
base-commit: 16fc44d6387e260f4932e9248b985837324705d8
On Thu, Apr 22, 2021 at 2:32 AM Daniel Latypov dlatypov@google.com wrote:
Add in:
- kunit_kmalloc_array() and wire up kunit_kmalloc() to be a special
case of it.
- kunit_kcalloc() for symmetry with kunit_kzalloc()
This should using KUnit more natural by making it more similar to the existing *alloc() APIs.
And while we shouldn't necessarily be writing unit tests where overflow should be a concern, it can't hurt to be safe.
Signed-off-by: Daniel Latypov dlatypov@google.com
This seems like a good addition to me: a bug and a couple of useless asides below.
Apart from the "kzalloc"/"kcalloc" confusion in the comment below, this is Reviewed-by: David Gow davidgow@google.com
-- David
include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++---- lib/kunit/test.c | 22 ++++++++++++---------- 2 files changed, 44 insertions(+), 14 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 49601c4b98b8..7fa0de4af977 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -577,16 +577,30 @@ static inline int kunit_destroy_named_resource(struct kunit *test, void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
/**
- kunit_kmalloc() - Like kmalloc() except the allocation is *test managed*.
- kunit_kmalloc_array() - Like kmalloc_array() except the allocation is *test managed*.
- @test: The test context object.
- @n: number of elements.
- @size: The size in bytes of the desired memory.
- @gfp: flags passed to underlying kmalloc().
- Just like `kmalloc(...)`, except the allocation is managed by the test case
*/
- 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.
-void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp); +void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t flags);
+/**
- kunit_kmalloc() - Like kmalloc() except the allocation is *test managed*.
- @test: The test context object.
- @size: The size in bytes of the desired memory.
- @gfp: flags passed to underlying kmalloc().
- See kmalloc() and kunit_kmalloc_array() for more information.
- */
+static inline void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp) +{
return kunit_kmalloc_array(test, 1, size, gfp);
Do we want to implement kunit_kmalloc() in terms of kunit_kmalloc_array()? It's interestingly backwards given that kmalloc_array() is implemented in terms of kmalloc(). The other option would be to have each kunit_* function wrap the actual function that's called, but that would introduce a lot of code duplication for a very small performance benefit.
I'm happy with it the way it is now that I've looked through the implementations, but I was a little uneasy at first that some of these functions might not actually call the function they're theoretically wrapping.
+}
/**
- kunit_kfree() - Like kfree except for allocations managed by KUnit.
@@ -601,13 +615,27 @@ void kunit_kfree(struct kunit *test, const void *ptr);
- @size: The size in bytes of the desired memory.
- @gfp: flags passed to underlying kmalloc().
- See kzalloc() and kunit_kmalloc() for more information.
*/
- See kzalloc() and kunit_kmalloc_array() for more information.
static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp) { return kunit_kmalloc(test, size, gfp | __GFP_ZERO); }
+/**
- kunit_kzalloc() - Just like kunit_kmalloc_array(), but zeroes the allocation.
The function is called kunit_kcalloc(), but the documentation comment calls it kunit_kzalloc(). Copy + paste error from above?
- @test: The test context object.
- @n: number of elements.
- @size: The size in bytes of the desired memory.
- @gfp: flags passed to underlying kmalloc().
- See kcalloc() and kunit_kmalloc_array() for more information.
- */
+static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp_t flags) +{
return kunit_kmalloc_array(test, n, size, flags | __GFP_ZERO);+}
void kunit_cleanup(struct kunit *test);
void kunit_log_append(char *log, const char *fmt, ...); diff --git a/lib/kunit/test.c b/lib/kunit/test.c index ec9494e914ef..052fccf69eef 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -540,41 +540,43 @@ int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match, } EXPORT_SYMBOL_GPL(kunit_destroy_resource);
-struct kunit_kmalloc_params { +struct kunit_kmalloc_array_params {
size_t n;
It's worth noting that we never actually use this after the resource is created. kmalloc_array() discards 'n' after the overflow check and multiply anyway.
Of course, we don't need 'size' either, and we were already tracking that. I guess it's just an overhead of the resource system, so nothing worth actually changing here.
size_t size; gfp_t gfp;};
-static int kunit_kmalloc_init(struct kunit_resource *res, void *context) +static int kunit_kmalloc_array_init(struct kunit_resource *res, void *context) {
struct kunit_kmalloc_params *params = context;
struct kunit_kmalloc_array_params *params = context;
res->data = kmalloc(params->size, params->gfp);
res->data = kmalloc_array(params->n, params->size, params->gfp); if (!res->data) return -ENOMEM; return 0;}
-static void kunit_kmalloc_free(struct kunit_resource *res) +static void kunit_kmalloc_array_free(struct kunit_resource *res) { kfree(res->data); }
-void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp) +void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t gfp) {
struct kunit_kmalloc_params params = {
struct kunit_kmalloc_array_params params = { .size = size,.n = n, .gfp = gfp }; return kunit_alloc_resource(test,
kunit_kmalloc_init,kunit_kmalloc_free,
kunit_kmalloc_array_init,kunit_kmalloc_array_free, gfp, ¶ms);} -EXPORT_SYMBOL_GPL(kunit_kmalloc); +EXPORT_SYMBOL_GPL(kunit_kmalloc_array);
void kunit_kfree(struct kunit *test, const void *ptr) {
base-commit: 16fc44d6387e260f4932e9248b985837324705d8
2.31.1.498.g6c1eba8ee3d-goog
On Wed, Apr 21, 2021 at 10:52 PM David Gow davidgow@google.com wrote:
On Thu, Apr 22, 2021 at 2:32 AM Daniel Latypov dlatypov@google.com wrote:
Add in:
- kunit_kmalloc_array() and wire up kunit_kmalloc() to be a special
case of it.
- kunit_kcalloc() for symmetry with kunit_kzalloc()
This should using KUnit more natural by making it more similar to the existing *alloc() APIs.
And while we shouldn't necessarily be writing unit tests where overflow should be a concern, it can't hurt to be safe.
Signed-off-by: Daniel Latypov dlatypov@google.com
This seems like a good addition to me: a bug and a couple of useless asides below.
"bug" = the kzalloc/kcalloc mixup in doc comments? Not sure if I'm overlooking one of your replies.
Apart from the "kzalloc"/"kcalloc" confusion in the comment below, this is Reviewed-by: David Gow davidgow@google.com
-- David
include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++---- lib/kunit/test.c | 22 ++++++++++++---------- 2 files changed, 44 insertions(+), 14 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 49601c4b98b8..7fa0de4af977 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -577,16 +577,30 @@ static inline int kunit_destroy_named_resource(struct kunit *test, void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
/**
- kunit_kmalloc() - Like kmalloc() except the allocation is *test managed*.
- kunit_kmalloc_array() - Like kmalloc_array() except the allocation is *test managed*.
- @test: The test context object.
- @n: number of elements.
- @size: The size in bytes of the desired memory.
- @gfp: flags passed to underlying kmalloc().
- Just like `kmalloc(...)`, except the allocation is managed by the test case
*/
- 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.
-void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp); +void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t flags);
+/**
- kunit_kmalloc() - Like kmalloc() except the allocation is *test managed*.
- @test: The test context object.
- @size: The size in bytes of the desired memory.
- @gfp: flags passed to underlying kmalloc().
- See kmalloc() and kunit_kmalloc_array() for more information.
- */
+static inline void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp) +{
return kunit_kmalloc_array(test, 1, size, gfp);Do we want to implement kunit_kmalloc() in terms of kunit_kmalloc_array()? It's interestingly backwards given that kmalloc_array() is implemented in terms of kmalloc().
I didn't want to have the checked multiply in test.c, and figured the little bit of extra indirection probably won't hurt. (Insert hopes about sufficiently smart compilers optimizing out some of it).
The other option would be to have each kunit_* function wrap the actual function that's called, but that would introduce a lot of code duplication for a very small performance benefit.
I'm happy with it the way it is now that I've looked through the implementations, but I was a little uneasy at first that some of these functions might not actually call the function they're theoretically wrapping.
+}
/**
- kunit_kfree() - Like kfree except for allocations managed by KUnit.
@@ -601,13 +615,27 @@ void kunit_kfree(struct kunit *test, const void *ptr);
- @size: The size in bytes of the desired memory.
- @gfp: flags passed to underlying kmalloc().
- See kzalloc() and kunit_kmalloc() for more information.
*/
- See kzalloc() and kunit_kmalloc_array() for more information.
static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp) { return kunit_kmalloc(test, size, gfp | __GFP_ZERO); }
+/**
- kunit_kzalloc() - Just like kunit_kmalloc_array(), but zeroes the allocation.
The function is called kunit_kcalloc(), but the documentation comment calls it kunit_kzalloc(). Copy + paste error from above?
Good catch, fixed.
- @test: The test context object.
- @n: number of elements.
- @size: The size in bytes of the desired memory.
- @gfp: flags passed to underlying kmalloc().
- See kcalloc() and kunit_kmalloc_array() for more information.
- */
+static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp_t flags) +{
return kunit_kmalloc_array(test, n, size, flags | __GFP_ZERO);+}
void kunit_cleanup(struct kunit *test);
void kunit_log_append(char *log, const char *fmt, ...); diff --git a/lib/kunit/test.c b/lib/kunit/test.c index ec9494e914ef..052fccf69eef 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -540,41 +540,43 @@ int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match, } EXPORT_SYMBOL_GPL(kunit_destroy_resource);
-struct kunit_kmalloc_params { +struct kunit_kmalloc_array_params {
size_t n;It's worth noting that we never actually use this after the resource is created. kmalloc_array() discards 'n' after the overflow check and multiply anyway.
Of course, we don't need 'size' either, and we were already tracking that. I guess it's just an overhead of the resource system, so nothing worth actually changing here.
Ack, yeah, we don't need any of these params after the allocation happens, including gfp. But we're not "tracking it:, I don't think?
The _free() function calls `kfree(res->data)`, so we're only keeping around the pointer itself while the resource is alive. The context object is only around for the init call.
It's also an object on the stack that goes out of scope after we return from kunit_kmalloc_array(), so I think we'd have a bug if we were trying to keep it around.
size_t size; gfp_t gfp;};
-static int kunit_kmalloc_init(struct kunit_resource *res, void *context) +static int kunit_kmalloc_array_init(struct kunit_resource *res, void *context) {
struct kunit_kmalloc_params *params = context;
struct kunit_kmalloc_array_params *params = context;
res->data = kmalloc(params->size, params->gfp);
res->data = kmalloc_array(params->n, params->size, params->gfp); if (!res->data) return -ENOMEM; return 0;}
-static void kunit_kmalloc_free(struct kunit_resource *res) +static void kunit_kmalloc_array_free(struct kunit_resource *res) { kfree(res->data); }
-void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp) +void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t gfp) {
struct kunit_kmalloc_params params = {
struct kunit_kmalloc_array_params params = { .size = size,.n = n, .gfp = gfp }; return kunit_alloc_resource(test,
kunit_kmalloc_init,kunit_kmalloc_free,
kunit_kmalloc_array_init,kunit_kmalloc_array_free, gfp, ¶ms);} -EXPORT_SYMBOL_GPL(kunit_kmalloc); +EXPORT_SYMBOL_GPL(kunit_kmalloc_array);
void kunit_kfree(struct kunit *test, const void *ptr) {
base-commit: 16fc44d6387e260f4932e9248b985837324705d8
2.31.1.498.g6c1eba8ee3d-goog
On Wed, Apr 21, 2021 at 11:32 AM Daniel Latypov dlatypov@google.com wrote:
Add in:
- kunit_kmalloc_array() and wire up kunit_kmalloc() to be a special
case of it.
- kunit_kcalloc() for symmetry with kunit_kzalloc()
This should using KUnit more natural by making it more similar to the existing *alloc() APIs.
And while we shouldn't necessarily be writing unit tests where overflow should be a concern, it can't hurt to be safe.
Signed-off-by: Daniel Latypov dlatypov@google.com
Aside from the copy and paste issue that David already pointed out, LGTM.
Reviewed-by: Brendan Higgins brendanhiggins@google.com
linux-kselftest-mirror@lists.linaro.org
-
Brendan Higgins -
Daniel Latypov -
David Gow