Re: [PATCH net-next v2 7/8] net/mlx5: HWS, Shrink empty matchers

[Yevgeny Kliteynik <kliteyn@nvidia.com>]

On 25-Jun-25 03:08, Jakub Kicinski wrote:
> On Sun, 22 Jun 2025 20:22:25 +0300 Mark Bloch wrote:
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/bwc.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/bwc.c
>> index 0a7903cf75e8..b7098c7d2112 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/bwc.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/bwc.c
>> @@ -3,6 +3,8 @@
>>   
>>   #include "internal.h"
>>   
>> +static int hws_bwc_matcher_move(struct mlx5hws_bwc_matcher *bwc_matcher);
> 
> Is there a circular dependency? Normally we recommend that people
> reorder code rather that add forward declarations.

Sure, I can rearrange the code. It would, however, mean moving a lot
of code... I think I'll do it in a separate refactoring patch before
this functional one.

>> +static int hws_bwc_rule_cnt_dec_with_shrink(struct mlx5hws_bwc_rule *bwc_rule,
>> +					    u16 bwc_queue_idx)
>> +{
>> +	struct mlx5hws_bwc_matcher *bwc_matcher = bwc_rule->bwc_matcher;
>> +	struct mlx5hws_context *ctx = bwc_matcher->matcher->tbl->ctx;
>> +	struct mutex *queue_lock; /* Protect the queue */
>> +	int ret;
>> +
>> +	hws_bwc_rule_cnt_dec(bwc_rule);
>> +
>> +	if (atomic_read(&bwc_matcher->rx_size.num_of_rules) ||
>> +	    atomic_read(&bwc_matcher->tx_size.num_of_rules))
>> +		return 0;
>> +
>> +	/* Matcher has no more rules - shrink it to save ICM. */
>> +
>> +	queue_lock = hws_bwc_get_queue_lock(ctx, bwc_queue_idx);
>> +	mutex_unlock(queue_lock);
>> +
>> +	hws_bwc_lock_all_queues(ctx);
>> +	ret = hws_bwc_matcher_rehash_shrink(bwc_matcher);
>> +	hws_bwc_unlock_all_queues(ctx);
>> +
>> +	mutex_lock(queue_lock);
> 
> Dropping and re-taking caller-held locks is a bad code smell.
> Please refactor - presumably you want some portion of the condition
> to be under the lock with the dec? return true / false based on that.
> let the caller drop the lock and do the shrink if true was returned
> (directly or with another helper)

There are multiple queues that can function in parallel. Each rule
selects a random queue and immediately locks it. All the further
processing of this rule is done when this lock is held.
Sometimes there is need to do operation that requires full ownership
of the matcher. That is, this rule has to be the only rule that is
being processed. In such case, all the locks should be acquired,
which means that we're facing the 'dining philosophers' scenario.
All the locks should be acquired in the same order: the lock is
freed, and then all the locks are acquired in an orderly manner.
To have all this logic in the same function that acquires the first
lock would mean really complicating the code and breaking the simple
logical flow of the functions.

Thanks for the review!