* [PATCH 1/2] kunit: introduce kunit_kmalloc_array/kunit_kcalloc() helpers @ 2021-04-21 18:32 Daniel Latypov 2021-04-22 5:52 ` David Gow 2021-04-22 20:02 ` Brendan Higgins 0 siblings, 2 replies; 4+ messages in thread From: Daniel Latypov @ 2021-04-21 18:32 UTC (permalink / raw) To: brendanhiggins, davidgow Cc: linux-kernel, kunit-dev, linux-kselftest, skhan, Daniel Latypov 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 -- 2.31.1.498.g6c1eba8ee3d-goog ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] kunit: introduce kunit_kmalloc_array/kunit_kcalloc() helpers 2021-04-21 18:32 [PATCH 1/2] kunit: introduce kunit_kmalloc_array/kunit_kcalloc() helpers Daniel Latypov @ 2021-04-22 5:52 ` David Gow 2021-04-22 19:26 ` Daniel Latypov 2021-04-22 20:02 ` Brendan Higgins 1 sibling, 1 reply; 4+ messages in thread From: David Gow @ 2021-04-22 5:52 UTC (permalink / raw) To: Daniel Latypov Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] kunit: introduce kunit_kmalloc_array/kunit_kcalloc() helpers 2021-04-22 5:52 ` David Gow @ 2021-04-22 19:26 ` Daniel Latypov 0 siblings, 0 replies; 4+ messages in thread From: Daniel Latypov @ 2021-04-22 19:26 UTC (permalink / raw) To: David Gow Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan 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 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] kunit: introduce kunit_kmalloc_array/kunit_kcalloc() helpers 2021-04-21 18:32 [PATCH 1/2] kunit: introduce kunit_kmalloc_array/kunit_kcalloc() helpers Daniel Latypov 2021-04-22 5:52 ` David Gow @ 2021-04-22 20:02 ` Brendan Higgins 1 sibling, 0 replies; 4+ messages in thread From: Brendan Higgins @ 2021-04-22 20:02 UTC (permalink / raw) To: Daniel Latypov Cc: David Gow, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan 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> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-22 20:03 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-21 18:32 [PATCH 1/2] kunit: introduce kunit_kmalloc_array/kunit_kcalloc() helpers Daniel Latypov 2021-04-22 5:52 ` David Gow 2021-04-22 19:26 ` Daniel Latypov 2021-04-22 20:02 ` Brendan Higgins
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).