Re: [PATCH net-next v3 02/13] net: mana: share the EQ pool across a queue-set swap

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 1

- [Medium] The patch makes the EQ pool port-owned and grows it against
  the *live* port context from inside the allocation phase, while the…
- [Medium] mana_grow_eqs()
  (drivers/net/ethernet/microsoft/mana/mana_en.c:1835) is the first NIC…
- [Low] mana_qset_scratch_alloc()'s kernel-doc
  (drivers/net/ethernet/microsoft/mana/mana_en.c:3983-3985) still…

--

net: mana: share the EQ pool across a queue-set swap

The EQ array moves from the queue set to the port, so a queue-set swap no
longer needs old + new MSI-X vectors. mana_create_eq() now sizes the array
to apc->max_queues and tracks how many EQs exist in apc->num_eqs, and a new
mana_grow_eqs() adds EQs on demand from mana_alloc_qset().

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index b9d9543d1a6dc..bd80658cf3efc 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -1814,6 +1821,72 @@ int mana_create_eq(struct mana_port_context *apc)
>  }
>  EXPORT_SYMBOL_NS(mana_create_eq, "NET_MANA");
>  
> +/**
> + * mana_grow_eqs - make sure the port has at least @need EQs
> + * @apc:  port context
> + * @need: number of EQs the new queue set requires
> + *
> + * EQs are bound to MSI-X vectors, so the pool is port-owned and shared across
> + * a swap: peak usage is max(old, new), not the sum. Grow-only, up to
> + * apc->max_queues.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +static int mana_grow_eqs(struct mana_port_context *apc, unsigned int need)
> +{
> +	struct gdma_dev *gd = apc->ac->gdma_dev;
> +	struct gdma_context *gc = gd->gdma_context;
> +	struct gdma_queue_spec spec = {};
> +	struct gdma_irq_context *gic;
> +	unsigned int i;
> +	int err;
> +	int msi;
> +
> +	if (WARN_ON(!apc->eqs))
> +		return -EINVAL;
> +
> +	if (need > apc->max_queues)
> +		return -EINVAL;
> +
> +	if (need <= apc->num_eqs)
> +		return 0;
> +
> +	spec.type = GDMA_EQ;
> +	spec.monitor_avl_buf = false;
> +	spec.queue_size = EQ_SIZE;
> +	spec.eq.callback = NULL;
> +	spec.eq.context = apc->eqs;
> +	spec.eq.log2_throttle_limit = LOG2_EQ_THROTTLE;
> +
> +	for (i = apc->num_eqs; i < need; i++) {
> +		msi = (i + 1) % gc->num_msix_usable;
> +
> +		gic = mana_gd_get_gic(gc, !gc->msi_sharing, &msi);
> +		if (IS_ERR(gic)) {
> +			err = PTR_ERR(gic);
> +			goto out;
> +		}
> +		spec.eq.msix_index = msi;
> +
> +		err = mana_gd_create_mana_eq(gd, &spec, &apc->eqs[i].eq);

[Severity: Medium]
This looks like the first NIC path that creates an EQ while the port is up
and its MSI-X vectors are already delivering interrupts.  Can a vector
interrupt land on the EQ before it is initialized?

mana_gd_create_mana_eq() -> mana_gd_create_eq() publishes the queue to the
handler list first:

gdma_main.c:mana_gd_create_eq() {
	...
	queue->id = INVALID_QUEUE_ID;
	...
	err = mana_gd_register_irq(queue, spec);
	...
	queue->eq.callback = spec->eq.callback;
	queue->eq.context = spec->eq.context;
	queue->head |= INITIALIZED_OWNER_BIT(log2_num_entries);
	...
}

gdma_main.c:mana_gd_register_irq() {
	...
	spin_lock_irqsave(&gic->lock, flags);
	list_add_rcu(&queue->entry, &gic->eq_list);
	spin_unlock_irqrestore(&gic->lock, flags);
}

and the hardirq handler walks that list under RCU only:

gdma_main.c:mana_gd_intr() {
	rcu_read_lock();
	list_for_each_entry_rcu(eq, eq_list, entry) {
		gic->handler(eq);
	}
	rcu_read_unlock();
}

With head still 0 and a zeroed ring, does mana_gd_process_eq_events() miss
its early return?

gdma_main.c:mana_gd_process_eq_events() {
	...
		old_bits = (eq->head / num_eqe - 1) & GDMA_EQE_OWNER_MASK;
		/* No more entries */
		if (owner_bits == old_bits) {
			if (i == 0)
				return;
	...
}

