Skip to contentSkip to conversation

net: mana: add ethtool private flag for full-page RX buffers

PATCH Superseded Dipayaan Roy 5 participants 9 messages Jul 1 – Jul 9

Subsystems (6)
Networking Network drivers BPF InfiniBand +2 more subsystems

v11 of 13 Superseded Latest is v13 →

lore
Share & tools
lore

Dipayaan Roy

author
#0 lore raw

v11 cover net: mana: add ethtool private flag for full-page RX buffers

On some ARM64 platforms with 4K PAGE_SIZE, utilizing page_pool fragments for allocation in the RX refill path (~2kB buffer per fragment) causes 15-20% throughput regression under high connection counts (>16 TCP streams at 180+ Gbps). Using full-page buffers on these platforms shows no regression and restores line-rate performance.

This behavior is observed on a single platform; other platforms perform better with page_pool fragments, indicating this is not a page_pool issue but platform-specific.

This series adds an ethtool private flag "full-page-rx" to let the user opt in to one RX buffer per page:

  ethtool --set-priv-flags eth0 full-page-rx on

There is no behavioral change by default. The flag can be persisted via udev rule for affected platforms.

This series depends on the following fixes now merged in net-next:
commit 17bfe0a8c014 ("net: mana: Add NULL guards in teardown path to prevent panic on attach failure") commit 5b05aa36ee24 ("net: mana: Skip redundant detach on already-detached port")

Changes in v11:
- Rebased on net-next
Changes in v10:
- Rebased on net-next which now includes the prerequisite fixes.
- Recovery logic in mana_set_priv_flags() leverages the idempotent mana_detach() from the merged fixes.
Changes in v9:
- Added correct tree.
Changes in v8:
- Fixed queue_reset_work recovery by restoring port_is_up before scheduling reset so the handler can properly re-attach.
- Simplified "err && schedule_port_reset" to "schedule_port_reset".
Changes in v7:
- Rebased onto net-next.
- Retained private flag approach after David Wei's testing on Grace (ARM64) confirmed that fragment mode outperforms full-page mode on other platforms, validating this is a single-platform workaround rather than a generic issue.
Changes in v6:
- Added missed maintainers.
Changes in v5:
- Split prep refactor into separate patch (patch 1/2)
Changes in v4:
- Dropping the smbios string parsing and add ethtool priv flag to reconfigure the queues with full page rx buffers.
Changes in v3:
- changed u8* to char*
Changes in v2:
- separate reading string index and the string, remove inline.

Dipayaan Roy (2):
net: mana: refactor mana_get_strings() and mana_get_sset_count() to
use switch
net: mana: force full-page RX buffers via ethtool private flag

 drivers/net/ethernet/microsoft/mana/mana_en.c |  22 ++-
 .../ethernet/microsoft/mana/mana_ethtool.c    | 178 +++++++++++++++---
 include/net/mana/mana.h                       |   8 +
 3 files changed, 177 insertions(+), 31 deletions(-)
Signature · 1 line
2.43.0
author
#1 lore raw

PATCH v11 1/2 net: mana: refactor mana_get_strings() and mana_get_sset_count() to use switch

Refactor mana_get_strings() and mana_get_sset_count() from if/else to switch statements in preparation for adding ethtool private flags support which requires handling ETH_SS_PRIV_FLAGS.

No functional change.

Reviewed-by Haiyang Zhang <[email protected]>
Signed-off-by Dipayaan Roy <[email protected]>
---
 .../ethernet/microsoft/mana/mana_ethtool.c    | 75 ++++++++++++-------
 1 file changed, 46 insertions(+), 29 deletions(-)
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +46 -29
index 94e658d07a27..fa9c49592828 100644
@@ -138,53 +138,70 @@ static int mana_get_sset_count(struct net_device *ndev, int stringset)
 	struct mana_port_context *apc = netdev_priv(ndev);
 	unsigned int num_queues = apc->num_queues;
 
-	if (stringset != ETH_SS_STATS)
+	switch (stringset) {
+	case ETH_SS_STATS:
+		return ARRAY_SIZE(mana_eth_stats) +
+		       ARRAY_SIZE(mana_phy_stats) +
+		       ARRAY_SIZE(mana_hc_stats)  +
+		       num_queues * (MANA_STATS_RX_COUNT + MANA_STATS_TX_COUNT);
+	default:
 		return -EINVAL;
-
-	return ARRAY_SIZE(mana_eth_stats) + ARRAY_SIZE(mana_phy_stats) + ARRAY_SIZE(mana_hc_stats) +
-			num_queues * (MANA_STATS_RX_COUNT + MANA_STATS_TX_COUNT);
+	}
 }
 
