Re: [PATCH net-next v7 01/13] net: mana: add queue-set allocation and teardown helpers

Re: [PATCH net-next v7 01/13] net: mana: add queue-set allocation and teardown helpers

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message presents the patch as uncalled helper
  infrastructure ('These helpers have no live replacement callers yet').
- [Low] mana_qset_scratch_alloc() clears scratch->rxbufs_pre, das_pre and
  rxbpre_total, with the comment 'Do not consume the live set's pre-…
- [Low] struct mana_qset does not carry mana_eqs_debugfs, and
  mana_qset_scratch_alloc() sets scratch->mana_eqs_debugfs = NULL.
- [Low] mana_qset_scratch_alloc() copies the whole live port context
  (`*scratch = *apc`) without holding any lock.

--

net: mana: add queue-set allocation and teardown helpers

This commit adds mana_qset_scratch_alloc(), mana_alloc_qset() and
mana_free_qset(), which build and tear down a queue set on a scratch copy
of the port context, along with XDP program peek and release helpers. It
also adds a txq->retiring flag checked in mana_poll_tx_cq(), changes how
mana_create_page_pool() sizes the pool, and clears ac->ports[i] in
mana_remove().

> Leave ordinary mana_dealloc_queues() byte-identical to the pinned
> base. Do not introduce reset generations, sibling-port rebuilds, or
> an incomplete independent failed-FLR TX-buffer policy change.

[ ... ]

> adds that nonresetting pre-publication wait. These helpers have no
> live replacement callers yet.

[Severity: Low]

The message presents the patch as helper code with no callers yet. The
patch also changes three live paths, and the message mentions none of
them.

