Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1585313122-26441-1-git-send-email-alan.maguire@oracle.com> <1585313122-26441-2-git-send-email-alan.maguire@oracle.com>
In-Reply-To: <1585313122-26441-2-git-send-email-alan.maguire@oracle.com>
From:   Brendan Higgins <brendanhiggins@google.com>
Date:   Thu, 28 May 2020 12:40:01 -0700
Message-ID: <CAFd5g44-eDSDQqt+ZR-CibEjgZ3x6RzYf6WFQ7t088Bf2c8qRg@mail.gmail.com>
Subject: Re: [PATCH v3 kunit-next 1/2] kunit: generalize kunit_resource API
 beyond allocated resources
To:     Alan Maguire <alan.maguire@oracle.com>
Cc:     shuah <shuah@kernel.org>,
        Patricia Alfonso <trishalfonso@google.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        KUnit Development <kunit-dev@googlegroups.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Mar 27, 2020 at 5:45 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> In its original form, the kunit resources API - consisting the
> struct kunit_resource and associated functions - was focused on
> adding allocated resources during test operation that would be
> automatically cleaned up on test completion.
>
> The recent RFC patch proposing converting KASAN tests to KUnit [1]
> showed another potential model - where outside of test context,
> but with a pointer to the test state, we wish to access/update
> test-related data, but expressly want to avoid allocations.
>
> It turns out we can generalize the kunit_resource to support
> static resources where the struct kunit_resource * is passed
> in and initialized for us. As part of this work, we also
> change the "allocation" field to the more general "data" name,
> as instead of associating an allocation, we can associate a
> pointer to static data.  Static data is distinguished by a NULL
> free functions.  A test is added to cover using kunit_add_resource()
> with a static resource and data.
>
> Finally we also make use of the kernel's krefcount interfaces
> to manage reference counting of KUnit resources.  The motivation
> for this is simple; if we have kernel threads accessing and
> using resources (say via kunit_find_resource()) we need to
> ensure we do not remove said resources (or indeed free them
> if they were dynamically allocated) until the reference count
> reaches zero.  A new function - kunit_put_resource() - is
> added to handle this, and it should be called after a
> thread using kunit_find_resource() is finished with the
> retrieved resource.
>
> We ensure that the functions needed to look up, use and
> drop reference count are "static inline"-defined so that
> they can be used by builtin code as well as modules in
> the case that KUnit is built as a module.
>
> A cosmetic change here also; I've tried moving to
> kunit_[action]_resource() as the format of function names
> for consistency and readability.
>
> [1] https://lkml.org/lkml/2020/2/26/1286
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>

One comment below, other than that:

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

Sorry for not keeping up with this. I forgot that I didn't give this a
reviewed-by.

> ---
>  include/kunit/test.h      | 157 +++++++++++++++++++++++++++++++++++++---------
>  lib/kunit/kunit-test.c    |  74 ++++++++++++++++------
>  lib/kunit/string-stream.c |  14 ++---
>  lib/kunit/test.c          | 154 ++++++++++++++++++++++++---------------------
>  4 files changed, 270 insertions(+), 129 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 9b0c46a..8c7f3ff 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -52,30 +59,27 @@
>   *
>   *     static void kunit_kmalloc_free(struct kunit_resource *res)
>   *     {
> - *             kfree(res->allocation);
> + *             kfree(res->data);
>   *     }
>   *
>   *     void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp)
>   *     {
>   *             struct kunit_kmalloc_params params;
> - *             struct kunit_resource *res;
>   *
>   *             params.size = size;
>   *             params.gfp = gfp;
>   *
> - *             res = kunit_alloc_resource(test, kunit_kmalloc_init,
> + *             return kunit_alloc_resource(test, kunit_kmalloc_init,
>   *                     kunit_kmalloc_free, &params);
> - *             if (res)
> - *                     return res->allocation;
> - *
> - *             return NULL;
>   *     }
>   */
>  struct kunit_resource {
> -       void *allocation;
> -       kunit_resource_free_t free;
> +       void *data;
>
>         /* private: internal use only. */
> +       kunit_resource_init_t init;

Apologies for bringing this up so late, but it looks like you never
addressed my comment from v1:

I don't think you use this `init` field anywhere; I only see it passed
as a parameter. Is there something obvious that I am missing?

> +       kunit_resource_free_t free;
> +       struct kref refcount;
>         struct list_head node;
>  };