-static void mana_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
+static void mana_get_strings_stats(struct mana_port_context *apc, u8 **data)
 {
-	struct mana_port_context *apc = netdev_priv(ndev);
 	unsigned int num_queues = apc->num_queues;
 	int i, j;
 
-	if (stringset != ETH_SS_STATS)
-		return;
 	for (i = 0; i < ARRAY_SIZE(mana_eth_stats); i++)
-		ethtool_puts(&data, mana_eth_stats[i].name);
+		ethtool_puts(data, mana_eth_stats[i].name);
 
 	for (i = 0; i < ARRAY_SIZE(mana_hc_stats); i++)
-		ethtool_puts(&data, mana_hc_stats[i].name);
+		ethtool_puts(data, mana_hc_stats[i].name);
 
 	for (i = 0; i < ARRAY_SIZE(mana_phy_stats); i++)
-		ethtool_puts(&data, mana_phy_stats[i].name);
+		ethtool_puts(data, mana_phy_stats[i].name);
 
 	for (i = 0; i < num_queues; i++) {
-		ethtool_sprintf(&data, "rx_%d_packets", i);
-		ethtool_sprintf(&data, "rx_%d_bytes", i);
-		ethtool_sprintf(&data, "rx_%d_xdp_drop", i);
-		ethtool_sprintf(&data, "rx_%d_xdp_tx", i);
-		ethtool_sprintf(&data, "rx_%d_xdp_redirect", i);
-		ethtool_sprintf(&data, "rx_%d_pkt_len0_err", i);
+		ethtool_sprintf(data, "rx_%d_packets", i);
+		ethtool_sprintf(data, "rx_%d_bytes", i);
+		ethtool_sprintf(data, "rx_%d_xdp_drop", i);
+		ethtool_sprintf(data, "rx_%d_xdp_tx", i);
+		ethtool_sprintf(data, "rx_%d_xdp_redirect", i);
+		ethtool_sprintf(data, "rx_%d_pkt_len0_err", i);
 		for (j = 0; j < MANA_RXCOMP_OOB_NUM_PPI - 1; j++)
-			ethtool_sprintf(&data, "rx_%d_coalesced_cqe_%d", i, j + 2);
+			ethtool_sprintf(data,
+					"rx_%d_coalesced_cqe_%d",
+					i,
+					j + 2);
 	}
 
 	for (i = 0; i < num_queues; i++) {
-		ethtool_sprintf(&data, "tx_%d_packets", i);
-		ethtool_sprintf(&data, "tx_%d_bytes", i);
-		ethtool_sprintf(&data, "tx_%d_xdp_xmit", i);
-		ethtool_sprintf(&data, "tx_%d_tso_packets", i);
-		ethtool_sprintf(&data, "tx_%d_tso_bytes", i);
-		ethtool_sprintf(&data, "tx_%d_tso_inner_packets", i);
-		ethtool_sprintf(&data, "tx_%d_tso_inner_bytes", i);
-		ethtool_sprintf(&data, "tx_%d_long_pkt_fmt", i);
-		ethtool_sprintf(&data, "tx_%d_short_pkt_fmt", i);
-		ethtool_sprintf(&data, "tx_%d_csum_partial", i);
-		ethtool_sprintf(&data, "tx_%d_mana_map_err", i);
+		ethtool_sprintf(data, "tx_%d_packets", i);
+		ethtool_sprintf(data, "tx_%d_bytes", i);
+		ethtool_sprintf(data, "tx_%d_xdp_xmit", i);
+		ethtool_sprintf(data, "tx_%d_tso_packets", i);
+		ethtool_sprintf(data, "tx_%d_tso_bytes", i);
+		ethtool_sprintf(data, "tx_%d_tso_inner_packets", i);
+		ethtool_sprintf(data, "tx_%d_tso_inner_bytes", i);
+		ethtool_sprintf(data, "tx_%d_long_pkt_fmt", i);
+		ethtool_sprintf(data, "tx_%d_short_pkt_fmt", i);
+		ethtool_sprintf(data, "tx_%d_csum_partial", i);
+		ethtool_sprintf(data, "tx_%d_mana_map_err", i);
+	}
+}
+
+static void mana_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+
+	switch (stringset) {
+	case ETH_SS_STATS:
+		mana_get_strings_stats(apc, &data);
+		break;
+	default:
+		break;
 	}
 }
Signature · 1 line
2.43.0
author
#2 lore raw

PATCH v11 2/2 net: mana: force full-page RX buffers via ethtool private flag

In reply to Dipayaan Roy

On some ARM64 platforms with 4K PAGE_SIZE, page_pool fragment allocation in the RX refill path can cause 15-20% throughput regression under high connection counts (>16 TCP streams).

Add an ethtool private flag "full-page-rx" that allows the user to force one RX buffer per page, bypassing the page_pool fragment path. This restores line-rate (180+ Gbps) performance on affected platforms.

Usage:
ethtool --set-priv-flags eth0 full-page-rx on

There is no behavioral change by default. The flag must be explicitly enabled by the user or udev rule.

The existing single-buffer-per-page logic for XDP and jumbo frames is consolidated into a new helper mana_use_single_rxbuf_per_page() which is now the single decision point for both the automatic and user-controlled paths.

Reviewed-by Jacob Keller <[email protected]>
Reviewed-by Haiyang Zhang <[email protected]>
Signed-off-by Dipayaan Roy <[email protected]>
---
 drivers/net/ethernet/microsoft/mana/mana_en.c |  22 +++-
 .../ethernet/microsoft/mana/mana_ethtool.c    | 103 ++++++++++++++++++
 include/net/mana/mana.h                       |   8 ++
 3 files changed, 131 insertions(+), 2 deletions(-)
drivers/net/ethernet/microsoft/mana/mana_en.c +20 -2
index 26aef21c6c2c..4bd83c782ea3 100644
@@ -755,6 +755,25 @@ static void *mana_get_rxbuf_pre(struct mana_rxq *rxq, dma_addr_t *da)
 	return va;
 }
 