mana_poll_tx_cq() now checks txq->retiring in the TX completion path:

    if (txq_stopped && !READ_ONCE(txq->retiring) && apc->port_is_up &&
        avail_space >= MAX_TX_WQE_SIZE) {

mana_create_page_pool() now sizes the pool from rxq->num_rx_buf rather
than netdev_priv(rxq->ndev)->rx_queue_size:

    pprm.pool_size = rxq->num_rx_buf / rxq->frag_count + 1;

mana_remove() now clears the port slot before free_netdev():

    /* Remove the port from reset walks before freeing its netdev.
     */
    ac->ports[i] = NULL;

None of these seem to change runtime behaviour at this commit. Only the
uncalled mana_free_qset() sets retiring, and for every current caller
mana_create_rxq() sets num_rx_buf to apc->rx_queue_size.

The mana_remove() comment mentions reset walks. There doesn't seem to be
any reset walk over ac->ports[], either in this tree or later in the
series. The remaining readers are:

  - mana_link_change_work(), which is already disabled and NULL-checks
    the slot
  - probe and remove
  - mana_get_primary_netdev(), which RDMA uses and which has no NULL
    check:

    ndev = netdev_master_upper_dev_get_rcu(ac->ports[port_index]);

RDMA is removed earlier through remove_adev(). Still, the message doesn't
say how clearing ports[i] interacts with this reader.

Could the mana_remove() change go into a separate patch with its own
rationale, or be dropped?

Could the message also describe what the patch does? Right now it refers
to "the pinned base" and lists things the patch does not do.

This isn't a bug, but the patch also adds an unrelated blank line in
struct mana_context.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 489558176a5de..7586972e78e09 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> +/* Copy the vport identity without borrowing the live queues. */
> +struct mana_port_context *mana_qset_scratch_alloc(struct mana_port_context *apc)
> +{
> +	struct mana_port_context *scratch;
> +
> +	scratch = kvzalloc_obj(*scratch, GFP_KERNEL);
> +	if (!scratch)
> +		return NULL;
> +
> +	*scratch = *apc;

[Severity: Low]

Is it safe to copy the whole live port context here without holding a
lock?

The copy includes:

  - the initialized vport_mutex
  - the queue_reset_work work_struct, which may be linked into a workqueue
  - fields written concurrently, such as eth_stats (written from NAPI in
    mana_poll_tx_cq()) and vport_use_count

Later in the series, the reconfiguration callers set channel_changing,
drop vport_mutex, and then call mana_qset_scratch_alloc(). Meanwhile
mana_cfg_vport() and mana_uncfg_vport() can take apc->vport_mutex from the
RDMA driver without RTNL. That looks like a data race KCSAN could report.

No current helper locks scratch->vport_mutex, queues
scratch->queue_reset_work or reads scratch->eth_stats. Any later code that
did would be working on a copied mutex or work item.

Would it be better to copy only the identity fields the queue helpers
need?

> +
> +	scratch->eqs		= NULL;
> +	scratch->tx_qp		= NULL;
> +	scratch->rxqs		= NULL;
> +	scratch->indir_table	= NULL;
> +	scratch->rxobj_table	= NULL;
> +	scratch->default_rxobj	= INVALID_MANA_HANDLE;
> +	scratch->mana_eqs_debugfs = NULL;
> +
> +	/* Do not consume the live set's pre-allocated RX buffers. */
> +	scratch->rxbufs_pre	= NULL;
> +	scratch->das_pre	= NULL;
> +	scratch->rxbpre_total	= 0;

[Severity: Low]

Do these assignments have any effect? The RX fill path never reads the
scratch context. mana_fill_rx_oob() and mana_get_rxbuf_pre() both get the
port context from the netdev:

mana_fill_rx_oob():
    struct mana_port_context *mpc = netdev_priv(rxq->ndev);
    ...
    if (mpc->rxbufs_pre)

mana_get_rxbuf_pre():
    mpc = netdev_priv(ndev);

rxq->ndev is the live netdev. So if the live apc had pre-allocated
buffers, building a replacement set on the scratch would still take them,
or fail with -ENOMEM on a size mismatch.

mana_alloc_qset() has no callers at this commit. Later in the series,
mana_pre_alloc_rxbufs() is only called from
mana_per_port_queue_reset_work_handler(). That handler allocates and
frees the buffers inside one rtnl_lock() section. So the live
apc->rxbufs_pre appears to always be NULL when a replacement set is
built.

Should these lines and the comment be dropped, or should the fill path use
the context that owns the queue?

> +
> +	/* Suppress debugfs names that would collide with the live set. */
> +	scratch->mana_port_debugfs = ERR_PTR(-ENODEV);
> +
> +	return scratch;
> +}

[ ... ]

> +	/* Fence RQs before unmapping, but teardown proceeds on errors. */
> +	mana_fence_rqs(scratch);
> +
> +	mana_destroy_rxqs(scratch);
> +
> +	mana_chn_xdp_release(retiring_prog, retiring_queues);
> +
> +	mana_destroy_txq(scratch);
> +	mana_destroy_eq(scratch);

[Severity: Low]

Can this leave the live EQ debugfs files pointing at freed memory?

struct mana_qset doesn't carry mana_eqs_debugfs, and
mana_qset_scratch_alloc() sets scratch->mana_eqs_debugfs to NULL. Suppose
mana_free_qset() retires a set whose EQs were created on the live apc.
mana_destroy_eq(scratch) then removes only the scratch's own dentry:

    debugfs_remove_recursive(apc->mana_eqs_debugfs);

That dentry is NULL or an ERR_PTR here. mana_destroy_eq() then frees the
EQs. The EQs/eqN head, tail, irq and eq_dump files that
mana_create_eq_debugfs() created on the live port would still exist.

Nothing reaches this at this commit. Later in the series, "net: mana:
share the EQ pool across a queue-set swap" makes mana_alloc_qset() borrow
apc->eqs and drops mana_destroy_eq() from mana_free_qset(). "net: mana:
release EQs left idle by a channel-count reduction" then stores per-EQ
dentries in apc->eqs[], so the final code no longer reaches this path.

> +	mana_cleanup_indir_table(scratch);
> +	kfree(scratch->rxqs);
> +	scratch->rxqs = NULL;
> +
> +	memset(qset, 0, sizeof(*qset));
> +}

[ ... ]

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

[PATCH net-next v7 01/13] net: mana: add queue-set allocation and teardown helpers

[Wei Hu]

From: Long Li <longli@microsoft.com>

Add queue-set allocation and teardown helpers using a scratch port
context without releasing the vport. These prepare reconfiguration
to retain its running queues if replacement allocation fails.

Leave ordinary mana_dealloc_queues() byte-identical to the pinned
base. Do not introduce reset generations, sibling-port rebuilds, or
an incomplete independent failed-FLR TX-buffer policy change.

