Hi!
On Fri 06-11-26 15:30:07, Bence Csókás wrote:
> On 2026. 11. 06. 15:13, Jan Kara wrote:
> > Hi!
> >
> > On Fri 06-11-26 10:31:58, Bence Csókás wrote:
> > > On 2026. 11. 06. 10:05, Christian Brauner wrote:
> > > > On Tue, Oct 27, 2026 at 05:22:00PM +0100, Bence Csokas wrote:
> > > > > Add __free() support to eventfd and update drm_syncobj to use it as an
> > > > > example consumer.
> > > > >
> > > > > A minor discrepancy I encountered is whether to put a semicolon after
> > > > > DEFINE_FREE(). It seems some in-tree code does, some doesn't. In the
> > > > > end,
> > > > > I followed the example set in core-api/cleanup.rst, and omitted it.
> > > > >
> > > > > Signed-off-by: Bence Csokas <[email protected]>
> > > > > ---
> > > >
> > > > I'm really having trouble seeing how this improves anything
> > > > significantly. Probably others have the same impression which is why
> > > > this doesn't get a lot of review.
> > >
> > > The goal of the __free() API is to reduce the number of goto-based error
> > > paths. Normally, you'd have to keep the state of every local in your head
> > > for every early return. (Has it been initialized? Has it been consumed? Or
> > > does it need to be freed?) Then you need to arrange your "goto tree"
> > > (error
> > > labels) in a way that frees everything, and then for every early exit,
> > > jump
> > > to the correct one. This can be quite complex for some functions. Look at
> > > KVM or HyperV eventfd code, for example.
> > >
> > > By contrast, you just mark the local var with _free() and an appropriate
> > > destructor snippet, and the consumption point (the point after which we
> > > don't need to free it on a return) with no_free_ptr(), and the compiler
> > > instruments every early return accordingly.
> >
> > Sure, but there's also the other side about which you don't speak: unless
> > the destructor is trivial (like kfree()), there are implications about
> > locking, when resources (such as references held by the object) get
> > released etc. So it can also be more complex to wrap your head around
> > __free usage than around standard cleanup code.
>
> Sure. Thankfully eventfd_ctx_put() is one of the pretty trivial ones,
> basically just calling kfree() on itself when the kref drops to 0.
Ok, fair.
> > So unless __free is providing a noticeable simplification of some goto
> > hell, cleanup in many functions or similar, I generally don't find it worth
> > it...
>
> Look into for instance mshv_irqfd_assign() and weep... And then imagine how
> much simpler it'd be if it were using __free() instead.
Well, over the years I've seen much worse things :) But I agree for that
function using __free would be a simplification.
> For the example in 2/2, I deliberately chose one callsite where it's trivial
> to deduce that behaviour is unchanged.
Well, the change is kind of "meh, I don't care much" for me. So I guess if
you include some conversion that actually makes code simpler, I guess
Christian might be open to merging this.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