+static bool
+mana_use_single_rxbuf_per_page(struct mana_port_context *apc, u32 mtu)
+{
+	/* On some platforms with 4K PAGE_SIZE, page_pool fragment allocation
+	 * in the RX refill path (~2kB buffer) can cause significant throughput
+	 * regression under high connection counts. Allow user to force one RX
+	 * buffer per page via ethtool private flag to bypass the fragment
+	 * path.
+	 */
+	if (apc->priv_flags & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF))
+		return true;
+
+	/* For xdp and jumbo frames make sure only one packet fits per page. */
+	if (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2 || mana_xdp_get(apc))
+		return true;
+
+	return false;
+}
+
 /* Get RX buffer's data size, alloc size, XDP headroom based on MTU */
 static void mana_get_rxbuf_cfg(struct mana_port_context *apc,
 			       int mtu, u32 *datasize, u32 *alloc_size,

@@ -765,8 +784,7 @@ static void mana_get_rxbuf_cfg(struct mana_port_context *apc,
 	/* Calculate datasize first (consistent across all cases) */
 	*datasize = mtu + ETH_HLEN;
 
-	/* For xdp and jumbo frames make sure only one packet fits per page */
-	if (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2 || mana_xdp_get(apc)) {
+	if (mana_use_single_rxbuf_per_page(apc, mtu)) {
 		if (mana_xdp_get(apc)) {
 			*headroom = XDP_PACKET_HEADROOM;
 			*alloc_size = PAGE_SIZE;
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +103 -0

137 lines not on this page · raw message

include/net/mana/mana.h +8 -0
index 13c87baf018e..8dc496f05938 100644
@@ -30,6 +30,12 @@ enum TRI_STATE {
 	TRI_STATE_TRUE = 1
 };
 
+/* MANA ethtool private flag bit positions */
+enum mana_priv_flag_bits {
+	MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF = 0,
+	MANA_PRIV_FLAG_MAX,
+};
+
 /* Number of entries for hardware indirection table must be in power of 2 */
 #define MANA_INDIRECT_TABLE_MAX_SIZE 512
 #define MANA_INDIRECT_TABLE_DEF_SIZE 64

@@ -532,6 +538,8 @@ struct mana_port_context {
 	u32 rxbpre_headroom;
 	u32 rxbpre_frag_count;
 
+	u32 priv_flags;
+
 	struct bpf_prog *bpf_prog;
 
 	/* Create num_queues EQs, SQs, SQ-CQs, RQs and RQ-CQs, respectively. */
Signature · 1 line
2.43.0
AI review
#3 lore raw

Re: [PATCH net-next v11 2/2] net: mana: force full-page RX buffers via ethtool private flag

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

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

New issues:
- [High] Forcing port_is_up = true in the mana_attach() error path leaves the port state inconsistent, leading to a double-teardown of hardware queues and a NULL pointer dereference in stats gathering.
- [High] The device restart sequence in mana_set_priv_flags() lacks vport_mutex synchronization and does not set apc->channel_changing, creating a race condition with concurrent RDMA operations.

Pre-existing issues:
- [High] Bulk pre-allocation of RX buffers uses GFP_ATOMIC and emergency memory reserves in a process context (ethtool), which can lead to memory depletion and spurious failures.
- [High] Pre-allocated RX buffers with from_pool = false are leaked when dropped in a queue configured with frag_count > 1.
--

commit 40ed2f3da9bc06c10116aa35575da8c740d52002
Author: Dipayaan Roy <[email protected]>

net: mana: force full-page RX buffers via ethtool private flag

This patch adds an ethtool private flag "full-page-rx" to force one RX buffer per page on ARM64 platforms. The single-buffer-per-page logic is consolidated into a new helper function. The driver is detached and reattached when the flag is modified to apply the new buffer configurations.

Quoted 4 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +0 -0
index 3531abfdaf433..24f6ce63b031b 100644

[ ... ]

Quoted 30 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +26 -0
@@ -611,6 +630,88 @@ static int mana_get_link_ksettings(struct net_device *ndev,
 	return 0;
 }
 
+static u32 mana_get_priv_flags(struct net_device *ndev)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+
+	return apc->priv_flags;
+}
+
+static int mana_set_priv_flags(struct net_device *ndev, u32 priv_flags)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+	u32 changed = apc->priv_flags ^ priv_flags;
+	u32 old_priv_flags = apc->priv_flags;
+	bool schedule_port_reset = false;
+	int err = 0;
+
+	if (!changed)
+		return 0;
+
+	/* Reject unknown bits */
+	if (priv_flags & ~GENMASK(MANA_PRIV_FLAG_MAX - 1, 0))
+		return -EINVAL;
+
+	if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
+		apc->priv_flags = priv_flags;
+
+		if (!apc->port_is_up) {

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 668

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +10 -0
+			/* Port is down, flag updated to apply on next up
+			 * so just return.
+			 */
+			return 0;
+		}
+
+		/* Pre-allocate buffers to prevent failure in mana_attach
+		 * later
+		 */
+		err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);

[Severity: High]
This is a pre-existing issue, but does calling mana_pre_alloc_rxbufs() from ethtool context cause memory allocation problems?

mana_pre_alloc_rxbufs() calls dev_alloc_pages() in a bulk loop, which uses GFP_ATOMIC and emergency memory reserves. Since this is now triggered from process context via ethtool, could this deplete system emergency reserves intended for atomic receive paths, leading to spurious failures or network drops under memory pressure?

[Severity: High]
This isn't a bug introduced by this patch, but does toggling this flag dynamically create conditions that permanently leak pre-allocated buffers?

When a packet is dropped, mana_rx_skb() checks if from_pool is false (which it is for pre-allocated buffers) and unconditionally saves the buffer to rxq->xdp_save_va. However, mana_get_rxfrag() only reuses xdp_save_va if rxq->frag_count == 1.

If this flag causes the queue to operate with frag_count > 1 after buffers are pre-allocated (e.g. by disabling the flag), xdp_save_va is ignored. Subsequent drops will trigger WARN_ON_ONCE(rxq->xdp_save_va) and permanently leak the old buffer.

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 676

Quoted 8 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +8 -0
+		if (err) {
+			netdev_err(ndev,
+				   "Insufficient memory for new allocations\n");
+			apc->priv_flags = old_priv_flags;
+			return err;
+		}
+
+		err = mana_detach(ndev, false);

[Severity: High]
Does this device restart sequence lack vport_mutex synchronization with concurrent RDMA operations?

In mana_set_channels(), detach and re-attach is protected by asserting apc->channel_changing under vport_mutex. Here, mana_detach() tears down the vport (dropping vport_use_count to 0) without setting apc->channel_changing.

Can an RDMA process concurrently call mana_cfg_vport(), observe a zero use count, and successfully claim the vport before mana_attach() blindly attempts to configure the exact same vport, resulting in conflicting hardware programming?

Quoted 14 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +14 -0
+		if (err) {
+			netdev_err(ndev, "mana_detach failed: %d\n", err);
+			apc->priv_flags = old_priv_flags;
+
+			/* Port is in an inconsistent state. Restore
+			 * 'port_is_up' so that queue reset work handler
+			 * can properly detach and re-attach.
+			 */
+			apc->port_is_up = true;
+			schedule_port_reset = true;
+			goto out;
+		}
+
+		err = mana_attach(ndev);

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 700

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +10 -0
+		if (err) {
+			netdev_err(ndev, "mana_attach failed: %d\n", err);
+			apc->priv_flags = old_priv_flags;
+
+			/* Restore 'port_is_up' so the reset work handler
+			 * can properly detach/attach. Without this,
+			 * the handler sees port_is_up=false and skips
+			 * queue allocation, leaving the port dead.
+			 */
+			apc->port_is_up = true;

[Severity: High]
Does forcing port_is_up to true in this error path lead to a NULL pointer dereference in stats gathering?

The internal failure path of mana_attach() calls mana_cleanup_port_context(), which frees apc->rxqs and sets it to NULL.

By forcing apc->port_is_up = true here, if userspace queries stats via ethtool -S or ip -s link, mana_get_stats64() and mana_get_ethtool_stats() will bypass their safety checks and blindly dereference the NULL apc->rxqs[q] array.

Quoted 3 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +3 -0
+			schedule_port_reset = true;
+		}
+	}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
maintainer
#4 lore raw

Re: [PATCH net-next v11 2/2] net: mana: force full-page RX buffers via ethtool private flag

In reply to Dipayaan Roy

From: AI Reviewer <[email protected]>

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: mana: force full-page RX buffers via ethtool private flag

This adds an ethtool private flag "full-page-rx" that forces one RX buffer per page to avoid a page_pool fragment path throughput regression on some 4K PAGE_SIZE ARM64 platforms. Toggling the flag while the port is up triggers a mana_detach() + mana_attach() reconfiguration to apply the new buffer layout.

A few questions inline.

drivers/net/ethernet/microsoft/mana/mana_en.c

Quoted 4 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0
index 7438ea6b3f26..38f50b4eef87 100644

[ ... mana_use_single_rxbuf_per_page() and mana_get_rxbuf_cfg() hunks
unrelated to the comments below ... ]

Quoted 4 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +0 -0
index 3531abfdaf43..24f6ce63b031 100644

[ ... ]

Quoted 18 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +14 -0
@@ -611,6 +630,88 @@ static int mana_get_link_ksettings(struct net_device *ndev,
 	return 0;
 }
 
+static u32 mana_get_priv_flags(struct net_device *ndev)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+
+	return apc->priv_flags;
+}
+
+static int mana_set_priv_flags(struct net_device *ndev, u32 priv_flags)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+	u32 changed = apc->priv_flags ^ priv_flags;
+	u32 old_priv_flags = apc->priv_flags;
+	bool schedule_port_reset = false;
+	int err = 0;

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 656

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +10 -0
+
+	if (!changed)
+		return 0;
+
+	/* Reject unknown bits */
+	if (priv_flags & ~GENMASK(MANA_PRIV_FLAG_MAX - 1, 0))
+		return -EINVAL;
+
+	if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
+		apc->priv_flags = priv_flags;

[Low]
Is the storage of priv_flags intentionally nested inside the per-flag branch? With only one flag today this is fine, but when a second bit is added to enum mana_priv_flag_bits, a user toggling only the new bit passes the !changed and unknown-bits checks, the outer if is skipped, apc->priv_flags is never updated, and mana_set_priv_flags() returns 0 while mana_get_priv_flags() keeps reporting the old state.

Would it be cleaner to store priv_flags unconditionally after validation and use per-flag if (changed & BIT(...)) blocks only for the flag-specific reconfiguration?

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 668

Quoted 12 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +12 -0
+
+		if (!apc->port_is_up) {
+			/* Port is down, flag updated to apply on next up
+			 * so just return.
+			 */
+			return 0;
+		}
+
+		/* Pre-allocate buffers to prevent failure in mana_attach
+		 * later
+		 */
+		err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);

[Low]
When the MTU is already jumbo (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2) or XDP is attached, mana_use_single_rxbuf_per_page() already returns true regardless of the new priv flag:

	if (apc->priv_flags & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF))
		return true;

	/* For xdp and jumbo frames make sure only one packet fits per page. */
	if (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2 || mana_xdp_get(apc))
		return true;