Queue-set retirement itself performs no function reset. Its caller
must drain old published TX queues before publishing a replacement;
unpublished sets have never admitted TX. The first live-swap commit
adds that nonresetting pre-publication wait. These helpers have no
live replacement callers yet.

Signed-off-by: Long Li <longli@microsoft.com>
Signed-off-by: Wei Hu <weh@microsoft.com>
---
 .../net/ethernet/microsoft/mana/mana_bpf.c    |  24 +++
 drivers/net/ethernet/microsoft/mana/mana_en.c | 184 +++++++++++++++++-
 include/net/mana/mana.h                       |  31 +++
 3 files changed, 236 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_bpf.c b/drivers/net/ethernet/microsoft/mana/mana_bpf.c
index 484f061bc75e..ec708f3401a0 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_bpf.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_bpf.c
@@ -263,3 +263,27 @@ int mana_bpf(struct net_device *ndev, struct netdev_bpf *bpf)
 		return -EOPNOTSUPP;
 	}
 }
+
+struct bpf_prog *mana_chn_xdp_peek(struct mana_port_context *apc)
+{
+	ASSERT_RTNL();
+
+	if (!apc->rxqs || !apc->rxqs[0])
+		return NULL;
+
+	return rtnl_dereference(apc->rxqs[0]->bpf_prog);
+}
+
+/* Keep the per-queue program pointers until RX polling stops. */
+void mana_chn_xdp_release(struct bpf_prog *prog, unsigned int num_queues)
+{
+	unsigned int i;
+
+	ASSERT_RTNL();
+
+	if (!prog)
+		return;
+
+	for (i = 0; i < num_queues; i++)
+		bpf_prog_put(prog);
+}
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 489558176a5d..7586972e78e0 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -2047,7 +2047,8 @@ static void mana_poll_tx_cq(struct mana_cq *cq)
 	/* Ensure checking txq_stopped before apc->port_is_up. */
 	smp_rmb();
 
