RE: [EXTERNAL] 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?

This is pre-existing upstream mechanism, already used by  mana_set_channels() ; this patch reuses it for the ring path rather than inventing a second scheme.

> 
> >
> > -	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?

patch 9/13 ("share the EQ pool across a queue-set swap") fixes this.

> 
> [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?

Patch 11/13 fixes it.

Long