Re: [PATCH v9 1/9] revocable: Revocable resource management

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Tue, May 05, 2026 at 05:55:40AM -0700, Bartosz Golaszewski wrote:
> On Mon, 27 Apr 2026 15:58:33 +0200, Tzung-Bi Shih <tzungbi@kernel.org> said:
> > diff --git a/include/linux/revocable.h b/include/linux/revocable.h
> > new file mode 100644
> > index 000000000000..2bcf23f01ace
> > --- /dev/null
> > +++ b/include/linux/revocable.h
> > @@ -0,0 +1,214 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright 2026 Google LLC
> > + */
> > +
> > +#ifndef __LINUX_REVOCABLE_H
> > +#define __LINUX_REVOCABLE_H
> > +
> > +#include <linux/cleanup.h>
> > +#include <linux/compiler.h>
> 
> I don't think you need this header.

Ack, will remove it in the next version.

> 
> > +#include <linux/kref.h>
> > +#include <linux/srcu.h>
> > +
> > +/**
> > + * enum revocable_alloc_type - The allocation method for a revocable provider.
> > + * @REVOCABLE_DYNAMIC: The struct revocable was dynamically allocated using
> > + *                     revocable_alloc() and its lifetime is managed by
> > + *                     reference counting.
> > + * @REVOCABLE_EMBEDDED: The struct revocable is embedded within another
> > + *                      structure.  Its lifetime is tied to the parent
> > + *                      structure and is not reference counted.
> > + */
> > +enum revocable_alloc_type {
> > +	REVOCABLE_DYNAMIC,
> > +	REVOCABLE_EMBEDDED,
> > +};
> 
> Maybe we don't need this public enum at all, we could just use a different
> release callback for kref_put() depending on how the revocable was allocated?
> 
> The enum is not used elsewhere so it doesn't make sense to document it as if it
> was part of the revocable API.
> 
> > +
> > +/**
> > + * struct revocable - A handle for resource provider.
> > + * @srcu: The SRCU to protect the resource.
> > + * @res:  The pointer of resource.  It can point to anything.
> > + * @kref: The refcount for this handle.
> > + * @alloc_type: The memory allocation type.
> > + */
> > +struct revocable {
> > +	struct srcu_struct srcu;
> > +	void __rcu *res;
> > +	struct kref kref;
> > +	enum revocable_alloc_type alloc_type;
> 
> This could be replaced with the pointer to the release callback, assigned
> by revocable_alloc()/revocable_init() respectively.
> 

Ack, will remove the enum.  A boolean is sufficient given that the
allocation type is binary.

> > +};
> > +
> > +/**
> > + * struct revocable_consumer - A handle for resource consumer.
> > + * @rev: The pointer of resource provider.
> > + * @idx: The index for the SRCU critical section.
> 
> Should any of these be accessed directly by the user? Maybe document them
> as __private?

I don't think that is necessary.  All members in both struct revocable and
struct revocable_consumer are intended to be opaque and should not be
accessed directly by users.  However, I made them public structures
because:
- The try_access_* macros need to allocate a struct revocable_consumer
  locally.
- KUnit tests require access to these members for verification.

I can add a comment to the structure definitions noting that they should
be treated as private.  Does it make sense?

> 
> > + */
> > +struct revocable_consumer {
> > +	struct revocable *rev;
> > +	int idx;
> > +};
> 
> I'd rename it to struct revocable_handle which indicates better what it is:
> it's a handle *owned* by the consumer.

Ack, will rename it.

> 
> > +
> > +void revocable_get(struct revocable *rev);
> > +void revocable_put(struct revocable *rev);
> > +
> > +struct revocable *revocable_alloc(void *res);
> > +void revocable_revoke(struct revocable *rev);
> > +int revocable_embed_init(struct revocable *rev, void *res);
> > +void revocable_embed_destroy(struct revocable *rev);
> > +
> > +void revocable_init(struct revocable *rev, struct revocable_consumer *rc);
> > +void revocable_deinit(struct revocable_consumer *rc);
> 
> If we hid the release logic, we could drop revocable_embed_destroy() and use
> the same refcounting functions for both variants. I'd suggest the following:
> 
> For refcounting (same for both variants):
> 
> 	void revocable_get(struct revocable *rev);
> 	void revocable_put(struct revocable *rev);
> 
> For dynamic variant:
> 
> 	struct revocable *revocable_alloc(void *res);
> 
> For embedded:
> 
> 	int revocable_init(struct revocable *rev, void *res);
> 
> For handles:
> 
> 	void revocable_handle_init(struct revocable *rev, struct
> revocable_consumer *rc);
> 	void revocable_handle_deinit(struct revocable_consumer *rc);
> 
> Does it make sense?

That makes sense.  I'll fix this in the next version.