In that case mana_get_rxbuf_cfg() produces identical output whether the new flag is on or off, yet toggling the flag here always performs a full mana_pre_alloc_rxbufs() + mana_detach() + mana_attach() cycle and flaps the link.

Would a pre/post comparison of mana_get_rxbuf_cfg() output be worth adding to skip the reconfiguration when nothing actually changes?

[High]
mana_set_priv_flags() drives the same detach/attach sequence used by mana_set_channels() and mana_per_port_queue_reset_work_handler(), but does not take apc->vport_mutex and set apc->channel_changing = true across the window.

The struct comment on channel_changing spells out the invariant:

	/* 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.
	 */
	bool channel_changing;

Without it, after mana_detach() uncfg's the vport an RDMA client can call mana_cfg_vport() and bump vport_use_count, and then mana_attach() will fail with -EBUSY when it re-cfg's the vport.

There is also no equivalent of the '!port_is_up && vport_use_count' early -EBUSY check that mana_set_channels() performs, so is the flag allowed to be changed silently while an RDMA client is using the vport?

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 676

Quoted 8 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +8 -0
+		if (err) {
+			netdev_err(ndev,
+				   "Insufficient memory for new allocations\n");
+			apc->priv_flags = old_priv_flags;
+			return err;
+		}
+
+		err = mana_detach(ndev, false);

[High]
mana_detach() begins with ASSERT_RTNL(), and so does mana_attach() called below. Is RTNL guaranteed to be held on the SET priv-flags path?

mana registers net_shaper_ops in mana_devops, which makes it an ops-locked driver, so netdev_need_ops_lock() is true and the ethtool core acquires rtnl only when the driver opts in via ETHTOOL_OP_NEEDS_RTNL_* for that specific command.

Looking at mana_ethtool_ops.op_needs_rtnl:

.op_needs_rtnl = ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM | ETHTOOL_OP_NEEDS_RTNL_GLINK,

ETHTOOL_OP_NEEDS_RTNL_SPFLAGS is not listed, so ASSERT_RTNL() in the detach/attach helpers below will WARN, and the code races with RTNL-serialized mutators (mana_open, mana_close, mana_change_mtu, mana_tx_timeout) that read and write apc->port_is_up, apc->rxqs, apc->tx_qp and apc->port_st_save.

Should ETHTOOL_OP_NEEDS_RTNL_SPFLAGS be added to op_needs_rtnl, matching what is already done for SET channels?

