Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220326002013.483394-1-dlatypov@google.com> <20220326002013.483394-2-dlatypov@google.com>
In-Reply-To: <20220326002013.483394-2-dlatypov@google.com>
From:   David Gow <davidgow@google.com>
Date:   Sat, 26 Mar 2022 12:27:46 +0800
Message-ID: <CABVgOSm+o=t=8hgR=T676rHJjWp0p-VXeoZt28G0kVqURqCGMg@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] kunit: split resource API impl from test.c into
 new resource.c
To:     Daniel Latypov <dlatypov@google.com>
Cc:     Brendan Higgins <brendanhiggins@google.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        KUnit Development <kunit-dev@googlegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Mar 26, 2022 at 8:20 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> We've split out the declarations from include/kunit/test.h into
> resource.h.
> This patch splits out the definitions as well for consistency.
>
> A side effect of this is git blame won't properly track history by
> default, users need to run
> $ git blame -L ,1 -C13 lib/kunit/resource.c
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

Looks good and works fine here. I'm going to try to rebase most of the
other resource system stuff I'm working on on top of these (which will
likely end up moving a bunch of code _again_, but is probably the
least terrible of all the available options).

One nitpick (newline at end of file) below, otherwise this is good.

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

>  lib/kunit/Makefile   |   1 +
>  lib/kunit/resource.c | 126 +++++++++++++++++++++++++++++++++++++++++++
>  lib/kunit/test.c     | 116 +--------------------------------------
>  3 files changed, 128 insertions(+), 115 deletions(-)
>  create mode 100644 lib/kunit/resource.c
>
> diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
> index c49f4ffb6273..29aff6562b42 100644
> --- a/lib/kunit/Makefile
> +++ b/lib/kunit/Makefile
> @@ -1,6 +1,7 @@
>  obj-$(CONFIG_KUNIT) +=                 kunit.o
>
>  kunit-objs +=                          test.o \
> +                                       resource.o \
>                                         string-stream.o \
>                                         assert.o \
>                                         try-catch.o \
> diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c
> new file mode 100644
> index 000000000000..b8bced246217
> --- /dev/null
> +++ b/lib/kunit/resource.c
> @@ -0,0 +1,126 @@
> +// 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>
> + */
> +
> +#include <kunit/resource.h>
> +#include <kunit/test.h>
> +#include <linux/kref.h>
> +
> +/*
> + * Used for static resources and when a kunit_resource * has been created by
> + * kunit_alloc_resource().  When an init function is supplied, @data is passed
> + * into the init function; otherwise, we simply set the resource data field to
> + * the data value passed in.
> + */
> +int kunit_add_resource(struct kunit *test,
> +                      kunit_resource_init_t init,
> +                      kunit_resource_free_t free,
> +                      struct kunit_resource *res,
> +                      void *data)
> +{
> +       int ret = 0;
> +       unsigned long flags;
> +
> +       res->free = free;
> +       kref_init(&res->refcount);
> +
> +       if (init) {
> +               ret = init(res, data);
> +               if (ret)
> +                       return ret;
> +       } else {
> +               res->data = data;
> +       }
> +
> +       spin_lock_irqsave(&test->lock, flags);
> +       list_add_tail(&res->node, &test->resources);
> +       /* refcount for list is established by kref_init() */
> +       spin_unlock_irqrestore(&test->lock, flags);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(kunit_add_resource);
> +
> +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)
> +{
> +       struct kunit_resource *existing;
> +
> +       if (!name)
> +               return -EINVAL;
> +
> +       existing = kunit_find_named_resource(test, name);
> +       if (existing) {
> +               kunit_put_resource(existing);
> +               return -EEXIST;
> +       }
> +
> +       res->name = name;
> +
> +       return kunit_add_resource(test, init, free, res, data);
> +}
> +EXPORT_SYMBOL_GPL(kunit_add_named_resource);
> +
> +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 *data)
> +{
> +       struct kunit_resource *res;
> +       int ret;
> +
> +       res = kzalloc(sizeof(*res), internal_gfp);
> +       if (!res)
> +               return NULL;
> +
> +       ret = kunit_add_resource(test, init, free, res, data);
> +       if (!ret) {
> +               /*
> +                * bump refcount for get; kunit_resource_put() should be called
> +                * when done.
> +                */
> +               kunit_get_resource(res);
> +               return res;
> +       }
> +       return NULL;
> +}
> +EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource);
> +
> +void kunit_remove_resource(struct kunit *test, struct kunit_resource *res)
> +{
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&test->lock, flags);
> +       list_del(&res->node);
> +       spin_unlock_irqrestore(&test->lock, flags);
> +       kunit_put_resource(res);
> +}
> +EXPORT_SYMBOL_GPL(kunit_remove_resource);
> +
> +int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match,
> +                          void *match_data)
> +{
> +       struct kunit_resource *res = kunit_find_resource(test, match,
> +                                                        match_data);
> +
> +       if (!res)
> +               return -ENOENT;
> +
> +       kunit_remove_resource(test, res);
> +
> +       /* We have a reference also via _find(); drop it. */
> +       kunit_put_resource(res);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(kunit_destroy_resource);
> +

.git/rebase-apply/patch:151: new blank line at EOF.

+

> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 3bca3bf5c15b..0f66c13d126e 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -6,10 +6,10 @@
>   * Author: Brendan Higgins <brendanhiggins@google.com>
>   */
>
> +#include <kunit/resource.h>
>  #include <kunit/test.h>
>  #include <kunit/test-bug.h>
>  #include <linux/kernel.h>
> -#include <linux/kref.h>
>  #include <linux/moduleparam.h>
>  #include <linux/sched/debug.h>
>  #include <linux/sched.h>
> @@ -592,120 +592,6 @@ void __kunit_test_suites_exit(struct kunit_suite **suites)
>  }
>  EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);
>
> -/*
> - * Used for static resources and when a kunit_resource * has been created by
> - * kunit_alloc_resource().  When an init function is supplied, @data is passed
> - * into the init function; otherwise, we simply set the resource data field to
> - * the data value passed in.
> - */
> -int kunit_add_resource(struct kunit *test,
> -                      kunit_resource_init_t init,
> -                      kunit_resource_free_t free,
> -                      struct kunit_resource *res,
> -                      void *data)
> -{
> -       int ret = 0;
> -       unsigned long flags;
> -
> -       res->free = free;
> -       kref_init(&res->refcount);
> -
> -       if (init) {
> -               ret = init(res, data);
> -               if (ret)
> -                       return ret;
> -       } else {
> -               res->data = data;
> -       }
> -
> -       spin_lock_irqsave(&test->lock, flags);
> -       list_add_tail(&res->node, &test->resources);
> -       /* refcount for list is established by kref_init() */
> -       spin_unlock_irqrestore(&test->lock, flags);
> -
> -       return ret;
> -}
> -EXPORT_SYMBOL_GPL(kunit_add_resource);
> -
> -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)
> -{
> -       struct kunit_resource *existing;
> -
> -       if (!name)
> -               return -EINVAL;
> -
> -       existing = kunit_find_named_resource(test, name);
> -       if (existing) {
> -               kunit_put_resource(existing);
> -               return -EEXIST;
> -       }
> -
> -       res->name = name;
> -
> -       return kunit_add_resource(test, init, free, res, data);
> -}
> -EXPORT_SYMBOL_GPL(kunit_add_named_resource);
> -
> -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 *data)
> -{
> -       struct kunit_resource *res;
> -       int ret;
> -
> -       res = kzalloc(sizeof(*res), internal_gfp);
> -       if (!res)
> -               return NULL;
> -
> -       ret = kunit_add_resource(test, init, free, res, data);
> -       if (!ret) {
> -               /*
> -                * bump refcount for get; kunit_resource_put() should be called
> -                * when done.
> -                */
> -               kunit_get_resource(res);
> -               return res;
> -       }
> -       return NULL;
> -}
> -EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource);
> -
> -void kunit_remove_resource(struct kunit *test, struct kunit_resource *res)
> -{
> -       unsigned long flags;
> -
> -       spin_lock_irqsave(&test->lock, flags);
> -       list_del(&res->node);
> -       spin_unlock_irqrestore(&test->lock, flags);
> -       kunit_put_resource(res);
> -}
> -EXPORT_SYMBOL_GPL(kunit_remove_resource);
> -
> -int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match,
> -                          void *match_data)
> -{
> -       struct kunit_resource *res = kunit_find_resource(test, match,
> -                                                        match_data);
> -
> -       if (!res)
> -               return -ENOENT;
> -
> -       kunit_remove_resource(test, res);
> -
> -       /* We have a reference also via _find(); drop it. */
> -       kunit_put_resource(res);
> -
> -       return 0;
> -}
> -EXPORT_SYMBOL_GPL(kunit_destroy_resource);
> -
>  struct kunit_kmalloc_array_params {
>         size_t n;
>         size_t size;
> --
> 2.35.1.1021.g381101b075-goog
>