Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net-next v7 01/13] net: mana: add queue-set allocation and teardown helpers
@ 2026-10-20 16:45 netdev-bot+sashiko
  0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-10-20 16:45 UTC (permalink / raw)
  To: weh
  Cc: longli, kotaranov, kuba, davem, pabeni, edumazet, andrew+netdev,
	jgg, leon, haiyangz, wei.liu, decui, shradhagupta, horms, ernis,
	stephen, netdev, linux-rdma, linux-hyperv, linux-kernel, bpf, sdf,
	daniel, hawk, ast, john.fastabend, weh, edumazet

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

^ permalink raw reply	[flat|nested] 2+ messages in thread
* [PATCH net-next v7 00/13] net: mana: reconfigure by replacing the queue set
@ 2026-10-16 15:38 Wei Hu
  2026-10-16 15:38 ` [PATCH net-next v7 01/13] net: mana: add queue-set allocation and teardown helpers Wei Hu
  0 siblings, 1 reply; 2+ messages in thread
From: Wei Hu @ 2026-10-16 15:38 UTC (permalink / raw)
  To: longli, kotaranov, kuba, davem, pabeni, edumazet, andrew+netdev,
	jgg, leon, haiyangz, wei.liu, decui, shradhagupta, horms, ernis,
	stephen
  Cc: netdev, linux-rdma, linux-hyperv, linux-kernel, bpf, sdf, daniel,
	hawk, ast, john.fastabend, weh, edumazet

I am carrying Long Li's queue-set reconfiguration series forward for this
revision. Long remains the author of twelve patches, and Dipayaan Roy
remains the author of the detach change. Their authorship and signoffs
are retained; I have added my signoff for this revision.

Replace detach/attach reconfiguration with pre-allocation and queue-set
replacement. Allocation failure leaves the running configuration intact.
Publication failure attempts rollback; a failed rollback closes the port
and holds carrier down until a successful reopen.

EQs and statistics belong to the port. Valid user-configured RSS tables
survive queue rebuilds. Channel-count changes reuse surviving queues:
reductions retire the tail and increases allocate only the added queues.
Ring, MTU, private-flag and XDP layout changes still perform full rebuilds.

This series remains independent of the HWC concurrency/dynamic-depth
series; it is not stacked on its feature commits. Their CQ-publication
hunks overlap, so the later-applied series needs reconciliation preserving
both table-publication and individual-CQ lifetime guarantees.
https://lore.kernel.org/all/cover.1790665894.git.weh@linux.microsoft.com/

Changes since v6:

  - Rebase onto net-next d8674294aefef02266c4d47ad10131f1bffbe534
    after Simon Horman reported that v6 did not apply and CI did not run.
    Preserve upstream's GDMA core/PCI/CDX split, its separate PCI capability
    group and the RX-headroom-aware full-page shortcut.
  - Fix the DMA-region unwind required by repeated EQ-pool growth in
    patch 2. Destroy a registration still owned by software if EQ creation
    fails. A successful hardware creation invalidates that handle, so a
    later EQ-test failure does not destroy transferred ownership twice.
  - Make mana_detach() return void in patch 9 and remove unreachable
    caller error branches. Preserve deallocation logging and ndo_stop's
    required integer return.
  - Keep the thirteen-patch layout, original authors and existing signoffs.

Review notes:

  - The reported /proc/net/dev stats UAF assumes unregister_netdevice()
    defers its grace period. Its NULL-head path synchronously unlists and
    synchronizes network readers, removes sysfs state, then synchronizes
    again before returning, in both the old and new base.
  - The two stats-folding deadlock reports require unsupported 32-bit
    MANA. PCI and CDX both restrict support to 64-bit, where u64_stats_sync
    has no sequence counter or reader retry loop.
  - The xdp_xmit lost-increment race is real but already present on the
    unmodified base. Moving the counter to port-owned storage does not
    introduce it; keep that independent statistics fix separate.

Rebase request:
https://lore.kernel.org/all/20261010162855.GU83879@horms.kernel.org/
EQ unwind / detach reviews:
https://lore.kernel.org/all/sashiko-outbox-166658@kernel.org/
https://lore.kernel.org/all/sashiko-outbox-166664@kernel.org/
v6 review inventory:
https://sashiko.dev/#/patchset/cover.1790795005.git.weh@linux.microsoft.com

Changes since v5 (retained from v6):


  - Rebase the independent queue-set series onto net-next base
    47a1446725732cd3996edf607e8739334bbf4d78.
  - Drain ALL old TX queues after closing the TX/XDP gate and waiting for
    readers, but before publishing pointers, counts or RSS. A shared
    120-second drain deadline rejects a replacement by resuming the old
    configuration without reprogramming RSS or resetting the function.
    Retirement does not use FLR. Ordinary mana_dealloc_queues() remains
    byte-identical to this base: existing reset policy and independent
    failed-FLR hardening are deliberately not changed.
  - Complete the shared EQ/CQ lifetime prerequisite in patch 2. Initialize
    EQ state before IRQ publication. After stopping a retiring CQ's NAPI
    and work and tearing down its WQ object, flush its actual parent EQ
    while the CQ callback is still published. Clear its table entry and
    wait for RCU IRQ readers before freeing the CQ and callback owner.
    Pair IRQ lookups with release publication at all CQ publishers;
    Ethernet publication follows NAPI/DIM initialization. Matching HWC
    and RDMA publication stores do not change their recovery policies.
  - Move port-lifetime statistics preparation ahead of the first live
    swap, now patch 3. Introduce RX retirement, writer handoff and folding
    atomically with the first swap in patch 4, rather than relying on a
    later patch to make intermediate statistics readers safe.
  - Keep publication barriers consistent with open/attach. Latch forced
    carrier shutdown regardless of the previous carrier state and clear
    it only after successful reopen. Retain the rollback reader grace
    period after restoring old pointers, before callers discard scratch
    state.
  - Join TX queue selection to the publication gate in patch 4. Acquire
    port_is_up before count/RSS reads and snapshot real_num_tx_queues.
    Retiring tail RQs can outlive a TX-count reduction; when a recorded
    RX index no longer fits the live TX count, use the current RSS mapping
    instead of returning that stale index.
  - Clear DRV_XOFF on inactive allocated TX queues in patch 4's cold-path
    restart helper, using netif_tx_start_queue() while preserving the
    availability checks and wake behavior for active queues. Publication
    stops all allocated queues, and the netdev watchdog scans allocated
    num_tx_queues, not just real_num_tx_queues. Leaving the inactive tail
    stopped after a channel shrink caused a watchdog timeout and recovery
    on the previous release candidate.
  - Keep old RX statistics retired until rollback steering restoration
    succeeds. Restart DIM only for RXQs actually returning from retirement,
    with NAPI disabled, work cancelled and DIM reinitialized; carried RXQs
    and their counters are not reset.
  - Reserve live MTU/XDP replacement with channel_changing under the
    existing vport_mutex through queue, fail-close and scratch cleanup.
    This excludes concurrent RDMA RAW-QP vport admission without changing
    early validation, no-op or down-port paths.
  - Correct the full-page RX shortcut description to include all existing
    early-return conditions.

Resource and control-path costs:

Full rebuilds temporarily retain both sets of SQ/RQ/CQ objects, RX buffers
and page pools. The port-owned EQ pool is shared, not doubled. Device
resource limits can reject this temporary peak; no fixed hardware queue
threshold is assumed. Allocation failure preserves the live configuration.
The pre-publication TX drain can also reject a replacement without changing
the live pointer/count/RSS configuration.

CQ retirement adds parent-EQ marker operations and RCU reader waits on the
control path; these are not free and no latency improvement is claimed.
The IRQ lookup uses an acquire load, not a new per-packet lock. This series
does not introduce a general recovery policy for failed hardware commands.

Patch layout:
  1:     Queue-set helpers, preserving ordinary deallocation policy.
  2:     Shared EQ pool and EQ/CQ publication/retirement prerequisite.
  3:     Port-lifetime statistics preparation.
  4:     First live swap, nonreset drain, RX handoff and safe TX restart.
  5-8:   Ring, private-flag, MTU and XDP reconfiguration.
  9:     Remove the unreachable detach early return.
  10-11: Release unused EQs and preserve user RSS tables.
  12-13: Reuse queues across channel-count reductions and increases.

Overlap with separately submitted net fixes:

The XDP pre-allocation pointer fix is subsumed by patch 8's queue-set path,
which leaves the live program unchanged during allocation:
https://lore.kernel.org/all/20260904202640.3900685-1-longli@microsoft.com/

The user-RSS-table fix overlaps patch 11. Keep the three-argument
mana_rss_table_keep() and its queue-set callers in the overlapping blocks;
the RTNL RSS requirement is shared with the standalone net submission:
https://lore.kernel.org/all/20260905004401.3937066-1-longli@microsoft.com/

Neither submission is resent as a standalone patch here. These merge
notes do not prescribe changes to unrelated net-tree code.

History:
  v5: https://lore.kernel.org/all/20260909222416.884246-1-longli@microsoft.com/
  v4: https://lore.kernel.org/all/20260908032843.397667-1-longli@microsoft.com/
  v3: https://lore.kernel.org/all/20260901014442.2945689-1-longli@microsoft.com/
  v2 repost:
      https://lore.kernel.org/all/20260813050418.2906468-1-longli@microsoft.com/
  v2 earlier posting:
      https://lore.kernel.org/all/20260811063506.2428213-1-longli@microsoft.com/
  Full-page RX predecessor (v12, not a queue-set v1):
      https://lore.kernel.org/all/20260711041415.3008868-1-dipayanroy@linux.microsoft.com/

The link labelled "v1" in the old queue-set covers actually points to the
four-patch full-page RX v12 predecessor, not an earlier 13-patch queue-set
posting.

Testing:

Source/static qualification completed:
  - Baseline plus all thirteen commits: 28 fresh GCC/Clang W=1 target
    builds and thirteen strict checkpatch/diff checks.
  - Exact-source EQ-unwind regression: both compilers expose four v6
    leaks; nine cases pass at fixed patch 2 and final v7.
  - Full baseline/release kernels build with identical configurations.
    Sparse retains a baseline warning; Smatch retains baseline frontend
    failures/diagnostics and is not counted as a clean pass.

Fresh runtime results:
  - Stable baseline/release TCP and 64-byte UDP comparisons completed,
    forward/reverse with three samples per workload. Supported PD/MR
    and native PD/CQ/MR resource controls passed.
  - VM5 completed 39 named cases across documented exact-release boots
    and three original mixed-profile cycles, not a single-boot matrix.
  - VM6's earlier run completed 34 cases and failed repeated cleanup.
    A later exact-release run passed the four remaining XDP cases and
    all three original mixed-profile cycles. The earlier failure is
    retained, not relabeled as a complete matrix pass.
  - Three PCI remove/rescan cycles per VM passed with concurrent
    /proc/net/dev and four sysfs-stat readers, per-function resource
    identity/count checks and health dwells.
  - Both VMs were restored to their safe kernels and original controls,
    including RSS keys, tables and default/user policy. Owned fixtures
    and helpers were removed and reservations released.
  - All thirteen mails also applied cleanly to net-next d0bd617658dd.

A later unmodified net-next baseline also reproduced the TX-drain failure
class, with a pending send and a matching owned TX_OKAY completion.
DIM causality and a common root cause across runs remain unproven.
That baseline-present issue is recorded for separate investigation, not
silently treated as a passed test or a demonstrated v7 regression.
No speculative driver workaround has been added.

Prior v6 sanitizer/fault and CPU-accounting results are historical,
not fresh v7 qualification. No fresh 32-bit, CDX hardware, full-locking
or general firmware-failure recovery coverage is claimed.

v6:
https://lore.kernel.org/all/cover.1790795005.git.weh@linux.microsoft.com/

Signed-off-by: Wei Hu <weh@microsoft.com>

Dipayaan Roy (1):
  net: mana: do not bail out of mana_detach on dealloc failure

Long Li (12):
  net: mana: add queue-set allocation and teardown helpers
  net: mana: share the EQ pool across a queue-set swap
  net: mana: keep per-queue statistics in the port context
  net: mana: swap queue sets in mana_set_channels
  net: mana: swap queue sets in mana_set_ringparam
  net: mana: swap queue sets in mana_set_priv_flags
  net: mana: swap queue sets in mana_change_mtu
  net: mana: swap queue sets in mana_xdp_set
  net: mana: release EQs left idle by a channel-count reduction
  net: mana: keep a user-configured RSS table across a queue rebuild
  net: mana: keep the surviving queues when the channel count is reduced
  net: mana: keep the existing queues when the channel count is raised

 drivers/infiniband/hw/mana/cq.c               |    3 +-
 .../net/ethernet/microsoft/mana/gdma_main.c   |   26 +-
 .../net/ethernet/microsoft/mana/hw_channel.c  |    3 +-
 .../net/ethernet/microsoft/mana/mana_bpf.c    |  108 +-
 drivers/net/ethernet/microsoft/mana/mana_en.c | 1341 +++++++++++++++--
 .../ethernet/microsoft/mana/mana_ethtool.c    |  313 ++--
 include/net/mana/gdma.h                       |    8 +-
 include/net/mana/mana.h                       |   99 +-
 8 files changed, 1615 insertions(+), 286 deletions(-)


base-commit: d8674294aefef02266c4d47ad10131f1bffbe534
-- 
2.43.0

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2026-10-20 16:46 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-20 16:45 [PATCH net-next v7 01/13] net: mana: add queue-set allocation and teardown helpers netdev-bot+sashiko
  -- strict thread matches above, loose matches on Subject: below --
2026-10-16 15:38 [PATCH net-next v7 00/13] net: mana: reconfigure by replacing the queue set Wei Hu
2026-10-16 15:38 ` [PATCH net-next v7 01/13] net: mana: add queue-set allocation and teardown helpers Wei Hu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox