Re: [PATCH v1 1/2] landlock: Fully release unused TSYNC work entries

["Günther Noack" <gnoack3000@gmail.com>]

On Mon, Feb 16, 2026 at 09:10:59PM +0100, Mickaël Salaün wrote:
> On Mon, Feb 16, 2026 at 08:57:34PM +0100, Mickaël Salaün wrote:
> > On Mon, Feb 16, 2026 at 08:33:05PM +0100, Günther Noack wrote:
> > > On Mon, Feb 16, 2026 at 06:43:25PM +0100, Mickaël Salaün wrote:
> > > > On Mon, Feb 16, 2026 at 04:25:53PM +0100, Günther Noack wrote:
> > > > > On Mon, Feb 16, 2026 at 03:26:38PM +0100, Mickaël Salaün wrote:
> 
> > > > > > @@ -389,6 +389,15 @@ static bool schedule_task_work(struct tsync_works *works,
> > > > > >  			 */
> > > > > >  			put_task_struct(ctx->task);
> > > > > >  			ctx->task = NULL;
> > > > > > +			ctx->shared_ctx = NULL;
> > > > > > +
> > > > > > +			/*
> > > > > > +			 * Cancel the tsync_works_provide() change to recycle the reserved
> > > > > > +			 * memory for the next thread, if any.  This also ensures that
> > > > > > +			 * cancel_tsync_works() and tsync_works_release() do not see any
> > > > > > +			 * NULL task pointers.
> > > > > > +			 */
> > > > > > +			works->size--;
> > > > > 
> > > > > Looks good.
> > > > > 
> > > > > [Optional code arrangement remarks:
> > > > > 
> > > > > I would recommend to put that logic in a helper function
> > > > > "tsync_works_return(struct tsync_works *s, struct tsync_work *)", to
> > > > > be in line with the existing implementation where the manipulation of
> > > > > struct tsync_works is encapsulated in the "tsync_*" helper functions.
> > > > > 
> > > > > The scope of that function would be to do the inverse of
> > > > > "tsync_works_provide()" -- putting the task_struct, decreasing
> > > > > works->size, and then, to be safe, also clearing the contents of the
> > > > > tsync_work struct (although that is strictly speaking not required if
> > > > > we decrease the size, I think).
> > > > 
> > > > Should we move the atomic_inc() to tsync_works_provide() and the
> > > > atomic_dec() to this new helper?
> > > 
> > > No, I would keep the atomic_inc() and atomic_dec() calls in the
> > > functions where they are now.  
> > > 
> > > The atomic counters belong logically to the synchronization scheme
> > > between the different threads, and I think it's clearer if we keep
> > > that synchronization code outside of the struct task_works
> > > abstraction.
> > > 
> > > I see the struct tsync_works and its operations (functions starting
> > > with "tsync_works_") as logically belonging together in an
> > > OO/encapsulation sense, and I think it's useful to have a clear
> > > boundary of responsibilities.  These functions are only in the
> > > business of managing the direct values stored in the "struct
> > > tsync_works", and in the business of allocating the memory for that
> > > data structure and incrementing refcounts to the struct task_struct.
> > > (The latter is mostly useful to have in tsync_works_provide() because
> > > the inverse put_task_struct() is useful to have in
> > > tsync_works_release(), and then it is symmetric.)
> > 
> > This makes sense.
> > 
> > > 
> > > 
> > > > > The only unusual thing about the tsync_works_return() function would
> > > > > be that it is only OK to return the very last tsync_work struct which
> > > > > was returned from tsync_works_provide().
> > > > 
> > > > What about renaming tsync_works_provide() to tsync_works_push() and this
> > > > new one to tsync_works_pop()?
> > > 
> > > I think I would find that naming slightly confusing: When a function
> > > is called "push", I would normally expect to pass a value to it, but
> > > we're getting one from it.
> > 
> > Well, it pushes the thread and returns the wrapped object.
> > 
> > > And when a method is called "pop" I would
> > > expect to get a value from it.  But the inverse is true here.
> > 
> > Fair
> > 
> > > With
> > > the names "provide" and "return" it feel that the directionality of
> > > argument passing would be clearer.
> > 
> > I don't understand the logic with "return": this tsync_works_return()
> > would not return anything.
> > 
> > What about something like tsync_works_shrink()?
> 
> tsync_works_trim() may be better.

The idea with "return" is that we are returning the previously
provided tsync_work item back into the struct tsync_works.  But I can
see that it can be confused with C's "return" statement.
tsync_works_shrink() or tsync_works_trim() is also OK.

Other options, btw, include "reclaim()" or "recycle()", if you like
that better (these LLMs are useful as thesaurus... 8-)).

I'm fine with either name, as long as the function still puts the
task_struct of the returned task_work item.  (That would be good to
keep doing, for symmetry with the _provide() and _release()
functions.)

–Günther