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

[netdev-bot+sashiko@kernel.org]

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