Linux RDMA and InfiniBand development
 help / color / mirror / Atom feed
* [PATCH net-next v7 0/4] net: mana: concurrent HWC requests and dynamic queue depth
@ 2026-10-13  8:05 Wei Hu
  2026-10-13  8:05 ` [PATCH net-next v7 1/4] net: mana: prepare HWC ownership for safe reinitialization Wei Hu
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Wei Hu @ 2026-10-13  8:05 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, gargaditya,
	kees+treewide, kees, mawasthi, weh, edumazet

This series picks up Long Li's v5 MANA Hardware Channel (HWC) work,
addresses its review findings, and retains its four-patch feature lineage.
Safety changes required by concurrency and repeated HWC reinitialization
are folded into the patch which owns each behavior rather than posted as a
separate fixes series.

I have taken over follow-up upstreaming from Long. Long remains the author
of every patch, his existing sign-off is preserved, and I added mine for
the changes and submission custody.

Patch 1 prepares HWC ownership for safe reinitialization. It serializes
complete shared-memory transactions, tracks possible PF ownership,
retains mappings after an unacknowledged destroy, fences the HWC EQ before
releasing CQ state, and safely publishes and withdraws the HWC CQ table.

Patch 2 gives each slot independent completion state, withdraws caller
storage before sender return, handles the response-versus-timeout race,
and prevents depth-one slot reuse after a genuine timeout.

Patch 3 adds concurrent requests with bounded admission, serialized SQ
posting, separate sender/response ownership, stale-response quarantine,
coherent timeout updates, private setup publication, and sender draining.

Patch 4 rebuilds the depth-one bootstrap queues at validated depths from 2
through 128. It validates both reports, message limits and doorbells,
keeps the untouched bootstrap channel for depth one or reports above 128,
and restores fresh bootstrap queues after a failed rebuild only when
teardown was acknowledged.

Generic Ethernet/RDMA CQ callback refcounting and generic service-work
versus device-lifecycle serialization are independent pre-existing work
and are intentionally excluded. Malformed-RX handling, duplicate-success
correlation, and broad device-controlled allocation/timeout policy caps
also remain separate follow-ups.

This series is independent of the separately posted queue-set v6 series.
That series addresses the pre-existing generic Ethernet CQ reclamation
gap discussed in the HWC v6 review. The CQ-publication hunks overlap;
whichever series lands second will need reconciliation preserving both
table-publication and individual-CQ lifetime guarantees.
https://lore.kernel.org/all/cover.1790795005.git.weh@linux.microsoft.com/

Changes in v7 (v6 -> v7):
 - Rebase onto net-next d8674294aefef02266c4d47ad10131f1bffbe534,
   following Simon Horman's request after the v6 CI application failure.
 - Preserve the upstream GDMA core/PCI transport split and CDX support.
   Initialize the HWC lifetime lock on both transport probe paths and
   perform PCI recovery's timeout cancellation through a GDMA core helper
   which protects the published HWC pointer across the module boundary.
 - Retain the four-patch lineage and the v6 ownership, request, timeout,
   publication, depth-validation and fallback policies.

Rebase request:
https://lore.kernel.org/all/20261009151906.GF83879@horms.kernel.org/

Changes in v6 (v5 -> v6):
 - Keep the four-patch v5 structure while folding review-required HWC
   ownership, teardown, timeout, publication and fallback safety into
   their owning feature patches.
 - Serialize complete ESTABLISH_HWC and DESTROY_HWC shared-memory
   transactions. Record possible PF ownership before MMIO submission and
   clear it only after DESTROY_HWC is acknowledged.
 - Retain PF-visible queues after an indeterminate destroy. Fence the
   dedicated HWC EQ and withdraw its CQ from dispatch without freeing the
   mappings, then retry teardown before assigning fresh queues.
 - Destroy the HWC EQ before CQ and completion state. Release-publish the
   CQ table and wait for IRQ-side RCU readers before freeing it, without
   adding per-completion refcount operations.
 - Withdraw the caller's response buffer before return and quarantine a
   timed-out slot until its late response arrives or teardown cancels it.
 - Return -EBUSY for healthy slot pressure and reserve -ETIMEDOUT for an
   admitted request which receives no response.
 - Publish slots under the inflight lock, protect response state with the
   per-slot lock, and retain sender/response references until both sides
   finish.
 - Synchronize hwc_timeout updates and publish the HWC only after queues,
   admission state, caller contexts and the direct EQ test are ready.
 - Replace the goto-heavy request and channel-creation flows called out in
   review with focused admission, submission, wait, build, validation,
   rebuild and fallback helpers.
 - Treat reported depth as a maximum: rebuild exactly for 2 through 128,
   retain untouched bootstrap queues for 1 or above 128, and require the
   rebuilt report to repeat the selected depth.
 - Enforce directional request/response maxima against both the report
   and allocated buffers. Rebuild only for exact 4096-byte reports while
   permitting bootstrap operation for other nonzero maxima.
 - Validate BAR page geometry and every stored doorbell. Invalidate the
   old HWC doorbell before re-establishment and suppress MMIO while the
   invalid sentinel is present, without adding full validation to each
   data-path doorbell ring.
 - Remove the proposed capability bit because driver-version
   advertisement occurs after HWC establishment and cannot negotiate
   creation-time behavior.
 - Preserve current net-next behavior which always uses the
   hardware-reported dest_vrq_id and dest_vrcq_id.

Changes in v5 (v4 -> v5):
 - Shortened comments and documented the existing error paths in patches
   1 and 2.
 - Rejected apparent success from a response accepted before request
   submission.
 - Guarded CQ unpublication when establishment failed before cq_table
   allocation.
 - Required successful retry teardown before creating fresh bootstrap
   queues after an initial destroy failure.

Changes in v4 (return to net-next after the v3 split):
 - Reworked the feature series into four standalone patches for handover,
   per-slot completion state, concurrency and dynamic depth.
 - Added bounded admission, guarded sender accounting, timeout quarantine,
   queue-depth bounds, message-buffer sizing and rebuild validation.
 - Reset routing IDs for each establish and rejected a missing doorbell.

Changes in v3 (v2 -> net v3, historical fixes-only posting):
 - Split the combined series after maintainer feedback, posting fixes to
   net and deferring concurrency and dynamic depth.
 - Added response-buffer withdrawal and stale-response handling.
 - Removed pcie_flr() teardown fallback and retained resources after an
   unacknowledged destroy.

Changes in v2 (v1 -> v2):
 - Bounds-checked the SGE pointer derived from inline OOB size.
 - Protected active-sender accounting and drained senders during
   teardown and failure exits.
 - Rebased on net-next e354f7d60f14.

v1:
 - Combined CQ-table lifetime, queue sizing, RX validation, teardown,
   concurrent-request and dynamic-depth work in seven patches.

Validation:
 - Exact application and strict checkpatch for all four patches.
 - Baseline and every intermediate commit built with GCC/Clang W=1,
   including GDMA core, PCI, CDX COMPILE_TEST and RDMA target objects.
 - Full baseline/release kernel and module builds on the new base.
 - Sparse: no new diagnostics relative to baseline. Smatch remains
   limited by identical baseline frontend and array-bound diagnostics;
   it is not counted as a clean pass.
 - Fresh baseline/release runs on two Azure MANA VMs: concurrent PD/MR
   and native RNIC PD/CQ/MR resource controls, channel/MTU changes,
   three PCI remove/rescan cycles per VM, and three samples each of
   forward/reverse TCP and 64-byte UDP. No new kernel warning or
   functional failure in these runs; safe kernels/configurations restored.
 - These runs do not establish negotiated multi-request depth, connected
   RDMA data-plane operation, CDX hardware support or fresh firmware-fault,
   fallback, sanitizer/lockdep coverage. Prior v6 fault evidence is not
   presented as fresh v7 execution.

v6:
https://lore.kernel.org/all/cover.1790665894.git.weh@linux.microsoft.com/
v5:
https://lore.kernel.org/all/20260908035201.402424-1-longli@microsoft.com/
v4:
https://lore.kernel.org/all/20260901200018.3194525-1-longli@microsoft.com/
v3:
https://lore.kernel.org/all/20260803234355.636038-1-longli@microsoft.com/
v2:
https://lore.kernel.org/all/20260721234339.1476932-1-longli@microsoft.com/
v1:
https://lore.kernel.org/all/20260715032942.3945317-1-longli@microsoft.com/

Long Li (4):
  net: mana: prepare HWC ownership for safe reinitialization
  net: mana: give each HWC message slot its own completion state
  net: mana: support concurrent HWC requests
  net: mana: add dynamic HWC queue depth with reinit path

 .../net/ethernet/microsoft/mana/gdma_cdx.c    |   1 +
 .../net/ethernet/microsoft/mana/gdma_main.c   | 140 ++-
 .../net/ethernet/microsoft/mana/gdma_pci.c    |  11 +-
 .../net/ethernet/microsoft/mana/hw_channel.c  | 928 +++++++++++++++---
 .../net/ethernet/microsoft/mana/shm_channel.c |  58 +-
 include/net/mana/gdma.h                       |  16 +
 include/net/mana/hw_channel.h                 |  56 +-
 include/net/mana/shm_channel.h                |  10 +-
 8 files changed, 1019 insertions(+), 201 deletions(-)


base-commit: d8674294aefef02266c4d47ad10131f1bffbe534
-- 
2.43.0


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH net-next v7 3/4] net: mana: support concurrent HWC requests
@ 2026-10-14  8:15 netdev-bot+sashiko
  0 siblings, 0 replies; 10+ messages in thread
From: netdev-bot+sashiko @ 2026-10-14  8:15 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,
	gargaditya, kees+treewide, kees, mawasthi, weh, edumazet

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

Critical: 0 · High: 2 · Medium: 0 (2) · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Once a timed-out request quarantines the HWC's only slot, every
  later request on a dead channel fails with -EBUSY instead of…
- [High] A slot can be freed and given to a new sender while the RX
  handler still holds ctx->lock on it.
- [Low] The title says the patch adds "support concurrent HWC requests",
  and the message says it allows "independent message IDs to make…
- [Low] If mana_hwc_publish_channel() finds the slot occupied (a
  WARN_ON_ONCE case), mana_hwc_create_channel() calls the full…

Pre-existing issues:
- [Medium] After one timeout, mana_hwc_timeout_reduce() permanently
  lowers hwc_timeout to 1 ms.
- [Medium] mana_hwc_rx_event_handler() returns early when msg_id >=
  num_inflight_msg, and on that path it does not repost rx_req, even…

--

net: mana: support concurrent HWC requests

This patch replaces the depth-one HWC timeout latch with per-slot sender
and response reference counting. It also serializes SQ posting, returns
-EBUSY when admission slots are exhausted, and moves hwc_timeout to atomic
helpers. Channel publication and teardown are restructured around a new
gc->hwc_lock and a sender drain.

Link: https://lore.kernel.org/r/20260908035201.402424-4-longli@microsoft.com
Link: https://lore.kernel.org/r/20260914175044.2a26bb46@kernel.org

> net: mana: support concurrent HWC requests

[ ... ]

> This preserves response-buffer withdrawal and timeout quarantine while
> allowing independent message IDs to make progress.

[Severity: Low]
Does this describe the commit accurately on its own?

At this commit, mana_hwc_create_bootstrap_channel() always sizes the
channel with HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH, which is 1. That gives
one semaphore permit and a one-bit map, so no requests can run
concurrently yet. The visible change here is the new error and timeout
behaviour.

Larger depths only arrive with the next patch in the series, "net: mana:
add dynamic HWC queue depth with reinit path". Could the message say so?

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 89b6e30863dae..9fdd84ccba653 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -6,57 +6,122 @@
>  #include <net/mana/hw_channel.h>
>  #include <linux/vmalloc.h>
>  
> -static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
> +u32 mana_hwc_timeout_read(const struct hw_channel_context *hwc)

[ ... ]

> +static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, void *resp,
> +				  u32 resp_len,
> +				  struct hwc_caller_ctx **caller_ctx)
>  {
>  	struct gdma_resource *r = &hwc->inflight_msg_res;
>  	struct hwc_caller_ctx *ctx;
>  	unsigned long flags;
> +	bool channel_up;
> +	u32 wait_ms;
>  	u32 index;
>  
> -	down(&hwc->sema);
> +	wait_ms = mana_hwc_timeout_read(hwc);
> +	if (down_timeout(&hwc->sema, msecs_to_jiffies(wait_ms))) {
> +		spin_lock_irqsave(&r->lock, flags);
> +		channel_up = hwc->channel_up;
> +		spin_unlock_irqrestore(&r->lock, flags);
>  
> -	spin_lock_irqsave(&r->lock, flags);
> +		/* Slot pressure is not evidence that the HWC stopped responding. */
> +		return channel_up ? -EBUSY : -ENODEV;
> +	}

[Severity: High]
Is -EBUSY the right return value when the slot is held by a request that
timed out?

At this commit the channel has a single slot. On timeout,
mana_hwc_wait_for_response() marks the request abandoned but leaves
resp_pending set:

    if (err == -EINPROGRESS) {
        ctx->responded = true;
        abandoned = true;
    }

So the bitmap bit and the semaphore permit stay held until a late response
arrives or teardown runs. Only that one caller sees -ETIMEDOUT.

Nothing on the timeout path clears channel_up. Every later sender
therefore fails down_timeout() here and gets -EBUSY. Before this series,
and with the latch from the earlier patch in the series, those senders
got -ETIMEDOUT.

The recovery triggers only check for -ETIMEDOUT. For example, in
mana_gf_stats_work_handler():

    err = mana_query_gf_stats(ac);
    if (err == -ETIMEDOUT) {
        ...
        mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST);

The recovery path in mana_gd_probe() and the rescan in mana_serv_reset()
check only err == -ETIMEDOUT || err == -EPROTO.

Suppose another command, such as ethtool PHY stats, vport config or an
RDMA command, times out first. Would the gf_stats work then keep getting
-EBUSY every 2 seconds, so the HWC reset is never scheduled and the device
stays wedged?

Logging is affected too. Once mana_gd_cancel_hwc_timeout() sets the
timeout to 0 in mana_serv_reset(), the first command sent during
mana_gd_suspend() is quarantined immediately. Every later command returns
-EBUSY. Since mana_need_log() only suppresses -ETIMEDOUT, each of them is
logged with dev_err.

The commit message describes -EBUSY as admission pressure that "must not
make callers start dead-channel recovery". When abandoned requests hold
the slots, isn't -EBUSY actually a sign of a dead channel?

[ ... ]

> @@ -97,40 +162,54 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  				 struct hwc_work_request *rx_req, u16 msg_id)
>  {
>  	const struct gdma_resp_hdr *resp_msg = rx_req->buf_va;
> +	struct gdma_resource *r = &hwc->inflight_msg_res;
>  	struct hwc_caller_ctx *ctx;
> +	bool release;
>  	int err;
>  
> -	if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
> +	spin_lock(&r->lock);
> +	if (!test_bit(msg_id, r->map)) {
> +		spin_unlock(&r->lock);
>  		dev_err(hwc->dev, "hwc_rx: invalid msg_id = %u\n", msg_id);
>  		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
>  		return;
>  	}

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. This path
reposts the receive buffer, but the out-of-range check in
mana_hwc_rx_event_handler() does not:

    msg_id = READ_ONCE(resp->response.hwc_msg_id);
    if (msg_id >= hwc->num_inflight_msg) {
        dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
        return;
    }

With a depth of 1, wouldn't a single out-of-range msg_id from the device
leave the RQ with no posted buffers for the rest of the channel's life?
With the new -EBUSY behaviour above, that starved channel would also never
trigger recovery.

>  
>  	ctx = hwc->caller_ctx + msg_id;
> -
>  	spin_lock(&ctx->lock);
> -	if (!ctx->output_buf) {
> +	spin_unlock(&r->lock);
> +
> +	/* Consume the response-side reference exactly once. This releases a
> +	 * quarantined slot after its late response arrives.
> +	 */
> +	release = ctx->resp_pending;
> +	ctx->resp_pending = false;

[Severity: High]
Can this slot be freed and handed to a new sender while the handler still
holds ctx->lock?

After r->lock is dropped, the handler keeps reading and writing
resp_pending, responded, output_buf and output_buflen, and it calls
memcpy() and complete(). When release is false, it does all of this
without holding a reference.

The last hwc_ctx_put() goes through mana_hwc_put_msg_index(). That
function clears the bit under r->lock only and never takes ctx->lock.
mana_hwc_get_msg_index() then reinitializes the slot, also under r->lock
only:

    refcount_set(&ctx->refcnt, 2);
    ctx->output_buf = resp;
    ctx->output_buflen = resp_len;
    ...
    ctx->responded = false;
    ctx->resp_pending = true;

Take a duplicate response for a msg_id whose first response has already
completed the request. The responded branch suggests duplicates are meant
to be tolerated:

CPU1 (duplicate response)      CPU2 (original sender)
mana_hwc_handle_resp()
  test_bit() succeeds
  spin_lock(&ctx->lock)
  spin_unlock(&r->lock)
                               mana_hwc_wait_for_response()
                                 /* already past its ctx->lock section */
                                 hwc_ctx_put()   /* last reference */
                                   bitmap_clear(r->map, ...)
                                   up(&hwc->sema)

CPU3 (new sender)
mana_hwc_get_msg_index()
  rewrites refcnt, output_buf, output_buflen, resp_pending, responded
  under r->lock only, while CPU1 still holds ctx->lock

Possible outcomes:

The duplicate can overwrite the new resp_pending with false. Or the new
sender's mana_hwc_submit_request() can see responded == true and fail
with -ENODEV. Either way refcnt never reaches zero, so the slot and its
permit leak until teardown, and with depth 1 every later request gets
-EBUSY.

The stale duplicate can be copied into the new caller's output buffer and
complete that request.

On weakly ordered CPUs, the handler could pair a new output_buf with a
stale output_buflen, or with NULL.

The struct hwc_caller_ctx comment says "Once its bitmap bit is set, lock
protects every field below except msg_id and refcnt". The commit message
says the slot is released "only after both sides have finished".

Would it work to initialize the slot while holding ctx->lock nested inside
r->lock, which matches the existing r->lock then ctx->lock order? Or to
clear the bitmap bit while holding ctx->lock?

[ ... ]

> @@ -983,13 +1115,151 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)

[ ... ]

> +	err = mana_hwc_create_bootstrap_channel(hwc);
> +	if (err) {
> +		mana_hwc_release_channel(hwc);
> +		return err;
> +	}
> +
> +	err = mana_hwc_publish_channel(hwc);
> +	if (err) {
> +		mana_hwc_release_channel(hwc);
> +		return err;
> +	}

[Severity: Low]
mana_hwc_publish_channel() fails only when its WARN_ON_ONCE() finds the
slot occupied. In that case this calls the full mana_hwc_release_channel()
on the new, unpublished hwc.

That function also resets gc-wide state that belongs to whichever channel
holds the slot. It frees gc->cq_table, zeroes gc->max_num_cqs, clears
gc->hwc.gdma_context, and resets the doorbell and pdid.

If SMC teardown fails on this path, mana_hwc_retain_channel() finds the
slot occupied and only logs. The hwc and its queues are then leaked with
no owner.

Does this case need a narrower unwind? It can only happen if channel
creation runs concurrently without serialization, which the normal
lifecycle doesn't do.

[ ... ]

> @@ -1004,108 +1274,86 @@ static int mana_hwc_response_status(struct hw_channel_context *hwc,

[ ... ]

> +static int mana_hwc_wait_for_response(struct hw_channel_context *hwc,
> +				      struct hwc_caller_ctx *ctx,
> +				      u32 command)
>  {

[ ... ]

> +	if (wait_ms)
> +		dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
> +			command, wait_ms);
> +
> +	mana_hwc_timeout_reduce(hwc);
> +	hwc_ctx_put(hwc, ctx);
> +
> +	return -ETIMEDOUT;

[Severity: Medium]
This isn't a bug introduced by this patch, but mana_hwc_timeout_reduce()
permanently lowers hwc_timeout to 1 ms after a single timeout.

Once a late response frees the slot, later commands are posted to the
firmware and abandoned after about one jiffy, while the firmware may still
be executing them. Callers like mana_gd_create_mana_wq_cq() then free the
DMA memory those commands refer to.

The code before this series behaved the same way. This patch actually
posts less often, since a slot is reused only after its late response
arrives.

Could the driver and the device still end up disagreeing about state here,
with the device writing by DMA into freed memory?

[ ... ]

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-10-14  8:15 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-13  8:05 [PATCH net-next v7 0/4] net: mana: concurrent HWC requests and dynamic queue depth Wei Hu
2026-10-13  8:05 ` [PATCH net-next v7 1/4] net: mana: prepare HWC ownership for safe reinitialization Wei Hu
2026-10-14  8:06   ` sashiko-bot
2026-10-13  8:05 ` [PATCH net-next v7 2/4] net: mana: give each HWC message slot its own completion state Wei Hu
2026-10-14  8:06   ` sashiko-bot
2026-10-13  8:05 ` [PATCH net-next v7 3/4] net: mana: support concurrent HWC requests Wei Hu
2026-10-14  8:06   ` sashiko-bot
2026-10-13  8:05 ` [PATCH net-next v7 4/4] net: mana: add dynamic HWC queue depth with reinit path Wei Hu
2026-10-14  8:06   ` sashiko-bot
  -- strict thread matches above, loose matches on Subject: below --
2026-10-14  8:15 [PATCH net-next v7 3/4] net: mana: support concurrent HWC requests netdev-bot+sashiko

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