Re: [PATCH v3 1/5] revocable: Revocable resource management

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

On Fri, Sep 12, 2025 at 11:05:20AM +0200, Danilo Krummrich wrote:
> On Fri Sep 12, 2025 at 10:17 AM CEST, Tzung-Bi Shih wrote:
> > +/**
> > + * struct revocable_provider - 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.
> > + */
> > +struct revocable_provider {
> > +	struct srcu_struct srcu;
> > +	void __rcu *res;
> > +	struct kref kref;
> > +};
> 
> I think a revocable provider should provide an optional revoke() callback where
> users of the revocable provider can release the revoked resource.
>
> But this can also be done as a follow-up.

Understood.  Since this effectively delegates the memory of `res` to the
struct revocable provider, I propose we name the callback .release().

> > +/**
> > + * struct revocable - A handle for resource consumer.
> > + * @rp: The pointer of resource provider.
> > + * @idx: The index for the RCU critical section.
> > + */
> > +struct revocable {
> > +	struct revocable_provider *rp;
> > +	int idx;
> > +};
> 
> I think I asked about this in the previous version (but I don't remember if
> there was an answer):

Yes, in v1 https://lore.kernel.org/chrome-platform/aJ7HUJ0boqYndbtD@google.com/.

> In Rust we get away with a single Revocable<T> structure, but we're using RCU
> instead of SRCU. It seems to me that the split between struct revocable and
> struct revocable_provider is only for the SRCU index? Or is there any other
> reason?

Yes, for the SRCU index.

> > +/**
> > + * revocable_provider_free() - Free struct revocable_provider.
> > + * @rp: The pointer of resource provider.
> > + *
> > + * This sets the resource `(struct revocable_provider *)->res` to NULL to
> > + * indicate the resource has gone.
> > + *
> > + * This drops the refcount to the resource provider.  If it is the final
> > + * reference, revocable_provider_release() will be called to free the struct.
> > + */
> > +void revocable_provider_free(struct revocable_provider *rp)
> > +{
> > +	rcu_assign_pointer(rp->res, NULL);
> > +	synchronize_srcu(&rp->srcu);
> > +	kref_put(&rp->kref, revocable_provider_release);
> > +}
> > +EXPORT_SYMBOL_GPL(revocable_provider_free);
> 
> I think naming this "free" is a bit misleading, since what it basically does is
> 
>   (1) Revoke access to the resource.
> 
>   (2) Drop a reference count of struct revocable_provider.
> 
> In a typical application there may still be struct revocable instances that have
> a reference to the provider, so we can't claim that it's freed here.
> 
> So, given that, I'd rather call this revocable_provider_revoke().

Ack, will fix it in the next version.

> > +static void devm_revocable_provider_free(void *data)
> > +{
> > +	struct revocable_provider *rp = data;
> > +
> > +	revocable_provider_free(rp);
> > +}
> 
> Same here, I'd call this devm_revocable_provider_revoke().

Ack, will fix it in the next version.

> > +DEFINE_FREE(revocable, struct revocable *, if (_T) revocable_release(_T))
> > +
> > +#define _REVOCABLE(_rev, _label, _res)						\
> > +	for (struct revocable *__UNIQUE_ID(name) __free(revocable) = _rev;	\
> > +	     (_res = revocable_try_access(_rev)) || true; ({ goto _label; }))	\
> > +		if (0) {							\
> > +_label:										\
> > +			break;							\
> > +		} else
> > +
> > +#define REVOCABLE(_rev, _res) _REVOCABLE(_rev, __UNIQUE_ID(label), _res)
> 
> This is basically the same as Revocable::try_access_with() [1] in Rust, i.e.
> try to access and run a closure.
> 
> Admittedly, REVOCABLE_TRY_ACCESS_WITH() is pretty verbose and I also do not have
> a great idea to shorten it.
> 
> Maybe you have a good idea, otherwise I'm also fine with the current name.
> 
> Otherwise, maybe it's worth to link to the Rust Revocable API for reference?

No, I don't have a good idea either.  Will use REVOCABLE_TRY_ACCESS_WITH()
to align with Rust Revocable API in the next version.