Quoted 15 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +15 -0
+		if (err) {
+			netdev_err(ndev, "mana_detach failed: %d\n", err);
+			apc->priv_flags = old_priv_flags;
+
+			/* Port is in an inconsistent state. Restore
+			 * 'port_is_up' so that queue reset work handler
+			 * can properly detach and re-attach.
+			 */
+			apc->port_is_up = true;
+			schedule_port_reset = true;
+			goto out;
+		}
+
+		err = mana_attach(ndev);
+		if (err) {

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 701

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +10 -0
+			netdev_err(ndev, "mana_attach failed: %d\n", err);
+			apc->priv_flags = old_priv_flags;
+
+			/* Restore 'port_is_up' so the reset work handler
+			 * can properly detach/attach. Without this,
+			 * the handler sees port_is_up=false and skips
+			 * queue allocation, leaving the port dead.
+			 */
+			apc->port_is_up = true;
+			schedule_port_reset = true;

[High]
On this branch, mana_detach() has already succeeded and run mana_cleanup_port_context(), so apc->rxqs is NULL and queues are torn down. Setting apc->port_is_up = true here between now and when queue_reset_work runs opens a window where callers gate on port_is_up as their "safe to access rxqs/tx_qp" predicate.

For example, mana_get_ethtool_stats() does:

	if (!apc->port_is_up)
		return;
	...
	rxq = apc->rxqs[q];
	...

A concurrent ethtool -S invocation during that window will pass the port_is_up gate and dereference apc->rxqs[q]->stats on a NULL rxqs.

Is the port_is_up restore actually needed for the reset work handler's mana_detach() call? On the reset path, mana_detach() takes its early return when !netif_device_present(ndev):

	if (!from_close && !netif_device_present(ndev))
		return 0;

That early return does not touch apc->port_st_save, so the saved state from the earlier successful mana_detach() should already be intact for the follow-up mana_attach().

There is also no smp_wmb() paired with this write, unlike the pattern used inside mana_detach()/mana_attach() proper.

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 714

Quoted 13 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +13 -0
+		}
+	}
+
+out:
+	mana_pre_dealloc_rxbufs(apc);
+
+	if (schedule_port_reset)
+		queue_work(apc->ac->per_port_queue_reset_wq,
+			   &apc->queue_reset_work);
+
+	return err;
+}
+

[ ... remaining hunks unrelated to the comments above ... ]

Signature · 1 line
This is an AI-generated review.
reviewer
#5 lore raw

Re: [PATCH net-next v11 2/2] net: mana: force full-page RX buffers via ethtool private flag

In reply to Dipayaan Roy

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: mana: force full-page RX buffers via ethtool private flag

This adds an ethtool private flag "full-page-rx" that forces one RX buffer per page to avoid a page_pool fragment path throughput regression on some 4K PAGE_SIZE ARM64 platforms. Toggling the flag while the port is up triggers a mana_detach() + mana_attach() reconfiguration to apply the new buffer layout.

A few questions inline.

drivers/net/ethernet/microsoft/mana/mana_en.c

Quoted 4 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0
index 7438ea6b3f26..38f50b4eef87 100644

[ ... mana_use_single_rxbuf_per_page() and mana_get_rxbuf_cfg() hunks
unrelated to the comments below ... ]

Quoted 4 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +0 -0
index 3531abfdaf43..24f6ce63b031 100644

[ ... ]

Quoted 18 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +14 -0
@@ -611,6 +630,88 @@ static int mana_get_link_ksettings(struct net_device *ndev,
 	return 0;
 }
 
+static u32 mana_get_priv_flags(struct net_device *ndev)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+
+	return apc->priv_flags;
+}
+
+static int mana_set_priv_flags(struct net_device *ndev, u32 priv_flags)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+	u32 changed = apc->priv_flags ^ priv_flags;
+	u32 old_priv_flags = apc->priv_flags;
+	bool schedule_port_reset = false;
+	int err = 0;

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 656

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +10 -0
+
+	if (!changed)
+		return 0;
+
+	/* Reject unknown bits */
+	if (priv_flags & ~GENMASK(MANA_PRIV_FLAG_MAX - 1, 0))
+		return -EINVAL;
+
+	if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
+		apc->priv_flags = priv_flags;

[Low]
Is the storage of priv_flags intentionally nested inside the per-flag branch? With only one flag today this is fine, but when a second bit is added to enum mana_priv_flag_bits, a user toggling only the new bit passes the !changed and unknown-bits checks, the outer if is skipped, apc->priv_flags is never updated, and mana_set_priv_flags() returns 0 while mana_get_priv_flags() keeps reporting the old state.

Would it be cleaner to store priv_flags unconditionally after validation and use per-flag if (changed & BIT(...)) blocks only for the flag-specific reconfiguration?

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 668

Quoted 12 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +12 -0
+
+		if (!apc->port_is_up) {
+			/* Port is down, flag updated to apply on next up
+			 * so just return.
+			 */
+			return 0;
+		}
+
+		/* Pre-allocate buffers to prevent failure in mana_attach
+		 * later
+		 */
+		err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);

[Low]
When the MTU is already jumbo (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2) or XDP is attached, mana_use_single_rxbuf_per_page() already returns true regardless of the new priv flag:

	if (apc->priv_flags & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF))
		return true;

	/* For xdp and jumbo frames make sure only one packet fits per page. */
	if (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2 || mana_xdp_get(apc))
		return true;

In that case mana_get_rxbuf_cfg() produces identical output whether the new flag is on or off, yet toggling the flag here always performs a full mana_pre_alloc_rxbufs() + mana_detach() + mana_attach() cycle and flaps the link.

Would a pre/post comparison of mana_get_rxbuf_cfg() output be worth adding to skip the reconfiguration when nothing actually changes?

[High]
mana_set_priv_flags() drives the same detach/attach sequence used by mana_set_channels() and mana_per_port_queue_reset_work_handler(), but does not take apc->vport_mutex and set apc->channel_changing = true across the window.

The struct comment on channel_changing spells out the invariant:

	/* 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.
	 */
	bool channel_changing;

Without it, after mana_detach() uncfg's the vport an RDMA client can call mana_cfg_vport() and bump vport_use_count, and then mana_attach() will fail with -EBUSY when it re-cfg's the vport.

There is also no equivalent of the '!port_is_up && vport_use_count' early -EBUSY check that mana_set_channels() performs, so is the flag allowed to be changed silently while an RDMA client is using the vport?

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 676

Quoted 8 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +8 -0
+		if (err) {
+			netdev_err(ndev,
+				   "Insufficient memory for new allocations\n");
+			apc->priv_flags = old_priv_flags;
+			return err;
+		}
+
+		err = mana_detach(ndev, false);

[High]
mana_detach() begins with ASSERT_RTNL(), and so does mana_attach() called below. Is RTNL guaranteed to be held on the SET priv-flags path?

mana registers net_shaper_ops in mana_devops, which makes it an ops-locked driver, so netdev_need_ops_lock() is true and the ethtool core acquires rtnl only when the driver opts in via ETHTOOL_OP_NEEDS_RTNL_* for that specific command.

