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

["Mickaël Salaün" <mic@digikod.net>]

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.