old_bits computes as 7 while owner_bits reads 0, so the handler would
consume zeroed EQEs, advance eq->head and ring the doorbell while
queue->id is still INVALID_QUEUE_ID.  The window spans the
mana_gd_create_hw_eq() and mana_gd_test_eq() HWC round trip.

The sharing precondition holds when gc->msi_sharing is set, and also when
an RDMA EQ already sits on the same index, since that path takes the vector
without setting the NIC bitmap bit:

drivers/infiniband/hw/mana/main.c:mana_ib_create_eqs() {
	...
		gic = mana_gd_get_gic(gc, false, &msi);
	...
}

Would this show up as an intermittent EQ creation failure in
mana_gd_test_eq(), or as missed events on the new queue set?  gdma_main.c is
unchanged across this series, and mana_grow_eqs() is still called on a live
port by mana_grow_qset() at the end of it.

> +		if (err) {
> +			dev_err(gc->dev, "Failed to grow EQ %u : %d\n", i, err);
> +			mana_gd_put_gic(gc, !gc->msi_sharing, msi);
> +			goto out;
> +		}
> +		apc->eqs[i].eq->eq.irq = gic->irq;
> +		mana_create_eq_debugfs(apc, i);
> +		apc->num_eqs = i + 1;
> +	}
> +
> +	return 0;
> +out:
> +	/* Keep whatever was created: the running queue set still needs its
> +	 * own EQs, and the extras are reused by the next attempt.
> +	 */
> +	return err;
> +}
> +

[ ... ]

> @@ -3926,14 +3996,14 @@ struct mana_port_context *mana_qset_scratch_alloc(struct mana_port_context *apc)
>  
>  	*scratch = *apc;
>  
> -	/* Owns no queues yet. */
> -	scratch->eqs		= NULL;
> +	/* EQs stay shared with the live port: they are a vector-backed
> +	 * resource and must not be duplicated for the new set.
> +	 */
>  	scratch->tx_qp		= NULL;
>  	scratch->rxqs		= NULL;
>  	scratch->indir_table	= NULL;
>  	scratch->rxobj_table	= NULL;
>  	scratch->default_rxobj	= INVALID_MANA_HANDLE;
> -	scratch->mana_eqs_debugfs = NULL;

[Severity: Low]
With these two assignments gone, the kernel-doc just above
mana_qset_scratch_alloc() no longer matches the code:

 * Builds a heap copy of @apc that shares its vport identity but owns no
 * queues, so the existing allocators and destroyers can run against it
 * without touching the live context.

After *scratch = *apc the scratch aliases apc->eqs, apc->num_eqs and the
live apc->mana_eqs_debugfs dentry.  mana_destroy_eq() is an exported
NET_MANA symbol taking any struct mana_port_context *, and it does
debugfs_remove_recursive(apc->mana_eqs_debugfs) plus destroy and
kfree(apc->eqs), so running it against a scratch as the doc invites would
tear down the live port's EQs and EQ debugfs tree.

No caller does that today, either here or at the end of the series, so this
is a stale comment rather than a live bug.  Should the wording call out
that EQs and the EQ debugfs dentry are the exception?