-	if (txq_stopped && apc->port_is_up && avail_space >= MAX_TX_WQE_SIZE) {
+	if (txq_stopped && !READ_ONCE(txq->retiring) && apc->port_is_up &&
+	    avail_space >= MAX_TX_WQE_SIZE) {
 		netif_tx_wake_queue(net_txq);
 		apc->eth_stats.wake_queue++;
 	}
@@ -3042,11 +3043,11 @@ static int mana_push_wqe(struct mana_rxq *rxq)
 
 static int mana_create_page_pool(struct mana_rxq *rxq, struct gdma_context *gc)
 {
-	struct mana_port_context *mpc = netdev_priv(rxq->ndev);
 	struct page_pool_params pprm = {};
 	int ret;
 
-	pprm.pool_size = mpc->rx_queue_size / rxq->frag_count + 1;
+	/* Size the pool for this RX queue, not the live configuration. */
+	pprm.pool_size = rxq->num_rx_buf / rxq->frag_count + 1;
 	pprm.nid = gc->numa_node;
 	pprm.napi = &rxq->rx_cq.napi;
 	pprm.netdev = rxq->ndev;
@@ -3795,6 +3796,179 @@ static int mana_dealloc_queues(struct net_device *ndev)
 	return 0;
 }
 
+static void mana_qset_snapshot(const struct mana_port_context *ctx,
+			       struct mana_qset *out)
+{
+	out->eqs		= ctx->eqs;
+	out->tx_qp		= ctx->tx_qp;
+	out->rxqs		= ctx->rxqs;
+	out->indir_table	= ctx->indir_table;
+	out->indir_table_sz	= ctx->indir_table_sz;
+	out->rxobj_table	= ctx->rxobj_table;
+	out->default_rxobj	= ctx->default_rxobj;
+	out->num_queues		= ctx->num_queues;
+	out->rx_queue_size	= ctx->rx_queue_size;
+	out->tx_queue_size	= ctx->tx_queue_size;
+	out->priv_flags		= ctx->priv_flags;
+}
+
+/* Vport identity and port debugfs outlive queue sets. */
+static void mana_qset_install(struct mana_port_context *ctx,
+			      const struct mana_qset *qset)
+{
+	ctx->eqs		= qset->eqs;
+	ctx->tx_qp		= qset->tx_qp;
+	ctx->rxqs		= qset->rxqs;
+	ctx->indir_table	= qset->indir_table;
+	ctx->indir_table_sz	= qset->indir_table_sz;
+	ctx->rxobj_table	= qset->rxobj_table;
+	ctx->default_rxobj	= qset->default_rxobj;
+	ctx->num_queues		= qset->num_queues;
+	ctx->rx_queue_size	= qset->rx_queue_size;
+	ctx->tx_queue_size	= qset->tx_queue_size;
+	ctx->priv_flags		= qset->priv_flags;
+}
+
+/* Copy the vport identity without borrowing the live queues. */
+struct mana_port_context *mana_qset_scratch_alloc(struct mana_port_context *apc)
+{
+	struct mana_port_context *scratch;
+
+	scratch = kvzalloc_obj(*scratch, GFP_KERNEL);
+	if (!scratch)
+		return NULL;
+
+	*scratch = *apc;
+
+	scratch->eqs		= NULL;
+	scratch->tx_qp		= NULL;
+	scratch->rxqs		= NULL;
+	scratch->indir_table	= NULL;
+	scratch->rxobj_table	= NULL;
+	scratch->default_rxobj	= INVALID_MANA_HANDLE;
+	scratch->mana_eqs_debugfs = NULL;
+
+	/* Do not consume the live set's pre-allocated RX buffers. */
+	scratch->rxbufs_pre	= NULL;
+	scratch->das_pre	= NULL;
+	scratch->rxbpre_total	= 0;
+
+	/* Suppress debugfs names that would collide with the live set. */
+	scratch->mana_port_debugfs = ERR_PTR(-ENODEV);
+
+	return scratch;
+}
+
+void mana_qset_scratch_free(struct mana_port_context *scratch)
+{
+	kvfree(scratch);
+}
+
+int mana_alloc_qset(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)
+{
+	struct net_device *ndev = scratch->ndev;
+	int err;
+
+	ASSERT_RTNL();
+
+	scratch->num_queues	= num_queues;
+	scratch->rx_queue_size	= rx_queue_size;
+	scratch->tx_queue_size	= tx_queue_size;
+	scratch->priv_flags	= priv_flags;
+
+	err = mana_init_port_context(scratch);
+	if (err)
+		goto out_err;
+
+	err = mana_rss_table_alloc(scratch);
+	if (err)
+		goto cleanup_rxq_array;
+
+	err = mana_create_eq(scratch);
+	if (err)
+		goto cleanup_rss;
+
+	err = mana_create_txq(scratch, ndev);
+	if (err)
+		goto cleanup_eq;
+
+	err = mana_add_rx_queues(scratch, ndev);
+	if (err)
+		goto cleanup_rxq;
+
+	mana_rss_table_init(scratch);
+
+	mana_qset_snapshot(scratch, out);
+	return 0;
+
+cleanup_rxq:
+	mana_destroy_rxqs(scratch);
+	mana_destroy_txq(scratch);
+cleanup_eq:
+	mana_destroy_eq(scratch);
+cleanup_rss:
+	mana_cleanup_indir_table(scratch);
+cleanup_rxq_array:
+	kfree(scratch->rxqs);
+	scratch->rxqs = NULL;
+out_err:
+	netdev_err(ndev, "%s(num_queues=%u) failed: %d\n", __func__,
+		   num_queues, err);
+	return err;
+}
+
+/* Under RTNL, free only queues no longer shared with the installed set. */
+void mana_free_qset(struct mana_port_context *scratch, struct mana_qset *qset)
+{
+	struct bpf_prog *retiring_prog;
+	unsigned int retiring_queues;
+
+	ASSERT_RTNL();
+
+	if (!qset->rxqs && !qset->tx_qp && !qset->eqs)
+		return;
+
+	if (qset->tx_qp) {
+		unsigned int q;
+
+		for (q = 0; q < qset->num_queues; q++) {
+			if (qset->tx_qp[q])
+				WRITE_ONCE(qset->tx_qp[q]->txq.retiring, true);
+		}
+	}
+
+	/* Keep retired queues and arrays through this grace period; local NAPI
+	 * synchronization does not drain other devices' XDP.
+	 */
+	synchronize_net();
+
+	mana_qset_install(scratch, qset);
+
+	/* Keep retiring RXQs' XDP programs and references until RX teardown. */
+	retiring_prog = mana_chn_xdp_peek(scratch);
+	retiring_queues = scratch->num_queues;
+
+	/* Published queues were drained before the swap; unpublished queues
+	 * have never admitted TX.
+	 */
+	/* Fence RQs before unmapping, but teardown proceeds on errors. */
+	mana_fence_rqs(scratch);
+
+	mana_destroy_rxqs(scratch);
+
+	mana_chn_xdp_release(retiring_prog, retiring_queues);
+
+	mana_destroy_txq(scratch);
+	mana_destroy_eq(scratch);
+	mana_cleanup_indir_table(scratch);
+	kfree(scratch->rxqs);
+	scratch->rxqs = NULL;
+
+	memset(qset, 0, sizeof(*qset));
+}
+
 int mana_detach(struct net_device *ndev, bool from_close)
 {
 	struct mana_port_context *apc = netdev_priv(ndev);
@@ -4279,6 +4453,10 @@ void mana_remove(struct gdma_dev *gd, bool suspending)
 		unregister_netdevice(ndev);
 		mana_cleanup_indir_table(apc);
 
+		/* Remove the port from reset walks before freeing its netdev.
+		 */
+		ac->ports[i] = NULL;
+
 		rtnl_unlock();
 
 		free_netdev(ndev);
diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
index 8c3dc9d0b299..1d91b62299f5 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -143,6 +143,9 @@ struct mana_txq {
 
 	bool napi_initialized;
 
+	/* Suppress completion wakeups on the replacement's netdev queue. */
+	bool retiring;
+
 	struct mana_stats_tx stats;
 };
 
@@ -548,6 +551,7 @@ struct mana_context {
 	u8 bm_hostmode;
 
 	struct mana_ethtool_hc_stats hc_stats;
+
 	struct workqueue_struct *per_port_queue_reset_wq;
 	/* Workqueue for querying hardware stats */
 	struct delayed_work gf_stats_work;
@@ -672,6 +676,23 @@ struct mana_port_context {
 	u32 steer_cqe_coalescing;
 };
 
+struct mana_qset {
+	struct mana_eq		*eqs;
+	struct mana_tx_qp	**tx_qp;
+	struct mana_rxq		**rxqs;
+
+	u32			*indir_table;
+	u32			indir_table_sz;
+	mana_handle_t		*rxobj_table;
+	mana_handle_t		default_rxobj;
+
+	unsigned int		num_queues;
+	unsigned int		rx_queue_size;
+	unsigned int		tx_queue_size;
+	u32			priv_flags;
+
+};
+
 netdev_tx_t mana_start_xmit(struct sk_buff *skb, struct net_device *ndev);
 int mana_config_rss(struct mana_port_context *ac, enum TRI_STATE rx,
 		    bool update_hash, bool update_tab);
@@ -681,6 +702,14 @@ int mana_alloc_queues(struct net_device *ndev);
 int mana_attach(struct net_device *ndev);
 int mana_detach(struct net_device *ndev, bool from_close);
 
+struct mana_port_context *
+mana_qset_scratch_alloc(struct mana_port_context *apc);
+void mana_qset_scratch_free(struct mana_port_context *scratch);
+int mana_alloc_qset(struct mana_port_context *scratch, unsigned int num_queues,
+		    unsigned int rx_queue_size, unsigned int tx_queue_size,
+		    u32 priv_flags, struct mana_qset *out);
+void mana_free_qset(struct mana_port_context *scratch, struct mana_qset *qset);
+
 void mana_dim_change(struct mana_cq *cq, bool enable);
 
 int mana_probe(struct gdma_dev *gd, bool resuming);
@@ -696,6 +725,8 @@ u32 mana_run_xdp(struct net_device *ndev, struct mana_rxq *rxq,
 		 struct xdp_buff *xdp, void *buf_va, uint pkt_len);
 struct bpf_prog *mana_xdp_get(struct mana_port_context *apc);
 void mana_chn_setxdp(struct mana_port_context *apc, struct bpf_prog *prog);
+struct bpf_prog *mana_chn_xdp_peek(struct mana_port_context *apc);
+void mana_chn_xdp_release(struct bpf_prog *prog, unsigned int num_queues);
 int mana_bpf(struct net_device *ndev, struct netdev_bpf *bpf);
 int mana_query_gf_stats(struct mana_context *ac);
 int mana_query_link_cfg(struct mana_port_context *apc);
-- 
2.43.0