Looking at mana_ethtool_ops.op_needs_rtnl:

.op_needs_rtnl = ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM | ETHTOOL_OP_NEEDS_RTNL_GLINK,

ETHTOOL_OP_NEEDS_RTNL_SPFLAGS is not listed, so ASSERT_RTNL() in the detach/attach helpers below will WARN, and the code races with RTNL-serialized mutators (mana_open, mana_close, mana_change_mtu, mana_tx_timeout) that read and write apc->port_is_up, apc->rxqs, apc->tx_qp and apc->port_st_save.

Should ETHTOOL_OP_NEEDS_RTNL_SPFLAGS be added to op_needs_rtnl, matching what is already done for SET channels?

Quoted 15 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +15 -0
+		if (err) {
+			netdev_err(ndev, "mana_detach failed: %d\n", err);
+			apc->priv_flags = old_priv_flags;
+
+			/* Port is in an inconsistent state. Restore
+			 * 'port_is_up' so that queue reset work handler
+			 * can properly detach and re-attach.
+			 */
+			apc->port_is_up = true;
+			schedule_port_reset = true;
+			goto out;
+		}
+
+		err = mana_attach(ndev);
+		if (err) {

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 701

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +10 -0
+			netdev_err(ndev, "mana_attach failed: %d\n", err);
+			apc->priv_flags = old_priv_flags;
+
+			/* Restore 'port_is_up' so the reset work handler
+			 * can properly detach/attach. Without this,
+			 * the handler sees port_is_up=false and skips
+			 * queue allocation, leaving the port dead.
+			 */
+			apc->port_is_up = true;
+			schedule_port_reset = true;

[High]
On this branch, mana_detach() has already succeeded and run mana_cleanup_port_context(), so apc->rxqs is NULL and queues are torn down. Setting apc->port_is_up = true here between now and when queue_reset_work runs opens a window where callers gate on port_is_up as their "safe to access rxqs/tx_qp" predicate.

For example, mana_get_ethtool_stats() does:

	if (!apc->port_is_up)
		return;
	...
	rxq = apc->rxqs[q];
	...

A concurrent ethtool -S invocation during that window will pass the port_is_up gate and dereference apc->rxqs[q]->stats on a NULL rxqs.

Is the port_is_up restore actually needed for the reset work handler's mana_detach() call? On the reset path, mana_detach() takes its early return when !netif_device_present(ndev):

	if (!from_close && !netif_device_present(ndev))
		return 0;

That early return does not touch apc->port_st_save, so the saved state from the earlier successful mana_detach() should already be intact for the follow-up mana_attach().

There is also no smp_wmb() paired with this write, unlike the pattern used inside mana_detach()/mana_attach() proper.

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 714

Quoted 13 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +13 -0
+		}
+	}
+
+out:
+	mana_pre_dealloc_rxbufs(apc);
+
+	if (schedule_port_reset)
+		queue_work(apc->ac->per_port_queue_reset_wq,
+			   &apc->queue_reset_work);
+
+	return err;
+}
+

[ ... remaining hunks unrelated to the comments above ... ]

author
#6 lore raw

Re: [PATCH net-next v11 2/2] net: mana: force full-page RX buffers via ethtool private flag

Quoted 66 lines · Simon Horman

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: mana: force full-page RX buffers via ethtool private flag

This adds an ethtool private flag "full-page-rx" that forces one RX buffer per page to avoid a page_pool fragment path throughput regression on some 4K PAGE_SIZE ARM64 platforms. Toggling the flag while the port is up triggers a mana_detach() + mana_attach() reconfiguration to apply the new buffer layout.

A few questions inline.

drivers/net/ethernet/microsoft/mana/mana_en.c

Quoted 4 lines
drivers/net/ethernet/microsoft/mana/mana_en.c +0 -0
index 7438ea6b3f26..38f50b4eef87 100644

[ ... mana_use_single_rxbuf_per_page() and mana_get_rxbuf_cfg() hunks
unrelated to the comments below ... ]

Quoted 4 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +0 -0
index 3531abfdaf43..24f6ce63b031 100644

[ ... ]

Quoted 18 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +14 -0
@@ -611,6 +630,88 @@ static int mana_get_link_ksettings(struct net_device *ndev,
 	return 0;
 }
 
+static u32 mana_get_priv_flags(struct net_device *ndev)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+
+	return apc->priv_flags;
+}
+
+static int mana_set_priv_flags(struct net_device *ndev, u32 priv_flags)
+{
+	struct mana_port_context *apc = netdev_priv(ndev);
+	u32 changed = apc->priv_flags ^ priv_flags;
+	u32 old_priv_flags = apc->priv_flags;
+	bool schedule_port_reset = false;
+	int err = 0;

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 656

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +10 -0
+
+	if (!changed)
+		return 0;
+
+	/* Reject unknown bits */
+	if (priv_flags & ~GENMASK(MANA_PRIV_FLAG_MAX - 1, 0))
+		return -EINVAL;
+
+	if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
+		apc->priv_flags = priv_flags;

[Low]
Is the storage of priv_flags intentionally nested inside the per-flag branch? With only one flag today this is fine, but when a second bit is added to enum mana_priv_flag_bits, a user toggling only the new bit passes the !changed and unknown-bits checks, the outer if is skipped, apc->priv_flags is never updated, and mana_set_priv_flags() returns 0 while mana_get_priv_flags() keeps reporting the old state.

Quoted 4 lines

Would it be cleaner to store priv_flags unconditionally after validation and use per-flag if (changed & BIT(...)) blocks only for the flag-specific reconfiguration?

Hi Simon, Paolo,

Agreed on this future concern,
but also we donot plan to have another mana driver private flag.

Quoted 25 lines

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 668

Quoted 12 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +12 -0
+
+		if (!apc->port_is_up) {
+			/* Port is down, flag updated to apply on next up
+			 * so just return.
+			 */
+			return 0;
+		}
+
+		/* Pre-allocate buffers to prevent failure in mana_attach
+		 * later
+		 */
+		err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);

[Low]
When the MTU is already jumbo (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2) or XDP is attached, mana_use_single_rxbuf_per_page() already returns true regardless of the new priv flag:

	if (apc->priv_flags & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF))
		return true;

	/* For xdp and jumbo frames make sure only one packet fits per page. */
	if (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2 || mana_xdp_get(apc))
		return true;
