[PATCH net-next v7 07/13] net: mana: swap queue sets in mana_change_mtu

[PATCH net-next v7 07/13] net: mana: swap queue sets in mana_change_mtu

[Wei Hu]

From: Long Li <longli@microsoft.com>

Carry the MTU in the queue set and size replacement RX buffers from it.
Publish ndev->mtu after RSS configuration succeeds; allocation failure
leaves the live queues and advertised MTU unchanged.

This full rebuild temporarily keeps both sets of SQ/RQ/CQ objects,
RX buffers and page pools live. The port-owned EQ pool is shared,
not doubled. Device resource limits may reject this temporary peak;
allocation failure preserves the live queues and configuration.

Reserve RAW-QP admission under vport_mutex for the entire live
replacement, including unpublished queues, fail-close and scratch
cleanup. Preserve validation, no-op and down-port behavior.

Signed-off-by: Long Li <longli@microsoft.com>
Signed-off-by: Wei Hu <weh@microsoft.com>
---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 66 ++++++++++++++-----
 .../ethernet/microsoft/mana/mana_ethtool.c    | 10 +--
 include/net/mana/mana.h                       |  7 +-
 3 files changed, 60 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index c80a06ffbe50..c1ff4d9b8d45 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -956,32 +956,52 @@ int mana_pre_alloc_rxbufs(struct mana_port_context *mpc, int new_mtu, int num_qu
 static int mana_change_mtu(struct net_device *ndev, int new_mtu)
 {
 	struct mana_port_context *mpc = netdev_priv(ndev);
-	unsigned int old_mtu = ndev->mtu;
+	struct mana_port_context *scratch;
+	struct mana_qset newq, oldq;
 	int err;
 
-	/* Pre-allocate buffers to prevent failure in mana_attach later */
-	err = mana_pre_alloc_rxbufs(mpc, new_mtu, mpc->num_queues);
-	if (err) {
-		netdev_err(ndev, "Insufficient memory for new MTU\n");
-		return err;
+	if (!mpc->port_is_up) {
+		mpc->configured_mtu = new_mtu;
+		WRITE_ONCE(ndev->mtu, new_mtu);
+		return 0;
 	}
 
-	err = mana_detach(ndev, false);
-	if (err) {
-		netdev_err(ndev, "mana_detach failed: %d\n", err);
-		goto out;
+	/* Fail-close may release the Ethernet vport reference. */
+	mutex_lock(&mpc->vport_mutex);
+	if (mpc->channel_changing) {
+		mutex_unlock(&mpc->vport_mutex);
+		return -EBUSY;
+	}
+	mpc->channel_changing = true;
+	mutex_unlock(&mpc->vport_mutex);
+
+	scratch = mana_qset_scratch_alloc(mpc);
+	if (!scratch) {
+		err = -ENOMEM;
+		goto clear_flag;
 	}
 
-	WRITE_ONCE(ndev->mtu, new_mtu);
+	err = mana_alloc_qset(mpc, scratch, mpc->num_queues,
+			      mpc->rx_queue_size, mpc->tx_queue_size,
+			      mpc->priv_flags, new_mtu, &newq);
+	if (err)
+		goto free_scratch;
 
-	err = mana_attach(ndev);
+	err = mana_publish_qset(mpc, &newq, &oldq);
 	if (err) {
-		netdev_err(ndev, "mana_attach failed: %d\n", err);
-		WRITE_ONCE(ndev->mtu, old_mtu);
+		mana_free_qset(scratch, &newq);
+		goto free_scratch;
 	}
 
-out:
-	mana_pre_dealloc_rxbufs(mpc);
+	mana_free_qset(scratch, &oldq);
+
+free_scratch:
+	mana_publish_close_if_needed(mpc);
+	mana_qset_scratch_free(scratch);
+clear_flag:
+	mutex_lock(&mpc->vport_mutex);
+	mpc->channel_changing = false;
+	mutex_unlock(&mpc->vport_mutex);
 	return err;
 }
 
@@ -3335,7 +3355,8 @@ static struct mana_rxq *mana_create_rxq(struct mana_port_context *apc,
 	rxq->rxq_idx = rxq_idx;
 	rxq->rxobj = INVALID_MANA_HANDLE;
 
-	mana_get_rxbuf_cfg(apc, ndev->mtu, &rxq->datasize, &rxq->alloc_size,
+	mana_get_rxbuf_cfg(apc, apc->configured_mtu, &rxq->datasize,
+			   &rxq->alloc_size,
 			   &rxq->headroom, &rxq->frag_count);
 	/* Create page pool for RX queue */
 	err = mana_create_page_pool(rxq, gc);
@@ -4045,6 +4066,7 @@ static void mana_qset_snapshot(const struct mana_port_context *ctx,
 	out->rx_queue_size	= ctx->rx_queue_size;
 	out->tx_queue_size	= ctx->tx_queue_size;
 	out->priv_flags		= ctx->priv_flags;
+	out->mtu		= ctx->configured_mtu;
 }
 
 /* Vport identity and port debugfs outlive queue sets. */
@@ -4061,6 +4083,7 @@ static void mana_qset_install(struct mana_port_context *ctx,
 	ctx->rx_queue_size	= qset->rx_queue_size;
 	ctx->tx_queue_size	= qset->tx_queue_size;
 	ctx->priv_flags		= qset->priv_flags;
+	ctx->configured_mtu	= qset->mtu;
 }
 
 /* Scratch starts without SQs/RQs and borrows the port's EQ pool. Never call
@@ -4101,7 +4124,7 @@ void mana_qset_scratch_free(struct mana_port_context *scratch)
 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)
+		    u32 priv_flags, int mtu, struct mana_qset *out)
 {
 	struct net_device *ndev = scratch->ndev;
 	int err;
@@ -4113,6 +4136,8 @@ int mana_alloc_qset(struct mana_port_context *apc,
 	scratch->tx_queue_size	= tx_queue_size;
 	scratch->priv_flags	= priv_flags;
 
+	scratch->configured_mtu	= mtu;
+
 	err = mana_init_port_context(scratch);
 	if (err)
 		goto out_err;
@@ -4365,6 +4390,8 @@ int mana_publish_qset(struct mana_port_context *apc, struct mana_qset *newq,
 	if (err)
 		goto rollback;
 
+	WRITE_ONCE(ndev->mtu, apc->configured_mtu);
+
 	/* Publish fields before opening the gate; pair with TX/XDP read
 	 * barriers. The post-gate full barrier cannot replace this.
 	 */
@@ -4420,6 +4447,8 @@ int mana_publish_qset(struct mana_port_context *apc, struct mana_qset *newq,
 	synchronize_net();
 	mana_fold_qset_rx_stats(apc, out_old);
 
+	WRITE_ONCE(ndev->mtu, apc->configured_mtu);
+
 	/* Publish restored fields before reopening the gate, as on success. */
 	smp_wmb();
 
@@ -4572,6 +4601,7 @@ static int mana_probe_port(struct mana_context *ac, int port_idx,
 	apc->port_handle = INVALID_MANA_HANDLE;
 	apc->pf_filter_handle = INVALID_MANA_HANDLE;
 	apc->port_idx = port_idx;
+	apc->configured_mtu = ndev->mtu;
 	apc->link_cfg_error = 1;
 	apc->cqe_coalescing_enable = 0;
 	apc->cqe8_coalescing_enable = 0;
diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
index 9c7c9879d0aa..43575aa3a168 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
@@ -738,7 +738,8 @@ static int mana_set_channels(struct net_device *ndev,
 	}
 
 	err = mana_alloc_qset(apc, scratch, new_count, apc->rx_queue_size,
-			      apc->tx_queue_size, apc->priv_flags, &newq);
+			      apc->tx_queue_size, apc->priv_flags,
+			      apc->configured_mtu, &newq);
 	if (err)
 		goto free_scratch;
 
@@ -827,7 +828,7 @@ static int mana_set_ringparam(struct net_device *ndev,
 	}
 
 	err = mana_alloc_qset(apc, scratch, apc->num_queues, new_rx, new_tx,
-			      apc->priv_flags, &newq);
+			      apc->priv_flags, apc->configured_mtu, &newq);
 	if (err) {
 		NL_SET_ERR_MSG_FMT(extack, "failed to change ring params: %d",
 				   err);
@@ -916,8 +917,9 @@ static int mana_set_priv_flags(struct net_device *ndev, u32 priv_flags)
 		goto clear_flag;
 	}
 
-	err = mana_alloc_qset(apc, scratch, apc->num_queues, apc->rx_queue_size,
-			      apc->tx_queue_size, priv_flags, &newq);
+	err = mana_alloc_qset(apc, scratch, apc->num_queues,
+			      apc->rx_queue_size, apc->tx_queue_size,
+			      priv_flags, apc->configured_mtu, &newq);
 	if (err)
 		goto free_scratch;
 
diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
index 14025024acfa..60c7df78316c 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -637,6 +637,10 @@ struct mana_port_context {
 	unsigned int rx_queue_size;
 	unsigned int tx_queue_size;
 
+	/* MTU used to size RX buffers, independent of ndev->mtu during a swap.
+	 */
+	int configured_mtu;
+
 	mana_handle_t port_handle;
 	mana_handle_t pf_filter_handle;
 
@@ -713,6 +717,7 @@ struct mana_qset {
 	unsigned int		tx_queue_size;
 	u32			priv_flags;
 
+	int			mtu;
 };
 
 netdev_tx_t mana_start_xmit(struct sk_buff *skb, struct net_device *ndev);
@@ -735,7 +740,7 @@ static inline struct mana_stats_rx *mana_rxq_stats(struct mana_rxq *rxq)
 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);
+		    u32 priv_flags, int mtu, struct mana_qset *out);
 int mana_publish_qset(struct mana_port_context *apc, struct mana_qset *newq,
 		      struct mana_qset *out_old);
 void mana_publish_close_if_needed(struct mana_port_context *apc);
-- 
2.43.0

Re: [PATCH net-next v7 07/13] net: mana: swap queue sets in mana_change_mtu

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mana: data race on TX stats syncp in mana_xdp_xmit
--

commit 03c88ebc19bffbae5acfce1a1948eb4cb597ee75
Author: Long Li <longli@microsoft.com>

net: mana: swap queue sets in mana_change_mtu

Carry the MTU in the queue set and size replacement RX buffers from it.
Publish ndev->mtu after RSS configuration succeeds; allocation failure
leaves the live queues and advertised MTU unchanged.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_bpf.c b/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> index 484f061bc75e4..687b9ce7d9a3b 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> @@ -68,7 +73,7 @@ int mana_xdp_xmit(struct net_device *ndev, int n, struct xdp_frame **frames,
>  		count++;
>  	}
>  
> -	tx_stats = &apc->tx_qp[q_idx]->txq.stats;
> +	tx_stats = apc->tx_qp[q_idx]->txq.stats;
>  
>  	u64_stats_update_begin(&tx_stats->syncp);
>  	tx_stats->xdp_xmit += count;

[Severity: Medium]
Does this stats update in mana_xdp_xmit() lack mutual exclusion when racing
with other CPUs?

When multiple CPUs redirect traffic via ndo_xdp_xmit, they can map to the
same q_idx (since q_idx = smp_processor_id() % real_num_tx_queues) and
execute u64_stats_update_begin() concurrently.

This could also race with mana_start_xmit(), which updates the same
per-queue syncp:

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 489558176a5de..c1ff4d9b8d45f 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -551,7 +579,7 @@ netdev_tx_t mana_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	/* Populated the packet and bytes counters based on post GSO packet
>  	 * calculations
>  	 */
> -	tx_stats = &txq->stats;
> +	tx_stats = txq->stats;
>  	u64_stats_update_begin(&tx_stats->syncp);
>  	tx_stats->packets += num_gso_seg;

While mana_start_xmit() holds __netif_tx_lock, that lock does not protect
against lockless mana_xdp_xmit() calls.

On architectures where u64_stats_update_begin() uses seqcounts, could this
concurrent modification corrupt the seqcount and cause readers (like
mana_get_stats64()) to infinite loop or read torn values?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791710735.git.weh@linux.microsoft.com?part=7

Re: [PATCH net-next v7 07/13] net: mana: swap queue sets in mana_change_mtu

[Wei Hu]

Hi,

> Does this stats update in mana_xdp_xmit() lack mutual exclusion when
> racing with other CPUs?

I will leave this counter update unchanged in v8. This is the same
baseline lost-update issue reported on patch 4: CPUs can share a queue,
and the aggregate xdp_xmit update is outside the per-frame send lock.
That exclusion gap predates this series in both upstream bases.

The seqcount-corruption/infinite-loop consequence requires the 32-bit
u64_stats_sync implementation. Supported PCI MANA configurations are
X86_64 or ARM64, and CDX requires 64BIT; their implementation has no
sequence counter or fetch retry loop. The lost increments are real, but
the more severe claimed consequence does not follow on those builds.

Also, the submitted patch 7 does not modify mana_bpf.c. The quoted
counter-pointer conversion belongs to an earlier patch in the series.
Neither that conversion nor this MTU change introduces the existing
writer-exclusion gap. I am keeping the independent counter fix out of v8.

Thanks,
Wei

Re: [PATCH net-next v7 07/13] net: mana: swap queue sets in mana_change_mtu

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] mana_change_mtu() adds a new use of the boolean
  channel_changing as an ad-hoc lock.

--

net: mana: swap queue sets in mana_change_mtu

With this patch, mana_change_mtu() builds a full replacement queue set
sized for the new MTU and swaps it in. It no longer does a detach and
attach. The MTU is carried in the queue set as configured_mtu, and
ndev->mtu is published only after RSS configuration succeeds.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index c80a06ffbe508..c1ff4d9b8d45f 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -956,32 +956,52 @@ int mana_pre_alloc_rxbufs(struct mana_port_context *mpc, int new_mtu, int num_qu
>  static int mana_change_mtu(struct net_device *ndev, int new_mtu)
>  {

[ ... ]

> +	/* Fail-close may release the Ethernet vport reference. */
> +	mutex_lock(&mpc->vport_mutex);
> +	if (mpc->channel_changing) {
> +		mutex_unlock(&mpc->vport_mutex);
> +		return -EBUSY;
> +	}
> +	mpc->channel_changing = true;
> +	mutex_unlock(&mpc->vport_mutex);
> +
> +	scratch = mana_qset_scratch_alloc(mpc);
> +	if (!scratch) {
> +		err = -ENOMEM;
> +		goto clear_flag;
>  	}
>  
> -	WRITE_ONCE(ndev->mtu, new_mtu);
> +	err = mana_alloc_qset(mpc, scratch, mpc->num_queues,
> +			      mpc->rx_queue_size, mpc->tx_queue_size,
> +			      mpc->priv_flags, new_mtu, &newq);
> +	if (err)
> +		goto free_scratch;
>  
> -	err = mana_attach(ndev);
> +	err = mana_publish_qset(mpc, &newq, &oldq);
>  	if (err) {
> -		netdev_err(ndev, "mana_attach failed: %d\n", err);
> -		WRITE_ONCE(ndev->mtu, old_mtu);
> +		mana_free_qset(scratch, &newq);
> +		goto free_scratch;
>  	}
>  
> -out:
> -	mana_pre_dealloc_rxbufs(mpc);
> +	mana_free_qset(scratch, &oldq);
> +
> +free_scratch:
> +	mana_publish_close_if_needed(mpc);
> +	mana_qset_scratch_free(scratch);
> +clear_flag:
> +	mutex_lock(&mpc->vport_mutex);
> +	mpc->channel_changing = false;
> +	mutex_unlock(&mpc->vport_mutex);
>  	return err;
>  }

[Severity: Medium]
Here, channel_changing seems to act as a lock around a code section rather
than as a record of object state. Is that the intent?

The flag is set under vport_mutex, and then the mutex is dropped. The whole
sleeping replacement runs after that: mana_qset_scratch_alloc(),
mana_alloc_qset(), mana_publish_qset(), mana_free_qset(),
mana_publish_close_if_needed() and mana_qset_scratch_free(). The flag is
cleared again at clear_flag.

The only reader is on the RDMA side, and it fails instead of waiting:

mana_cfg_vport() {
    ...
	mutex_lock(&apc->vport_mutex);
	if (apc->vport_use_count > 0 ||
	    (check_channel_changing && apc->channel_changing)) {
		mutex_unlock(&apc->vport_mutex);
		return -EBUSY;
	}
    ...
}

As a result, RAW-QP creation gets a transient -EBUSY during any MTU change
on a running port. Lockdep also cannot see this exclusion.

vport_mutex probably can't just be held across the region, because the
fail-close path takes it itself:

mana_publish_close_if_needed()
  -> mana_dealloc_queues()
    -> mana_destroy_vport()
      -> mana_uncfg_vport()
        -> mutex_lock(&apc->vport_mutex)

Can the -EBUSY check at the top of mana_change_mtu() ever trigger? Every
writer of channel_changing holds RTNL for its whole region, and RDMA only
reads the flag. The writers are mana_change_mtu(), the queue reset work,
the set_channels, set_ringparam and set_priv_flags handlers, and the XDP
setup path.

The same pattern already exists in the queue reset work,
mana_set_channels() and mana_set_priv_flags(). This patch extends it to
the MTU path, and it is still there at the end of the series. Could a real
lock, or some way for RDMA admission to wait, be used here?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791710735.git.weh%40linux.microsoft.com