On Thu, Apr 22, 2021 at 2:32 AM Daniel Latypov <dlatypov@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
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@xxxxxxxxxx>
-- 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
>