Quoted 7 lines

In that case mana_get_rxbuf_cfg() produces identical output whether the new flag is on or off, yet toggling the flag here always performs a full mana_pre_alloc_rxbufs() + mana_detach() + mana_attach() cycle and flaps the link.

Would a pre/post comparison of mana_get_rxbuf_cfg() output be worth adding to skip the reconfiguration when nothing actually changes?

Good point will add a pre and post comparison as well to skip the detach/attach when already in that configuration.

Quoted 15 lines

[High]
mana_set_priv_flags() drives the same detach/attach sequence used by mana_set_channels() and mana_per_port_queue_reset_work_handler(), but does not take apc->vport_mutex and set apc->channel_changing = true across the window.

The struct comment on channel_changing spells out the invariant:

	/* 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.
	 */
	bool channel_changing;
Quoted 8 lines

Without it, after mana_detach() uncfg's the vport an RDMA client can call mana_cfg_vport() and bump vport_use_count, and then mana_attach() will fail with -EBUSY when it re-cfg's the vport.

There is also no equivalent of the '!port_is_up && vport_use_count' early -EBUSY check that mana_set_channels() performs, so is the flag allowed to be changed silently while an RDMA client is using the vport?

Agreed. Will add the same vport_mutex + channel_changing pattern used by mana_set_channels() and mana_per_port_queue_reset_work_handler() to block RDMA from grabbing the vport during the detach/attach window.

Quoted 24 lines

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 676

Quoted 8 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +8 -0
+		if (err) {
+			netdev_err(ndev,
+				   "Insufficient memory for new allocations\n");
+			apc->priv_flags = old_priv_flags;
+			return err;
+		}
+
+		err = mana_detach(ndev, false);

[High]
mana_detach() begins with ASSERT_RTNL(), and so does mana_attach() called below. Is RTNL guaranteed to be held on the SET priv-flags path?

mana registers net_shaper_ops in mana_devops, which makes it an ops-locked driver, so netdev_need_ops_lock() is true and the ethtool core acquires rtnl only when the driver opts in via ETHTOOL_OP_NEEDS_RTNL_* for that specific command.

Looking at mana_ethtool_ops.op_needs_rtnl:

.op_needs_rtnl = ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM | ETHTOOL_OP_NEEDS_RTNL_GLINK,

Quoted 9 lines

ETHTOOL_OP_NEEDS_RTNL_SPFLAGS is not listed, so ASSERT_RTNL() in the detach/attach helpers below will WARN, and the code races with RTNL-serialized mutators (mana_open, mana_close, mana_change_mtu, mana_tx_timeout) that read and write apc->port_is_up, apc->rxqs, apc->tx_qp and apc->port_st_save.

Should ETHTOOL_OP_NEEDS_RTNL_SPFLAGS be added to op_needs_rtnl, matching what is already done for SET channels?

Agreed, Will add ETHTOOL_OP_NEEDS_RTNL_SPFLAGS to .op_needs_rtnl.

Quoted 51 lines
Quoted 15 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +15 -0
+		if (err) {
+			netdev_err(ndev, "mana_detach failed: %d\n", err);
+			apc->priv_flags = old_priv_flags;
+
+			/* Port is in an inconsistent state. Restore
+			 * 'port_is_up' so that queue reset work handler
+			 * can properly detach and re-attach.
+			 */
+			apc->port_is_up = true;
+			schedule_port_reset = true;
+			goto out;
+		}
+
+		err = mana_attach(ndev);
+		if (err) {

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 701

Quoted 10 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +10 -0
+			netdev_err(ndev, "mana_attach failed: %d\n", err);
+			apc->priv_flags = old_priv_flags;
+
+			/* Restore 'port_is_up' so the reset work handler
+			 * can properly detach/attach. Without this,
+			 * the handler sees port_is_up=false and skips
+			 * queue allocation, leaving the port dead.
+			 */
+			apc->port_is_up = true;
+			schedule_port_reset = true;

[High]
On this branch, mana_detach() has already succeeded and run mana_cleanup_port_context(), so apc->rxqs is NULL and queues are torn down. Setting apc->port_is_up = true here between now and when queue_reset_work runs opens a window where callers gate on port_is_up as their "safe to access rxqs/tx_qp" predicate.

For example, mana_get_ethtool_stats() does:

	if (!apc->port_is_up)
		return;
	...
	rxq = apc->rxqs[q];
	...

A concurrent ethtool -S invocation during that window will pass the port_is_up gate and dereference apc->rxqs[q]->stats on a NULL rxqs.

Is the port_is_up restore actually needed for the reset work handler's mana_detach() call? On the reset path, mana_detach() takes its early return when !netif_device_present(ndev):

	if (!from_close && !netif_device_present(ndev))
		return 0;
Quoted 7 lines

That early return does not touch apc->port_st_save, so the saved state from the earlier successful mana_detach() should already be intact for the follow-up mana_attach().

There is also no smp_wmb() paired with this write, unlike the pattern used inside mana_detach()/mana_attach() proper.

For the mana_attach() failure path: agreed, will remove port_is_up = true. After a successful mana_detach(ndev, false), netif_device_detach() has already been called so the reset handler's mana_detach() takes the early return via !netif_device_present(), preserving port_st_save for the subsequent mana_attach().

For the mana_detach() failure path: keeping port_is_up = true is intentional. Here netif_device_detach() was never reached, so the reset handler's mana_detach() would re-enter fully and overwrite port_st_save from port_is_up, if left as false, mana_attach() would skip queue allocation and leave the port dead.

Quoted 7 lines
Quoted 7 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +7 -0
+		}
+	}
+
+out:
+	mana_pre_dealloc_rxbufs(apc);
+
+	if (schedule_port_reset)
Quoted 8 lines

drivers/net/ethernet/microsoft/mana/mana_ethtool.c static int mana_get_link_ksettings(struct net_device *ndev, · line 714

Quoted 6 lines
drivers/net/ethernet/microsoft/mana/mana_ethtool.c +6 -0
+		queue_work(apc->ac->per_port_queue_reset_wq,
+			   &apc->queue_reset_work);
+
+	return err;
+}
+

[ ... remaining hunks unrelated to the comments above ... ]

