> 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
