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

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

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.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 7438ea6b3f26..38f50b4eef87 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

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

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index 3531abfdaf43..24f6ce63b031 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c

[ ... ]

> @@ -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;

[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?

> +
> +		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?

> +		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?

> +		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) {
> +			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.

> +		}
> +	}
> +
> +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 ... ]
-- 
This is an AI-generated review.