Re: [PATCH net-next v2 03/13] net: mana: swap queue sets in mana_set_ringparam

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Hardware Resource Exhaustion (MSI-X vectors) / Functional Regression
- [High] Ad-hoc Synchronization with Flags

Pre-existing issues:
- [High] State Loss (RSS Indirection Table)
--

--- Patch [3]: [PATCH net-next v2 03/13] net: mana: swap queue sets in mana_set_ringparam ---
commit e63909b90b30b5041af0319613913c9654a4cc80
Author: Long Li <[email protected]>

net: mana: swap queue sets in mana_set_ringparam

Convert the ring size path to pre-allocate and swap, for the same reasons
as the channel count path. An allocation failure returns the error with
the running configuration untouched, and the vport is never released.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index b12291555eaeb..40e9886c87a81 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
[ ... ]
> @@ -791,32 +790,61 @@ static int mana_set_ringparam(struct net_device *ndev,
[ ... ]
> +	/* Block RDMA from acquiring the vport for the duration. The vport
> +	 * itself is never released, so vport_use_count stays > 0.
> +	 */
> +	mutex_lock(&apc->vport_mutex);
> +	if (apc->channel_changing) {
> +		mutex_unlock(&apc->vport_mutex);
> +		return -EBUSY;
>  	}
> +	apc->channel_changing = true;
> +	mutex_unlock(&apc->vport_mutex);

[Severity: High]
Does this code use apc->channel_changing as an ad-hoc lock?

Since this flag is set on entry and cleared on exit to prevent concurrent
RDMA vport access, could we use a proper synchronization primitive here instead
of a boolean flag?

>  
> -	apc->tx_queue_size = new_tx;
> -	apc->rx_queue_size = new_rx;
> +	scratch = mana_qset_scratch_alloc(apc);
> +	if (!scratch) {
> +		err = -ENOMEM;
> +		goto clear_flag;
> +	}
>  
> -	err = mana_attach(ndev);
> +	err = mana_alloc_qset(scratch, apc->num_queues, new_rx, new_tx,
> +			      apc->priv_flags, &newq);

[Severity: High]
By pre-allocating a complete new queue set alongside the existing one, does
this require twice as many MSI-X vectors simultaneously?

If the system is already fully utilizing its vectors, could mana_create_eq()
fail to acquire vectors from the MSI-X pool, causing the ring parameter
changes to reliably fail with -ENOSPC?

[Severity: High]
This is a pre-existing issue, but does this code cause a loss of the custom RSS
indirection table?

During mana_alloc_qset(), mana_rss_table_init() is called, which unconditionally
overwrites the indirection table with default round-robin values. If a user
has configured a custom RSS indirection table, will those settings be
irrevocably destroyed here?

[ ... ]
> +clear_flag:
> +	mutex_lock(&apc->vport_mutex);
> +	apc->channel_changing = false;
> +	mutex_unlock(&apc->vport_mutex);
>  	return err;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3