Re: [PATCH net-next 11/12] net/mlx5: HWS, Free unused action STE tables

[Vlad Dogaru <vdogaru@nvidia.com>]

On Thu, Apr 10, 2025 at 07:28:18PM +0200, Michal Kubiak wrote:
> On Tue, Apr 08, 2025 at 05:00:55PM +0300, Tariq Toukan wrote:
> > From: Vlad Dogaru <vdogaru@nvidia.com>
> > 
> > Periodically check for unused action STE tables and free their
> > associated resources. In order to do this safely, add a per-queue lock
> > to synchronize the garbage collect work with regular operations on
> > steering rules.
> > 
> > Signed-off-by: Vlad Dogaru <vdogaru@nvidia.com>
> > Reviewed-by: Yevgeny Kliteynik <kliteyn@nvidia.com>
> > Reviewed-by: Mark Bloch <mbloch@nvidia.com>
> > Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> > ---
> >  .../mlx5/core/steering/hws/action_ste_pool.c  | 88 ++++++++++++++++++-
> >  .../mlx5/core/steering/hws/action_ste_pool.h  | 11 +++
> >  .../mellanox/mlx5/core/steering/hws/context.h |  1 +
> >  3 files changed, 96 insertions(+), 4 deletions(-)
> 
> [...]
> 
> > +
> > +static void hws_action_ste_pool_cleanup(struct work_struct *work)
> > +{
> > +	enum mlx5hws_pool_optimize opt;
> > +	struct mlx5hws_context *ctx;
> > +	LIST_HEAD(cleanup);
> > +	int i;
> > +
> > +	ctx = container_of(work, struct mlx5hws_context,
> > +			   action_ste_cleanup.work);
> > +
> > +	for (i = 0; i < ctx->queues; i++) {
> > +		struct mlx5hws_action_ste_pool *p = &ctx->action_ste_pool[i];
> > +
> > +		mutex_lock(&p->lock);
> > +		for (opt = MLX5HWS_POOL_OPTIMIZE_NONE;
> > +		     opt < MLX5HWS_POOL_OPTIMIZE_MAX; opt++)
> > +			hws_action_ste_pool_element_collect_stale(
> > +				&p->elems[opt], &cleanup);
> > +		mutex_unlock(&p->lock);
> > +	}
> 
> As I understand, in the loop above all unused items are being collected
> to remove them at the end of the function, using `hws_action_ste_table_cleanup_list()`.
> 
> I noticed that only the collecting of elements is protected with the mutex.
> So I have a question: is it possible that in a very short period of time
> (between `mutex_unlock()` and `hws_action_ste_table_cleanup_list()` calls),
> the cleanup list can somehow be invalidated (by changing the STE state
> in another thread)?

An action_ste_table is either:
(a) in an action_ste_pool (indirectly, via a pool element); or
(b) in a cleanup list.

In situation (a), both the table's last_used timestamp and its offsets
are protected by the parent pool's mutex. The table can only be accessed
through its parent pool.

In situation (b), the table can only be accessed by its parent list, and
the only thing we do with it is free it.

There is only one transition, from state (a) to state (b), never the
other way around. This transition is done under the parent pool's mutex.

We only move tables to the cleanup list when all of their elements are
available, so there is no risk of a `chunk_free` call accessing a table
that's on a cleanup list: there are no chunks left to free.

I think this is airtight, but I'm happy to explore possible scenarios if
you have any in mind.

Thank you for the detailed review,
Vlad