Re: [PATCH 3/3] Revert "revocable: Revocable resource management"
On Mon, Jan 26, 2026 at 02:20:24PM +0100, Johan Hovold wrote:
> On Sat, Jan 24, 2026 at 06:46:11PM +0100, Danilo Krummrich wrote:
> > On Sat Jan 24, 2026 at 6:05 PM CET, Johan Hovold wrote:
> > > The revocable implementation uses two separate abstractions, struct
> > > revocable_provider and struct revocable, in order to store the SRCU read
> > > lock index which must be passed unaltered to srcu_read_unlock() in the
> > > same context when a resource is no longer needed:
> > >
> > > struct revocable {
> > > struct revocable_provider *rp;
> > > int idx;
> > > };
> > >
> > > void *revocable_try_access(struct revocable *rev)
> > > {
> > > struct revocable_provider *rp = rev->rp;
> > >
> > > rev->idx = srcu_read_lock(&rp->srcu);
> > > return srcu_dereference(rp->res, &rp->srcu);
> > > }
> > >
> > > void revocable_withdraw_access(struct revocable *rev)
> > > {
> > > struct revocable_provider *rp = rev->rp;
> > >
> > > srcu_read_unlock(&rp->srcu, rev->idx);
> > > }
> > >
> > > Multiple threads may however share the same struct revocable and
> > > therefore potentially overwrite the SRCU index of another thread which
> > > can cause the SRCU synchronisation in revocable_provider_revoke() to
> > > never complete.
> >
> > I think the easiest fix would be to just return the index to the caller and
> > let
> > the corresponding revocable macro accessors handle it, such that it is still
> > transparent to the user.
>
> It can certainly be made to work, but it will be a very different API
> since there would no longer be any need for the non-intuitive
> revocable_provider and revocable split.
Thank you both for the suggestions. I'll try it out.
Re: [PATCH 3/3] Revert "revocable: Revocable resource management"
On Sat, Jan 24, 2026 at 06:46:11PM +0100, Danilo Krummrich wrote:
> On Sat Jan 24, 2026 at 6:05 PM CET, Johan Hovold wrote:
> > The revocable implementation uses two separate abstractions, struct
> > revocable_provider and struct revocable, in order to store the SRCU read
> > lock index which must be passed unaltered to srcu_read_unlock() in the
> > same context when a resource is no longer needed:
> >
> > struct revocable {
> > struct revocable_provider *rp;
> > int idx;
> > };
> >
> > void *revocable_try_access(struct revocable *rev)
> > {
> > struct revocable_provider *rp = rev->rp;
> >
> > rev->idx = srcu_read_lock(&rp->srcu);
> > return srcu_dereference(rp->res, &rp->srcu);
> > }
> >
> > void revocable_withdraw_access(struct revocable *rev)
> > {
> > struct revocable_provider *rp = rev->rp;
> >
> > srcu_read_unlock(&rp->srcu, rev->idx);
> > }
> >
> > Multiple threads may however share the same struct revocable and
> > therefore potentially overwrite the SRCU index of another thread which
> > can cause the SRCU synchronisation in revocable_provider_revoke() to
> > never complete.
>
> I think the easiest fix would be to just return the index to the caller and
> let
> the corresponding revocable macro accessors handle it, such that it is still
> transparent to the user.
It can certainly be made to work, but it will be a very different API
since there would no longer be any need for the non-intuitive
revocable_provider and revocable split.
And that's not the kind of change that should be done incrementally
as that makes it much harder to review (and it affects all of the
current code; api, implementation, docs, tests).
Johan
Re: [PATCH 3/3] Revert "revocable: Revocable resource management"
On Sat Jan 24, 2026 at 6:05 PM CET, Johan Hovold wrote:
> The revocable implementation uses two separate abstractions, struct
> revocable_provider and struct revocable, in order to store the SRCU read
> lock index which must be passed unaltered to srcu_read_unlock() in the
> same context when a resource is no longer needed:
>
> struct revocable {
> struct revocable_provider *rp;
> int idx;
> };
>
> void *revocable_try_access(struct revocable *rev)
> {
> struct revocable_provider *rp = rev->rp;
>
> rev->idx = srcu_read_lock(&rp->srcu);
> return srcu_dereference(rp->res, &rp->srcu);
> }
>
> void revocable_withdraw_access(struct revocable *rev)
> {
> struct revocable_provider *rp = rev->rp;
>
> srcu_read_unlock(&rp->srcu, rev->idx);
> }
>
> Multiple threads may however share the same struct revocable and
> therefore potentially overwrite the SRCU index of another thread which
> can cause the SRCU synchronisation in revocable_provider_revoke() to
> never complete.
I think the easiest fix would be to just return the index to the caller and let
the corresponding revocable macro accessors handle it, such that it is still
transparent to the user.
Re: [PATCH 3/3] Revert "revocable: Revocable resource management"
On Sat, Jan 24, 2026 at 06:05:35PM +0100, Johan Hovold wrote: > This reverts commit 2da75b5b72559e5214a24aaf7ac933bb5bc2f1fe. I forgot to update the commit ids before posting. This was supposed to say: 62eb557580eb2177cf16c3fd2b6efadff297b29a and similar for patch 1 and 2: 9d4502fef00fa7a798d3c0806d4da4466a7ffc6f cd7693419bb5abd91ad2f407dab69c480e417a61 Sorry about that. Johan
