Message-ID: <CAGS_qxr16JNasdZvqtBOcUZKhadWN8U85WHTWTM=iFxQsqWTpA@mail.gmail.com>
Date:   Thu, 22 Apr 2021 12:26:44 -0700
From:   Daniel Latypov <dlatypov@...gle.com>
To:     David Gow <davidgow@...gle.com>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH 1/2] kunit: introduce kunit_kmalloc_array/kunit_kcalloc() helpers

On Wed, Apr 21, 2021 at 10:52 PM David Gow <davidgow@...gle.com> wrote:
>
> On Thu, Apr 22, 2021 at 2:32 AM Daniel Latypov <dlatypov@...gle.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@...gle.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@...gle.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,
> >                                     &params);
> >  }
> > -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
> >

Powered by blists - more mailing lists