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