>  
>  	/* Never consume the live set's pre-allocated RX buffers; the swap path
>  	 * has no post-teardown allocation to de-risk.
> @@ -3959,7 +4029,8 @@ void mana_qset_scratch_free(struct mana_port_context *scratch)
>   * installed set keeps serving traffic meanwhile. On error nothing is left
>   * allocated.
>   */
> -int mana_alloc_qset(struct mana_port_context *scratch, unsigned int num_queues,
> +int mana_alloc_qset(struct mana_port_context *apc,
> +		    struct mana_port_context *scratch, unsigned int num_queues,
>  		    unsigned int rx_queue_size, unsigned int tx_queue_size,
>  		    u32 priv_flags, struct mana_qset *out)
>  {
> @@ -3981,13 +4052,20 @@ int mana_alloc_qset(struct mana_port_context *scratch, unsigned int num_queues,
>  	if (err)
>  		goto cleanup_rxq_array;
>  
> -	err = mana_create_eq(scratch);
> +	/* Grow the port's shared EQ pool if this set needs more. The pool
> +	 * belongs to @apc, not to either queue set, so both sets can be
> +	 * live at once without double-booking MSI-X vectors.
> +	 */
> +	err = mana_grow_eqs(apc, num_queues);
>  	if (err)
>  		goto cleanup_rss;
>  
> +	scratch->eqs = apc->eqs;
> +	scratch->num_eqs = apc->num_eqs;
> +

[Severity: Medium]
Two documented contracts appear to be contradicted by this call.

The comment retained right above mana_alloc_qset() still says:

 * installed set keeps serving traffic meanwhile. On error nothing is left
 * allocated.

but the cleanup_eq label is deleted here and mana_grow_eqs() ends with:

out:
	/* Keep whatever was created: the running queue set still needs its
	 * own EQs, and the extras are reused by the next attempt.
	 */
	return err;

so a failed rebuild permanently raises apc->num_eqs and the port's MSI-X
footprint.  Should the "on error nothing is left allocated" wording be
adjusted, or the growth unwound?

The second one is the header comment this patch rewrites (see the
include/net/mana/mana.h hunk below): mana_grow_eqs(apc, num_queues) writes
apc->eqs[i].eq and apc->num_eqs and creates children under the live
apc->mana_eqs_debugfs while the old set is installed and TX is enabled, not
inside mana_publish_qset().

For what it is worth, the "nothing is left allocated" half is repaired
later in this series: mana_alloc_qset() stops growing EQs and
mana_grow_qset() unwinds with mana_shrink_eqs(apc, apc->num_queues) on
failure.  The header claim about where the live context is mutated still
reads the same at the end of the series, while mana_grow_qset() keeps
calling mana_grow_eqs() during allocation.

>  	err = mana_create_txq(scratch, ndev);
>  	if (err)
> -		goto cleanup_eq;
> +		goto cleanup_rss;
>  
>  	err = mana_add_rx_queues(scratch, ndev);
>  	if (err)
> @@ -4005,8 +4083,6 @@ int mana_alloc_qset(struct mana_port_context *scratch, unsigned int num_queues,
>  	 */
>  	mana_destroy_rxqs(scratch);
>  	mana_destroy_txq(scratch);
> -cleanup_eq:
> -	mana_destroy_eq(scratch);
>  cleanup_rss:
>  	mana_cleanup_indir_table(scratch);
>  cleanup_rxq_array:

[ ... ]

> diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> index 356aaa652fa6f..2117a30116c26 100644
> --- a/include/net/mana/mana.h
> +++ b/include/net/mana/mana.h

[ ... ]

> @@ -710,13 +714,14 @@ int mana_attach(struct net_device *ndev);
>  int mana_detach(struct net_device *ndev, bool from_close);
>  
>  /* Pre-allocate + swap reconfiguration. Allocation and teardown run against a
> - * scratch context, so the live port context is only ever mutated with TX
> - * disabled.
> + * scratch context, so the live port context is mutated only inside
> + * mana_publish_qset() with TX disabled. Both sets share a port-owned EQ pool.
>   */

Is this claim accurate once mana_grow_eqs() exists?  The allocation phase
mutates the live @apc: apc->eqs[i].eq, apc->num_eqs and new debugfs
children under apc->mana_eqs_debugfs are all written from
mana_alloc_qset() -> mana_grow_eqs(), with the old set still installed and
TX enabled, so the mutation is not confined to mana_publish_qset().

>  struct mana_port_context *
>  mana_qset_scratch_alloc(struct mana_port_context *apc);
>  void mana_qset_scratch_free(struct mana_port_context *scratch);
> -int mana_alloc_qset(struct mana_port_context *scratch, unsigned int num_queues,
> +int mana_alloc_qset(struct mana_port_context *apc,
> +		    struct mana_port_context *scratch, unsigned int num_queues,
>  		    unsigned int rx_queue_size, unsigned int tx_queue_size,
>  		    u32 priv_flags, struct mana_qset *out);
>  void mana_free_qset(struct mana_port_context *scratch, struct mana_qset *qset);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901014442.2945689-1-longli%40microsoft.com