Thanks for the review comments, I will address them in the next version.

Regards
Dipayaan Roy

#7 lore raw
In reply to Dipayaan Roy
Quoted 10 lines · Dipayaan Roy

On some ARM64 platforms with 4K PAGE_SIZE, utilizing page_pool fragments for allocation in the RX refill path (~2kB buffer per fragment) causes 15-20% throughput regression under high connection counts (>16 TCP streams at 180+ Gbps). Using full-page buffers on these platforms shows no regression and restores line-rate performance.

This behavior is observed on a single platform; other platforms perform better with page_pool fragments, indicating this is not a page_pool issue but platform-specific.

Quoted 7 lines

This series adds an ethtool private flag "full-page-rx" to let the user opt in to one RX buffer per page:

  ethtool --set-priv-flags eth0 full-page-rx on

There is no behavioral change by default. The flag can be persisted via udev rule for affected platforms.

Were you able to track down what is the actual bottleneck on the 'broken' platform? What is the performance of full-page approach on healthy platforms? On changelog below you mention the frag approach 'outperforms' the full-page one.

Quoted 49 lines

This series depends on the following fixes now merged in net-next:
commit 17bfe0a8c014 ("net: mana: Add NULL guards in teardown path to prevent panic on attach failure") commit 5b05aa36ee24 ("net: mana: Skip redundant detach on already-detached port")

Changes in v11:
- Rebased on net-next
Changes in v10:
- Rebased on net-next which now includes the prerequisite fixes.
- Recovery logic in mana_set_priv_flags() leverages the idempotent mana_detach() from the merged fixes.
Changes in v9:
- Added correct tree.
Changes in v8:
- Fixed queue_reset_work recovery by restoring port_is_up before scheduling reset so the handler can properly re-attach.
- Simplified "err && schedule_port_reset" to "schedule_port_reset".
Changes in v7:
- Rebased onto net-next.
- Retained private flag approach after David Wei's testing on Grace (ARM64) confirmed that fragment mode outperforms full-page mode on other platforms, validating this is a single-platform workaround rather than a generic issue.
Changes in v6:
- Added missed maintainers.
Changes in v5:
- Split prep refactor into separate patch (patch 1/2)
Changes in v4:
- Dropping the smbios string parsing and add ethtool priv flag to reconfigure the queues with full page rx buffers.
Changes in v3:
- changed u8* to char*
Changes in v2:
- separate reading string index and the string, remove inline.

Dipayaan Roy (2):
net: mana: refactor mana_get_strings() and mana_get_sset_count() to
use switch
net: mana: force full-page RX buffers via ethtool private flag

 drivers/net/ethernet/microsoft/mana/mana_en.c |  22 ++-
 .../ethernet/microsoft/mana/mana_ethtool.c    | 178 +++++++++++++++---
 include/net/mana/mana.h                       |   8 +
 3 files changed, 177 insertions(+), 31 deletions(-)
Signature · 1 line
2.43.0
author
#8 lore raw
Quoted 16 lines · Maciej Fijalkowski
Quoted 15 lines · Dipayaan Roy

On some ARM64 platforms with 4K PAGE_SIZE, utilizing page_pool fragments for allocation in the RX refill path (~2kB buffer per fragment) causes 15-20% throughput regression under high connection counts (>16 TCP streams at 180+ Gbps). Using full-page buffers on these platforms shows no regression and restores line-rate performance.

This behavior is observed on a single platform; other platforms perform better with page_pool fragments, indicating this is not a page_pool issue but platform-specific.

This series adds an ethtool private flag "full-page-rx" to let the user opt in to one RX buffer per page:

  ethtool --set-priv-flags eth0 full-page-rx on
Quoted 8 lines
Quoted 2 lines

There is no behavioral change by default. The flag can be persisted via udev rule for affected platforms.

Were you able to track down what is the actual bottleneck on the 'broken' platform? What is the performance of full-page approach on healthy platforms? On changelog below you mention the frag approach 'outperforms' the full-page one.

Hi Maciej,

The HW team identified a PCIE root port stall occurring due to a PCIe errata in a HW IP used for this platform. Using full pages increases the time in packet refill path and indirectly helping to reduce the back pressure in NIC pipeline caused due to the stall in root port. As per them it will be fixed in next version of this hw which is not anytime soon.

On various other healthy platforms with 4k base page size we tested, we see improvements using page fragments than full pages around anywhere between 5 to 15%.

Regards
Dipayaan Roy

Quoted 49 lines
Quoted 49 lines

This series depends on the following fixes now merged in net-next:
commit 17bfe0a8c014 ("net: mana: Add NULL guards in teardown path to prevent panic on attach failure") commit 5b05aa36ee24 ("net: mana: Skip redundant detach on already-detached port")

Changes in v11:
- Rebased on net-next
Changes in v10:
- Rebased on net-next which now includes the prerequisite fixes.
- Recovery logic in mana_set_priv_flags() leverages the idempotent mana_detach() from the merged fixes.
Changes in v9:
- Added correct tree.
Changes in v8:
- Fixed queue_reset_work recovery by restoring port_is_up before scheduling reset so the handler can properly re-attach.
- Simplified "err && schedule_port_reset" to "schedule_port_reset".
Changes in v7:
- Rebased onto net-next.
- Retained private flag approach after David Wei's testing on Grace (ARM64) confirmed that fragment mode outperforms full-page mode on other platforms, validating this is a single-platform workaround rather than a generic issue.
Changes in v6:
- Added missed maintainers.
Changes in v5:
- Split prep refactor into separate patch (patch 1/2)
Changes in v4:
- Dropping the smbios string parsing and add ethtool priv flag to reconfigure the queues with full page rx buffers.
Changes in v3:
- changed u8* to char*
Changes in v2:
- separate reading string index and the string, remove inline.

Dipayaan Roy (2):
net: mana: refactor mana_get_strings() and mana_get_sset_count() to
use switch
net: mana: force full-page RX buffers via ethtool private flag

 drivers/net/ethernet/microsoft/mana/mana_en.c |  22 ++-
 .../ethernet/microsoft/mana/mana_ethtool.c    | 178 +++++++++++++++---
 include/net/mana/mana.h                       |   8 +
 3 files changed, 177 insertions(+), 31 deletions(-)
Signature · 1 line
2.43.0