Re: [PATCH] kunit: Rework kunit_resource allocation policy

[Daniel Latypov]

On Wed, Mar 23, 2022 at 2:22 AM Brendan Higgins
<brendanhiggins@xxxxxxxxxx> wrote:
>

<snip>

>
> > The latter ~never need to get "found" (e.g. kunit_kmalloc() users).
> > The one exception: when people use kunit_kfree() to free things early,
> > which requires us to "find" these resources we otherwise wouldn't care
> > about.
> >
> > So I don't know how we can split the API unless we get rid of kunit_kfree().
> > Its presence means kunit_kmalloc() and friends need refcounting.
>
> Do we need to choose between dropping kunit_kfree() and refcounting? I
> think this is semantically different from other findable resources,
> and I think it fairly obviously entails the complexity of using it.

Yes, they're different.
We could do something different and just have a atomic bool "is_freed"
for the kunit_kmalloc() style resources.

But is it worth it?

Currently kunit_kfree() is defined as
697:void kunit_kfree(struct kunit *test, const void *ptr)
698-{
699-    struct kunit_resource *res;
700-
701-    res = kunit_find_resource(test, kunit_resource_instance_match,
702-                              (void *)ptr);
703-
704-    /*
705-     * Removing the resource from the list of resources drops the
706-     * reference count to 1; the final put will trigger the free.
707-     */
708-    kunit_remove_resource(test, res);
709-
710-    kunit_put_resource(res);
711-
712-}

i.e. the overhead of using a refcount is that we need to call
kunit_put_resource() bc we called kunit_find_resource().
IMO, this less semantic overhead than adding a different mechanism
specifically for kunit_kfree().

Tangent:
Huh, it segfaults if you call kunit_kfree() on a non-kunit allocated ptr.
res == NULL on 701 in that case, but kunit_remove_resource() doesn't
guard against that.
It also happens if you call kunit_free() twice.

That's analogous to how kfree() works, so I guess that's fine.
A difference though is
  kfree(NULL); // is fine
  kunit_free(test, NULL); // segfaults, res == NULL above

But thinking on it more, someone could register a resource w/ data == NULL.
I.e. a named resource which just acts as a flag via presence/absence.
kunit_kfree(test, NULL) would the most recent such resource though.

Should we do the trick/hack where we check the free function first in
kunit_kfree() to avoid such confusion?