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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 3

- [Medium] The commit message ("If the swap itself fails the previous set
  is put back and the port keeps running on it") and the…
- [Medium] mana_publish_qset() lowers ndev->real_num_rx_queues to the new
  (smaller) count before the retiring RX queues stop delivering.
- [Medium] mana_publish_qset() reopens the datapath with 'smp_wmb();
  WRITE_ONCE(apc->port_is_up, true); mana_start_txqs(apc);' and…
- [Low] mana_set_channels() builds the complete replacement queue set
  with mana_alloc_qset() while the current set is still instantiated on…
- [Low] The comment on struct mana_port_context::channel_changing in
  include/net/mana/mana.h still describes the flag as covering…
- [Low] The new inline port-down branch of mana_set_channels() writes
  apc->num_queues = new_count and resizes apc->rxqs, but unlike every…

--

net: mana: swap queue sets in mana_set_channels

The channel count path stops using detach/attach: mana_set_channels() now
builds a replacement queue set, hands it to a new mana_publish_qset() that
swaps it in under RTNL, and retires the old set afterwards. Two new port
context fields (publish_dead_end, carrier_forced_off) record the case where
neither the new nor the old set could be installed.

A few questions about the new paths below.

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

[ ... ]

> @@ -4094,6 +4145,289 @@ int mana_alloc_qset(struct mana_port_context *apc,
>  	return err;
>  }
>  
> +/* Close a port mana_publish_qset() gave up on; does nothing otherwise. Under
> + * RTNL.
> + *
> + * The caller releases the unpublished set first: closing destroys the shared
> + * EQ pool its CQs attach to, and only the caller knows whether it owns its
> + * queues or shares them with the live set. RX is already off.
> + *
> + * Merely stopping the port would leave port_is_up false with queues still
> + * allocated, so mana_detach() skips teardown and the next open trips
> + * WARN_ON(apc->eqs).
> + */
> +void mana_publish_close_if_needed(struct mana_port_context *apc)
> +{
> +	ASSERT_RTNL();
> +
> +	if (!apc->publish_dead_end)
> +		return;
> +
> +	apc->publish_dead_end = false;
> +
> +	/* mana_dealloc_queues() requires the port already marked down, which
> +	 * mana_publish_qset() did before the swap it is unwinding.
> +	 */
> +	if (mana_dealloc_queues(apc->ndev))
> +		netdev_err(apc->ndev,
> +			   "failed to close the port after a failed rollback\n");
> +}
> +
> +/* Start only the netdev queues that can take work. A carried-over queue may
> + * still have a full ring, and restarting it would just make mana_start_xmit()
> + * drop; leave it for mana_poll_tx_cq() to wake. Must run after port_is_up is
> + * set, or that wakeup is gated off.
> + */
> +static void mana_start_txqs(struct mana_port_context *apc)
> +{
> +	struct net_device *ndev = apc->ndev;
> +	unsigned int i;
> +
> +	if (!apc->tx_qp)
> +		return;
> +
> +	for (i = 0; i < apc->num_queues; i++) {
> +		if (!apc->tx_qp[i])
> +			continue;
> +
> +		if (mana_can_tx(apc->tx_qp[i]->txq.gdma_sq))
> +			netif_tx_wake_queue(netdev_get_tx_queue(ndev, i));
> +	}
> +}

[ ... ]

> +static void mana_publish_give_up(struct mana_port_context *apc)
> +{
> +	int err;
> +
> +	apc->rss_state = TRI_STATE_FALSE;
> +
> +	err = mana_disable_vport_rx(apc);
> +	if (err && mana_en_need_log(apc, err))
> +		netdev_err(apc->ndev, "failed to disable vPort RX: %d\n", err);
> +

[ ... ]

> +	apc->carrier_forced_off = netif_carrier_ok(apc->ndev);
> +	netif_carrier_off(apc->ndev);
> +	apc->publish_dead_end = true;
> +}

[ ... ]

> +	err = netif_set_real_num_tx_queues(ndev, apc->num_queues);
> +	if (err)
> +		goto rollback;
> +
> +	err = netif_set_real_num_rx_queues(ndev, apc->num_queues);
> +	if (err)
> +		goto rollback;

[Severity: Medium]
When the channel count is reduced, does lowering ndev->real_num_rx_queues
here happen before the retiring RX queues stop delivering?

At this point mana_publish_qset() has only installed the new (smaller) set.
The old RQs with rxq_idx >= the new count still have buffers posted and
their NAPIs enabled; they are only disabled much later, in
mana_free_qset() -> mana_destroy_rxqs() -> mana_destroy_rxq(), after
mana_config_rss() (an HWC round trip plus mana_fence_rqs(), up to 10s per
queue), a synchronize_net() and mana_drain_txqs() (bounded at 120s).

Everything those queues complete in that window is stamped with their own
index:

mana_rx_skb()
	...
	skb_record_rx_queue(skb, rxq_idx);

and that index then reaches the core:

net/core/dev.c:netif_get_rxqueue() {
	...
	if (unlikely(index >= dev->real_num_rx_queues)) {
		WARN_ONCE(dev->real_num_rx_queues > 1,
			  "%s received packet on queue %u, but number "
			  "of RX queues is %u\n",
			  dev->name, index, dev->real_num_rx_queues);
	...
}

netif_get_rxqueue() is reached from get_rps_cpu() whenever RPS/RFS is
configured and from netif_receive_generic_xdp(), so an 8 -> 4 reduction
under RX load looks like it can produce that warning plus a fallback to
rxqueue 0's RPS state. With the old detach/attach flow every RQ was
destroyed by mana_dealloc_queues() before the counts changed, so this
window did not exist.

Would it work to postpone netif_set_real_num_rx_queues() until after the
retiring queues have been quiesced?

> +
> +	/* Carry the XDP program over before steering can reach the new RXQs:
> +	 * they were created with bpf_prog == NULL, so a packet arriving first
> +	 * would bypass an attached program. This also takes the per-queue
> +	 * references that mana_free_qset() drops for the old set.
> +	 */
> +	mana_chn_setxdp(apc, mana_xdp_get(apc));
> +
> +	err = mana_config_rss(apc, TRI_STATE_TRUE, true, true);
> +	if (err)
> +		goto rollback;
> +
> +	/* Pair with the queue-state stores above: a datapath reader that sees
> +	 * the gate open must also see the queue set it is about to index.
> +	 */
> +	smp_wmb();
> +
> +	WRITE_ONCE(apc->port_is_up, true);
> +	mana_start_txqs(apc);

[Severity: Medium]
Is a store-store barrier enough here, given that mana_start_txqs() then
loads queue state?

smp_wmb() does not order the store to apc->port_is_up against the following
load of the SQ head/tail inside mana_can_tx(), so this is the store-buffer
pattern:

CPU0 (ethtool, RTNL)			CPU1 (NAPI)
WRITE_ONCE(apc->port_is_up, true);	mana_move_wq_tail();  /* frees room */
mana_start_txqs()			smp_mb(); smp_rmb();
  mana_can_tx() -> ring still full	  reads apc->port_is_up == false
  no netif_tx_wake_queue()		  no netif_tx_wake_queue()

The completion side is gated on a plain read:

mana_poll_tx_cq() {
	...
	/* Ensure checking txq_stopped before apc->port_is_up. */
	smp_rmb();

	if (txq_stopped && !READ_ONCE(txq->retiring) && apc->port_is_up &&
	    avail_space >= MAX_TX_WQE_SIZE) {
		netif_tx_wake_queue(net_txq);
	...
}

A fence on only one side of the store-buffer test still allows both loads
to return stale values, since CPU0's store to port_is_up can sit in its
store buffer while CPU0's load of the ring executes.

All queues are XOFF at that point because of the netif_tx_disable() earlier
in this function, and mana_start_txqs() is the only thing that can restart
them; once pending_sends is drained no further completion retries. Does
that leave the netdev TX queue stopped until the 15s watchdog fires
ndo_tx_timeout and resets the port?

The same pattern is repeated on the rollback path below, where the restored
old queues keep their rings and can legitimately be full. Would an smp_mb()
after the port_is_up store (or before reading it in mana_poll_tx_cq()) close
this?

> +
> +	return 0;
> +
> +rollback:
> +	netdev_err(ndev, "%s failed: %d, restoring previous queue set\n",
> +		   __func__, err);

[ ... ]

> +	if (netif_set_real_num_tx_queues(ndev, apc->num_queues) ||
> +	    netif_set_real_num_rx_queues(ndev, apc->num_queues)) {

[ ... ]

> +		netdev_err(ndev, "failed to restore queue counts, closing the port\n");
> +		mana_publish_give_up(apc);
> +		return err;
> +	}
> +
> +	if (mana_config_rss(apc, TRI_STATE_TRUE, true, true)) {

[ ... ]

> +		netdev_err(ndev, "failed to restore RSS steering, closing the port\n");
> +		mana_publish_give_up(apc);
> +		return err;
> +	}

[Severity: Medium]
The commit message says:

  "If the swap itself fails the previous set is put back and the port keeps
  running on it."

Do these two branches match that description?  Both call
mana_publish_give_up(), which disables vport RX, forces the carrier down and
sets apc->publish_dead_end, and the caller then reaches
mana_publish_close_if_needed() -> mana_dealloc_queues().

The result is a third outcome: the interface stays administratively up but
carries no traffic until an admin does a down/up. netif_set_real_num_rx_queues()
can return -ENOMEM from the sysfs/kobject update, and mana_config_rss() can
fail on an ordinary HWC error or timeout, so it is reachable.

Could the changelog describe that outcome, along with the two new fields
(publish_dead_end, carrier_forced_off) and the mana_open() /
mana_link_state_handle() carrier arbitration that exist only to make it
recoverable?

The same applies to the function comment added in mana_ethtool.c below,
which promises "On failure the existing queues keep running".

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index ece7ff9cc409a..2ef3d461e7072 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> @@ -648,52 +648,98 @@ static int mana_set_coalesce(struct net_device *ndev,

[ ... ]

> +/* Build the new queue set while the current one keeps serving traffic, then
> + * swap. On failure the existing queues keep running and the requested value is
> + * never replaced by a fallback. The vport is never torn down, so RDMA cannot
> + * take it mid-reconfiguration.
>   */
>  static int mana_set_channels(struct net_device *ndev,
>  			     struct ethtool_channels *channels)
>  {

[ ... ]

> +	mutex_lock(&apc->vport_mutex);
> -	if (!apc->port_is_up && apc->vport_use_count) {
> +	if (!apc->port_is_up) {
> +		struct mana_rxq **rxqs;
> +
> +		if (apc->vport_use_count) {
> +			mutex_unlock(&apc->vport_mutex);
> +			return -EBUSY;
> +		}
> +
> +		rxqs = kzalloc_objs(struct mana_rxq *, new_count);
> +		if (!rxqs) {
> +			mutex_unlock(&apc->vport_mutex);
> +			return -ENOMEM;
> +		}
> +
> +		kfree(apc->rxqs);
> +		apc->rxqs = rxqs;
> +		apc->num_queues = new_count;
> +		mutex_unlock(&apc->vport_mutex);
> +		return 0;
> +	}

[Severity: Low]
Should this port-down branch also update the netdev core counters and the
RSS table?

Every other writer of apc->num_queues pairs it with them:

mana_alloc_queues() {
	...
	err = netif_set_real_num_tx_queues(ndev, apc->num_queues);
	...
	err = netif_set_real_num_rx_queues(ndev, apc->num_queues);
	...
	mana_rss_table_init(apc);
}

and mana_publish_qset() added in this patch does the same. Here only
apc->num_queues and apc->rxqs change, yet mana_get_channels() reports the
new value immediately:

	channels->combined_count = apc->num_queues;

So after "ip link set eth0 down; ethtool -L eth0 combined 4", does
"ethtool -l" advertise 4 while real_num_tx_queues / real_num_rx_queues and
the sysfs queue listing still describe the old count, and can "ethtool -x"
return indirection entries >= 4, until the next mana_open() quietly fixes
both?

> +	if (apc->channel_changing) {
>  		mutex_unlock(&apc->vport_mutex);
>  		return -EBUSY;
>  	}
>  	apc->channel_changing = true;
>  	mutex_unlock(&apc->vport_mutex);
>  
> -	err = mana_pre_alloc_rxbufs(apc, ndev->mtu, new_count);
> -	if (err) {
> -		netdev_err(ndev, "Insufficient memory for new allocations");
> +	scratch = mana_qset_scratch_alloc(apc);
> +	if (!scratch) {
> +		err = -ENOMEM;
>  		goto clear_flag;
>  	}
>  
> -	err = mana_detach(ndev, false);
> -	if (err) {
> -		netdev_err(ndev, "mana_detach failed: %d\n", err);
> -		goto out;
> -	}
> +	err = mana_alloc_qset(apc, scratch, new_count, apc->rx_queue_size,
> +			      apc->tx_queue_size, apc->priv_flags, &newq);
> +	if (err)
> +		goto free_scratch; /* current qset untouched, nothing to undo */

[Severity: Low]
Building the whole replacement set here means old_count + new_count SQ and
RQ work-queue objects have to coexist on the same vport, but the only
validation is new_count <= apc->max_queues, and apc->max_queues is itself
clamped to the firmware-reported per-vport limits:

mana_query_vport_cfg() {
	...
	*max_sq = resp.max_num_sq;
	*max_rq = resp.max_num_rq;
}

Can "ethtool -L ethX combined N" now be rejected by the device at
MANA_CREATE_WQ_OBJ time for values that the previous detach/attach
implementation accepted, because old + new exceeds max_num_sq /
max_num_rq?

Later patches in this series appear to remove this condition:
mana_split_qset() re-partitions the existing queues and mana_grow_qset()
only fills the [old_count, new_count) slots, so the peak returns to
max(old_count, new_count). Is it worth folding that reduction in earlier, or
noting the temporary doubled peak in the changelog?

> diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> index 2117a30116c26..dd767ab623912 100644
> --- a/include/net/mana/mana.h
> +++ b/include/net/mana/mana.h
> @@ -639,6 +639,21 @@ struct mana_port_context {
>  	 */
>  	bool channel_changing;

[Severity: Low]
Could the comment above this field be refreshed?  It currently reads:

	/* Set by mana_set_channels() under vport_mutex to block RDMA
	 * from grabbing the vport during the detach/attach window.
	 * Checked by mana_cfg_vport() when called from the RDMA path.
	 */

After this patch mana_set_channels() has no detach/attach window (the vport
is never released), and mana_cfg_vport() is no longer the only reader:
mana_set_channels() itself now tests the flag and returns a new -EBUSY.

The other writer also still claims parity that no longer holds:

mana_per_port_queue_reset_work_handler() {
	...
	/* Block RDMA from grabbing the vport during the detach/attach
	 * window, same as mana_set_channels().
	 */
	mutex_lock(&apc->vport_mutex);
	apc->channel_changing = true;

It sets the flag unconditionally rather than following the new
test-then-set protocol. Both writers run under RTNL today, so nothing
misbehaves, but should they use the same protocol so a future caller that
does not hold RTNL cannot lose the -EBUSY or clear the flag early?

> +	/* mana_publish_qset() could neither publish the new set nor restore the
> +	 * old one. Vport RX is already off; the port still has to be closed,
> +	 * which mana_publish_close_if_needed() does once the caller has
> +	 * released the set that failed.
> +	 */
> +	bool publish_dead_end;
> +
> +	/* The dead end above took the carrier down for a software reason, not
> +	 * a link event, so nothing else will ever put it back: the link
> +	 * handler only runs on a real HWC_DATA_HW_LINK_* transition. Remember
> +	 * it so the next successful open restores it, which is what makes the
> +	 * documented "recoverable with a down/up" actually true.
> +	 */
> +	bool carrier_forced_off;

[ ... ]

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