All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] net/mana: fix Rx interrupt vector beyond device count
@ 2026-11-02  7:08 Orgad Shaneh
  2026-11-02  7:08 ` [PATCH 2/2] net/mana: fix Rx interrupt enable failing on lock contention Orgad Shaneh
  0 siblings, 1 reply; 11+ messages in thread
From: Orgad Shaneh @ 2026-11-02  7:08 UTC (permalink / raw)
  To: dev; +Cc: Wei Hu, Long Li, Stephen Hemminger, stable

With Rx interrupts enabled, the CQ of Rx queue i is created on
completion vector i. The device may expose fewer completion vectors
than Rx queues (the RHEL 8.10 mana_ib driver reports one), so
ibv_create_cq() fails with EINVAL for queue 1 and the port does not
start.

Spread the queues over the vectors the device has.

Fixes: afd5d170727e ("net/mana: support Rx interrupts")
Cc: stable@dpdk.org

Signed-off-by: Orgad Shaneh <orgads@gmail.com>
---
Builds on main (meson -Dwerror=true, gcc 14, rdma-core 56).
In use on 26.07 on an Azure MANA VF.

 .mailmap              | 1 +
 drivers/net/mana/rx.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index 08317e81e6..61d5c6df00 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1265,6 +1265,7 @@ Omkar Maslekar <omkar.maslekar@intel.com>
 Omri Mor <omrimor2@illinois.edu>
 Ophir Munk <ophirmu@nvidia.com> <ophirmu@mellanox.com>
 Or Ami <ora@mellanox.com>
+Orgad Shaneh <orgads@gmail.com>
 Ori Kam <orika@nvidia.com> <orika@mellanox.com>
 Ouyang Hao <ouyanghao@tencent.com>
 Owen Hilyard <ohilyard@iol.unh.edu>
diff --git a/drivers/net/mana/rx.c b/drivers/net/mana/rx.c
index 274c6e88ce..aa21136167 100644
--- a/drivers/net/mana/rx.c
+++ b/drivers/net/mana/rx.c
@@ -289,7 +289,8 @@ mana_start_rx_queues(struct rte_eth_dev *dev)
 
 		rxq->cq = ibv_create_cq(priv->ib_ctx, rxq->num_desc,
 					NULL, rxq->channel,
-					rxq->channel ? i : 0);
+					rxq->channel ?
+					i % priv->ib_ctx->num_comp_vectors : 0);
 		if (!rxq->cq) {
 			ret = -errno;
 			DRV_LOG(ERR, "failed to create rx cq queue %d", i);
-- 
2.53.0


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

* [PATCH 2/2] net/mana: fix Rx interrupt enable failing on lock contention
  2026-11-02  7:08 [PATCH 1/2] net/mana: fix Rx interrupt vector beyond device count Orgad Shaneh
@ 2026-11-02  7:08 ` Orgad Shaneh
  2026-11-02  8:12   ` Stephen Hemminger
                     ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Orgad Shaneh @ 2026-11-02  7:08 UTC (permalink / raw)
  To: dev; +Cc: Wei Hu, Long Li, Stephen Hemminger, stable

Rx interrupt enable and disable take the per-device reset_ops_lock
with a trylock and return -EBUSY when it is held. Every Rx queue
takes that lock on each enable and disable, so with two or more
queues on different lcores the calls collide routinely. A failed
enable leaves the CQ unarmed, and a thread that then sleeps in
rte_epoll_wait() is not woken by the next packet; it sleeps until
its timeout with packets in the ring. A failed disable leaves the
event unconsumed. Applications do not expect -EBUSY from these
calls outside a reset, and l3fwd-power ignores the return value.

Seen on Azure (MANA VF) with 2 queues and a 10 ms epoll timeout:
about 550 lost wakeups per queue in one run, p99 latency 9.9 ms,
and 22.7% packet loss at the higher rate tested. With one queue
there was no loss.

The section under the lock is a doorbell write or a non-blocking
read of the completion channel, so wait for the lock instead.
The device state check is kept, so the calls still return -EBUSY
while a reset is in progress; a caller that arrives while the reset
thread holds the lock waits for that step to finish first.

Fixes: 077ca514d641 ("net/mana: support device reset")
Cc: stable@dpdk.org

Signed-off-by: Orgad Shaneh <orgads@gmail.com>
---
Builds on main (meson -Dwerror=true, gcc 14, rdma-core 56).
Tested on Azure D2s_v6 (MANA VF, Debian 13, kernel 7.2.11), main + 1/2
with and without this patch, 2 Rx queues, 10 ms epoll timeout, no
retry in the application, one 12 s run per rate:

  pps each way   without: loss / p99 / lost wakeups / -EBUSY   with
  25k            0 %     /  8.3 ms /  300 /   913             0 % / 78 us / 0 / 0
  100k           0.047 % /  9.9 ms / 2232 /  7733             0 % / 88 us / 0 / 0
  200k           22.8 %  / 10.0 ms / 2472 / 10093             0 % / 87 us / 0 / 0

With the patch, 400k pps on 2 queues and 100k on 1 queue are also clean.

An alternative that never blocks is to guard these two calls with the
per-queue burst_state gate instead of the device mutex (the reset path
already blocks and drains it before teardown). It is a larger change,
and the dev_state check would still be needed after a failed reset,
which clears burst_state. Happy to switch if you prefer it.

 drivers/net/mana/mana.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mana/mana.c b/drivers/net/mana/mana.c
index 0ea6d9a54a..a25c7590ef 100644
--- a/drivers/net/mana/mana.c
+++ b/drivers/net/mana/mana.c
@@ -449,6 +449,24 @@ mana_reset_trylock(struct mana_priv *priv)
 	return 0;
 }
 
+/*
+ * Like mana_reset_trylock(), but wait for the lock. Used by Rx interrupt
+ * enable/disable, which every queue calls on each sleep and wakeup: they
+ * must not fail just because another queue holds the lock for a moment.
+ */
+static int
+mana_reset_lock(struct mana_priv *priv)
+{
+	pthread_mutex_lock(&priv->reset_ops_lock);
+
+	if (rte_atomic_load_explicit(&priv->dev_state,
+	    rte_memory_order_acquire) != MANA_DEV_ACTIVE) {
+		pthread_mutex_unlock(&priv->reset_ops_lock);
+		return -EBUSY;
+	}
+	return 0;
+}
+
 static int
 mana_dev_info_get_lock(struct rte_eth_dev *dev,
 		       struct rte_eth_dev_info *dev_info)
@@ -1126,7 +1144,7 @@ mana_rx_intr_enable_lock(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 	struct mana_priv *priv = dev->data->dev_private;
 	int ret;
 
-	if (mana_reset_trylock(priv))
+	if (mana_reset_lock(priv))
 		return -EBUSY;
 	ret = mana_rx_intr_enable(dev, rx_queue_id);
 	pthread_mutex_unlock(&priv->reset_ops_lock);
@@ -1139,7 +1157,7 @@ mana_rx_intr_disable_lock(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 	struct mana_priv *priv = dev->data->dev_private;
 	int ret;
 
-	if (mana_reset_trylock(priv))
+	if (mana_reset_lock(priv))
 		return -EBUSY;
 	ret = mana_rx_intr_disable(dev, rx_queue_id);
 	pthread_mutex_unlock(&priv->reset_ops_lock);
-- 
2.53.0


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

* Re: [PATCH 2/2] net/mana: fix Rx interrupt enable failing on lock contention
  2026-11-02  7:08 ` [PATCH 2/2] net/mana: fix Rx interrupt enable failing on lock contention Orgad Shaneh
@ 2026-11-02  8:12   ` Stephen Hemminger
  2026-11-02  8:27     ` Orgad Shaneh
  2026-11-09 11:25   ` [EXTERNAL] " Wei Hu
  2026-11-09 22:21   ` Stephen Hemminger
  2 siblings, 1 reply; 11+ messages in thread
From: Stephen Hemminger @ 2026-11-02  8:12 UTC (permalink / raw)
  To: Orgad Shaneh; +Cc: dev, Wei Hu, Long Li, stable

On Mon,  2 Nov 2026 07:08:09 +0000
Orgad Shaneh <orgads@gmail.com> wrote:

> Fixes: 077ca514d641 ("net/mana: support device reset")
> Cc: stable@dpdk.org
 

Since this code didnt go into a previous stable release
doing cc to stable is not needed

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

* Re: [PATCH 2/2] net/mana: fix Rx interrupt enable failing on lock contention
  2026-11-02  8:12   ` Stephen Hemminger
@ 2026-11-02  8:27     ` Orgad Shaneh
  0 siblings, 0 replies; 11+ messages in thread
From: Orgad Shaneh @ 2026-11-02  8:27 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, Wei Hu, Long Li, stable

Stephen Hemminger <stephen@networkplumber.org> wrote:
> > Fixes: 077ca514d641 ("net/mana: support device reset")
> > Cc: stable@dpdk.org
>
> Since this code didnt go into a previous stable release
> doing cc to stable is not needed

Right, reset support is only in 26.07. Please drop the Cc: stable
when applying, or I can send a v2 without it if you prefer.

1/2 fixes code from 22.11, so I left Cc: stable there.

On the AI review of this patch: reset_ops_lock is a default-type,
non-robust mutex, so pthread_mutex_lock() cannot return an error
here, and the existing calls in the reset path do not check it
either.

The iol-unit-arm64 failure is memif_pmd_autotest crashing on
Fedora 44, which this patch does not touch.

Recheck-request: iol-unit-arm64-testing

Thanks,
Orgad

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

* RE: [EXTERNAL] [PATCH 2/2] net/mana: fix Rx interrupt enable failing on lock contention
  2026-11-02  7:08 ` [PATCH 2/2] net/mana: fix Rx interrupt enable failing on lock contention Orgad Shaneh
  2026-11-02  8:12   ` Stephen Hemminger
@ 2026-11-09 11:25   ` Wei Hu
  2026-11-09 14:02     ` Orgad Shaneh
  2026-11-09 22:21   ` Stephen Hemminger
  2 siblings, 1 reply; 11+ messages in thread
From: Wei Hu @ 2026-11-09 11:25 UTC (permalink / raw)
  To: Orgad Shaneh, dev@dpdk.org; +Cc: Long Li, Stephen Hemminger, Aditya Garg

+ Aditya
- stable@dpdk.org

Hi Orgad,

> -----Original Message-----
> From: Orgad Shaneh <orgads@gmail.com>
> Sent: Monday, November 2, 2026 3:08 PM
> To: dev@dpdk.org
> Cc: Wei Hu <weh@microsoft.com>; Long Li <longli@kernel.org>; Stephen
> Hemminger <stephen@networkplumber.org>; stable@dpdk.org
> Subject: [EXTERNAL] [PATCH 2/2] net/mana: fix Rx interrupt enable failing on
> lock contention
> 
> [You don't often get email from orgads@gmail.com. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Rx interrupt enable and disable take the per-device reset_ops_lock with a
> trylock and return -EBUSY when it is held. Every Rx queue takes that lock on
> each enable and disable, so with two or more queues on different lcores the
> calls collide routinely. A failed enable leaves the CQ unarmed, and a thread that
> then sleeps in
> rte_epoll_wait() is not woken by the next packet; it sleeps until its timeout
> with packets in the ring. A failed disable leaves the event unconsumed.
> Applications do not expect -EBUSY from these calls outside a reset, and l3fwd-
> power ignores the return value.
> 
> Seen on Azure (MANA VF) with 2 queues and a 10 ms epoll timeout:
> about 550 lost wakeups per queue in one run, p99 latency 9.9 ms, and 22.7%
> packet loss at the higher rate tested. With one queue there was no loss.
> 
> The section under the lock is a doorbell write or a non-blocking read of the
> completion channel, so wait for the lock instead.
> The device state check is kept, so the calls still return -EBUSY while a reset is in
> progress; a caller that arrives while the reset thread holds the lock waits for
> that step to finish first.
> 
> Fixes: 077ca514d641 ("net/mana: support device reset")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Orgad Shaneh <orgads@gmail.com>
> ---
> Builds on main (meson -Dwerror=true, gcc 14, rdma-core 56).
> Tested on Azure D2s_v6 (MANA VF, Debian 13, kernel 7.2.11), main + 1/2
> with and without this patch, 2 Rx queues, 10 ms epoll timeout, no retry in the
> application, one 12 s run per rate:
> 
>   pps each way   without: loss / p99 / lost wakeups / -EBUSY   with
>   25k            0 %     /  8.3 ms /  300 /   913             0 % / 78 us / 0 / 0
>   100k           0.047 % /  9.9 ms / 2232 /  7733             0 % / 88 us / 0 / 0
>   200k           22.8 %  / 10.0 ms / 2472 / 10093             0 % / 87 us / 0 / 0
> 
> With the patch, 400k pps on 2 queues and 100k on 1 queue are also clean.
> 
> An alternative that never blocks is to guard these two calls with the per-queue
> burst_state gate instead of the device mutex (the reset path already blocks
> and drains it before teardown). It is a larger change, and the dev_state check
> would still be needed after a failed reset, which clears burst_state. Happy to
> switch if you prefer it.
> 
>  drivers/net/mana/mana.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/mana/mana.c b/drivers/net/mana/mana.c index
> 0ea6d9a54a..a25c7590ef 100644
> --- a/drivers/net/mana/mana.c
> +++ b/drivers/net/mana/mana.c
> @@ -449,6 +449,24 @@ mana_reset_trylock(struct mana_priv *priv)
>         return 0;
>  }
> 
> +/*
> + * Like mana_reset_trylock(), but wait for the lock. Used by Rx
> +interrupt
> + * enable/disable, which every queue calls on each sleep and wakeup:
> +they
> + * must not fail just because another queue holds the lock for a moment.
> + */
> +static int
> +mana_reset_lock(struct mana_priv *priv) {
> +       pthread_mutex_lock(&priv->reset_ops_lock);
> +
> +       if (rte_atomic_load_explicit(&priv->dev_state,
> +           rte_memory_order_acquire) != MANA_DEV_ACTIVE) {
> +               pthread_mutex_unlock(&priv->reset_ops_lock);
> +               return -EBUSY;
> +       }
> +       return 0;
> +}
> +

App threads could get stuck as long as 15 seconds waiting for the lock when 
the device is in reset. Is it better to add following?

static int
mana_reset_lock(struct mana_priv *priv)
{
+        if (rte_atomic_load_explicit(&priv->dev_state,
+                       rte_memory_order_relaxed) != MANA_DEV_ACTIVE)
+                return -EBUSY;

        pthread_mutex_lock(&priv->reset_ops_lock);

        if (rte_atomic_load_explicit(&priv->dev_state,
                        rte_memory_order_acquire) != MANA_DEV_ACTIVE) {
                pthread_mutex_unlock(&priv->reset_ops_lock);
                return -EBUSY;
        }
        return 0;
}

Thanks,
Wei

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

* Re: [PATCH 2/2] net/mana: fix Rx interrupt enable failing on lock contention
  2026-11-09 11:25   ` [EXTERNAL] " Wei Hu
@ 2026-11-09 14:02     ` Orgad Shaneh
  2026-11-10 14:33       ` Stephen Hemminger
  0 siblings, 1 reply; 11+ messages in thread
From: Orgad Shaneh @ 2026-11-09 14:02 UTC (permalink / raw)
  To: Wei Hu; +Cc: dev, Long Li, Stephen Hemminger, Aditya Garg

Hi Wei,

Wei Hu <weh@microsoft.com> wrote:
> App threads could get stuck as long as 15 seconds waiting for the lock when
> the device is in reset. Is it better to add following?

The 15 s recovery wait in mana_reset_thread() runs with reset_ops_lock
released and dev_state at RESET_EXIT, so a caller in that window takes
the lock, sees the state and returns -EBUSY at once. The lock is held
during teardown (dev_stop, the RESET_ENTER request to secondaries with
its 5 s IPC timeout, dev_close) and during re-probe, and a caller that
arrives then waits for that step.

Your early check fixes that: both steps run with dev_state already
non-ACTIVE, so the caller returns -EBUSY without touching the lock.

One window stays open. A caller can pass the check just before
mana_intr_handler() takes the lock, then block through
mana_reset_enter() and, if the reset thread wins the lock next,
through the whole teardown. Checking the state while waiting closes
that too, and it is no bigger:

static int
mana_reset_lock(struct mana_priv *priv)
{
	while (pthread_mutex_trylock(&priv->reset_ops_lock)) {
		if (rte_atomic_load_explicit(&priv->dev_state,
		    rte_memory_order_relaxed) != MANA_DEV_ACTIVE)
			return -EBUSY;
		rte_pause();
	}

	if (rte_atomic_load_explicit(&priv->dev_state,
	    rte_memory_order_acquire) != MANA_DEV_ACTIVE) {
		pthread_mutex_unlock(&priv->reset_ops_lock);
		return -EBUSY;
	}
	return 0;
}

Outside a reset the lock is only held by another queue's doorbell
write or event read, so the spin is short. A caller then never waits
on a reset step at all.

I'll send v2 with this, without Cc: stable, unless you prefer your
version.

Thanks,
Orgad

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

* Re: [PATCH 2/2] net/mana: fix Rx interrupt enable failing on lock contention
  2026-11-02  7:08 ` [PATCH 2/2] net/mana: fix Rx interrupt enable failing on lock contention Orgad Shaneh
  2026-11-02  8:12   ` Stephen Hemminger
  2026-11-09 11:25   ` [EXTERNAL] " Wei Hu
@ 2026-11-09 22:21   ` Stephen Hemminger
  2026-11-10  9:42     ` [PATCH] net/mana: do not block Rx interrupt calls during a reset Orgad Shaneh
  2 siblings, 1 reply; 11+ messages in thread
From: Stephen Hemminger @ 2026-11-09 22:21 UTC (permalink / raw)
  To: Orgad Shaneh; +Cc: dev, Wei Hu, Long Li, stable

On Mon,  2 Nov 2026 07:08:09 +0000
Orgad Shaneh <orgads@gmail.com> wrote:

> Rx interrupt enable and disable take the per-device reset_ops_lock
> with a trylock and return -EBUSY when it is held. Every Rx queue
> takes that lock on each enable and disable, so with two or more
> queues on different lcores the calls collide routinely. A failed
> enable leaves the CQ unarmed, and a thread that then sleeps in
> rte_epoll_wait() is not woken by the next packet; it sleeps until
> its timeout with packets in the ring. A failed disable leaves the
> event unconsumed. Applications do not expect -EBUSY from these
> calls outside a reset, and l3fwd-power ignores the return value.
> 
> Seen on Azure (MANA VF) with 2 queues and a 10 ms epoll timeout:
> about 550 lost wakeups per queue in one run, p99 latency 9.9 ms,
> and 22.7% packet loss at the higher rate tested. With one queue
> there was no loss.
> 
> The section under the lock is a doorbell write or a non-blocking
> read of the completion channel, so wait for the lock instead.
> The device state check is kept, so the calls still return -EBUSY
> while a reset is in progress; a caller that arrives while the reset
> thread holds the lock waits for that step to finish first.
> 
> Fixes: 077ca514d641 ("net/mana: support device reset")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Orgad Shaneh <orgads@gmail.com>
> ---

Since this is better than current main I applied it to next-net

AI review noted:

Patch 2/2
---------

Info: blocking can be long while a reset is in progress.

The reset thread holds reset_ops_lock across mana_dev_stop(),
mana_mp_req_on_rxtx(RESET_ENTER) and mana_dev_close(). On reset
exit it holds it across ibv_close_device(), mana_pci_probe(),
mana_mp_req_on_rxtx(RESET_EXIT) and mana_dev_start(). Each mp
request is rte_mp_request_sync() with MANA_MP_REQ_TIMEOUT_SEC (5 s).
An lcore that calls rte_eth_dev_rx_intr_enable() during one of
those steps can block for several seconds instead of getting
-EBUSY at once.

No deadlock results: mana_reset_enter() only spins on the
burst_state busy bit, which an lcore in rx_intr_enable/disable
does not hold, and the ethdev event callbacks are all emitted after
the lock is dropped. This is acceptable for a stable fix. It would
help to say "may block for the duration of the reset teardown or
re-probe step" in the commit message, rather than "waits for that
step to finish".

The burst_state gate mentioned below the --- is the better
long-term design, since it avoids putting every queue's
sleep/wakeup through one device mutex. Fine as a follow-up for
main; not needed for this fix.


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

* [PATCH] net/mana: do not block Rx interrupt calls during a reset
  2026-11-09 22:21   ` Stephen Hemminger
@ 2026-11-10  9:42     ` Orgad Shaneh
  2026-11-10 14:47       ` Stephen Hemminger
  0 siblings, 1 reply; 11+ messages in thread
From: Orgad Shaneh @ 2026-11-10  9:42 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, Wei Hu, Long Li, Aditya Garg

Rx interrupt enable and disable wait for reset_ops_lock and then
return -EBUSY if a reset is in progress. The reset thread holds that
lock across the teardown (mana_dev_stop(), the RESET_ENTER request
to secondary processes, mana_dev_close()) and across the re-probe,
and each request to the secondaries may take up to
MANA_MP_REQ_TIMEOUT_SEC. An lcore that calls rx_intr_enable or
rx_intr_disable during one of those steps blocks for that long,
only to get -EBUSY.

Retry the trylock while the device is active, and return -EBUSY as
soon as it is not. Outside a reset the lock is held only by another
queue's doorbell write or event read, so the wait stays short; once
a reset has started, the caller returns at once.

Suggested-by: Wei Hu <weh@microsoft.com>
Signed-off-by: Orgad Shaneh <orgads@gmail.com>
---
Applies on next-net for-main, on top of efa36b47334d ("net/mana: fix
Rx interrupt enable failing on lock contention"). Squash it into that
commit if you prefer.

Follow-up to Wei's review and the AI review note:
https://inbox.dpdk.org/dev/20261109142127.7a54ee80@phoenix.local/

Builds with meson -Dwerror=true (gcc 14, rdma-core 56). With no reset
in progress the calls take the lock and run the same code as before,
so the Azure results in efa36b47334d still apply. A MANA reset cannot
be triggered on demand on Azure, so the reset path is untested.

The burst_state gate stays a possible follow-up for main.

 drivers/net/mana/mana.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mana/mana.c b/drivers/net/mana/mana.c
index f624cde6b9..eea9dac253 100644
--- a/drivers/net/mana/mana.c
+++ b/drivers/net/mana/mana.c
@@ -451,14 +451,21 @@ mana_reset_trylock(struct mana_priv *priv)
 }
 
 /*
- * Like mana_reset_trylock(), but wait for the lock. Used by Rx interrupt
- * enable/disable, which every queue calls on each sleep and wakeup: they
- * must not fail just because another queue holds the lock for a moment.
+ * Like mana_reset_trylock(), but wait while another queue holds the lock.
+ * Used by Rx interrupt enable/disable, which every queue calls on each
+ * sleep and wakeup: they must not fail just because another queue holds
+ * the lock for a moment. Once a reset has started, fail at once instead
+ * of waiting for its teardown or re-probe step.
  */
 static int
 mana_reset_lock(struct mana_priv *priv)
 {
-	pthread_mutex_lock(&priv->reset_ops_lock);
+	while (pthread_mutex_trylock(&priv->reset_ops_lock)) {
+		if (rte_atomic_load_explicit(&priv->dev_state,
+		    rte_memory_order_relaxed) != MANA_DEV_ACTIVE)
+			return -EBUSY;
+		rte_pause();
+	}
 
 	if (rte_atomic_load_explicit(&priv->dev_state,
 	    rte_memory_order_acquire) != MANA_DEV_ACTIVE) {
-- 
2.52.0


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

* Re: [PATCH 2/2] net/mana: fix Rx interrupt enable failing on lock contention
  2026-11-09 14:02     ` Orgad Shaneh
@ 2026-11-10 14:33       ` Stephen Hemminger
  0 siblings, 0 replies; 11+ messages in thread
From: Stephen Hemminger @ 2026-11-10 14:33 UTC (permalink / raw)
  To: Orgad Shaneh; +Cc: Wei Hu, dev, Long Li, Aditya Garg

On Mon,  9 Nov 2026 14:02:07 +0000
Orgad Shaneh <orgads@gmail.com> wrote:

> static int
> mana_reset_lock(struct mana_priv *priv)
> {
> 	while (pthread_mutex_trylock(&priv->reset_ops_lock)) {
> 		if (rte_atomic_load_explicit(&priv->dev_state,
> 		    rte_memory_order_relaxed) != MANA_DEV_ACTIVE)
> 			return -EBUSY;
> 		rte_pause();
> 	}

You need to use pthread_yield(), rte_pause only generates a CPU rep nop
which yields to other hyper thread if it is another process need
to cause yield in cpu scheduler. 

> 
> 	if (rte_atomic_load_explicit(&priv->dev_state,
> 	    rte_memory_order_acquire) != MANA_DEV_ACTIVE) {
> 		pthread_mutex_unlock(&priv->reset_ops_lock);
> 		return -EBUSY;
> 	}
> 	return 0;
> }

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

* Re: [PATCH] net/mana: do not block Rx interrupt calls during a reset
  2026-11-10  9:42     ` [PATCH] net/mana: do not block Rx interrupt calls during a reset Orgad Shaneh
@ 2026-11-10 14:47       ` Stephen Hemminger
  2026-11-10 15:13         ` [PATCH v2] " Orgad Shaneh
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Hemminger @ 2026-11-10 14:47 UTC (permalink / raw)
  To: Orgad Shaneh; +Cc: dev, Wei Hu, Long Li, Aditya Garg

On Tue, 10 Nov 2026 09:42:43 +0000
Orgad Shaneh <orgads@gmail.com> wrote:

> Rx interrupt enable and disable wait for reset_ops_lock and then
> return -EBUSY if a reset is in progress. The reset thread holds that
> lock across the teardown (mana_dev_stop(), the RESET_ENTER request
> to secondary processes, mana_dev_close()) and across the re-probe,
> and each request to the secondaries may take up to
> MANA_MP_REQ_TIMEOUT_SEC. An lcore that calls rx_intr_enable or
> rx_intr_disable during one of those steps blocks for that long,
> only to get -EBUSY.
> 
> Retry the trylock while the device is active, and return -EBUSY as
> soon as it is not. Outside a reset the lock is held only by another
> queue's doorbell write or event read, so the wait stays short; once
> a reset has started, the caller returns at once.
> 
> Suggested-by: Wei Hu <weh@microsoft.com>
> Signed-off-by: Orgad Shaneh <orgads@gmail.com>
> ---

More detailed AI review of this.

Subject: Re: [PATCH] net/mana: do not block Rx interrupt calls during a reset

The reset-path logic is right. mana_intr_handler() stores
MANA_DEV_RESET_ENTER under reset_ops_lock before the reset thread
starts, the state stays non-ACTIVE through teardown (RESET_ENTER) and
re-probe (RESET_EXIT), and ACTIVE is only stored at the end of
re-probe with the lock still held. The relaxed load in the loop is
only a hint; the acquire load after the lock is taken is the one that
matters. The PCI remove path stores RESET_FAILED, which also ends the
spin.

Warning

The commit message and the new comment say that outside a reset the
lock is held only by another queue's doorbell write or event read.
That is not true. mana_dev_stop_lock() and mana_dev_close_lock() take
reset_ops_lock with dev_state still MANA_DEV_ACTIVE and hold it
across mana_dev_stop(), which calls
mana_mp_req_on_rxtx(dev, MANA_MP_REQ_STOP_RXTX). That request can
wait MANA_MP_REQ_TIMEOUT_SEC (5 s). dev_state does not change, so an
lcore in rx_intr_enable/disable now busy-spins with rte_pause() for up
to 5 s, where before it slept in pthread_mutex_lock(). The control
ops that use mana_reset_trylock() also hold the lock from a
control thread that can be preempted.

rte_pause() is only a CPU hint ("pause" on x86). It gives pipeline
resources to the SMT sibling, not the CPU to another thread. If the
lock holder is preempted on the same CPU (a control thread or a
secondary's thread sharing the lcore's core), the spinner keeps the
CPU until its time slice runs out, and the holder cannot release the
lock until then. Yield instead:

	while (pthread_mutex_trylock(&priv->reset_ops_lock)) {
		if (rte_atomic_load_explicit(&priv->dev_state,
		    rte_memory_order_relaxed) != MANA_DEV_ACTIVE)
			return -EBUSY;
		sched_yield();
	}

Use sched_yield() rather than pthread_yield(), which is a GNU
extension deprecated since glibc 2.34.

Calling rx_intr_* concurrently with dev_stop is an application bug.
But the wait is not "short", and the commit message should not say it
is. Either fix the wording:

  Outside a reset the lock is held by another queue's Rx interrupt
  call or by a control operation. The data path must already be
  stopped before dev_stop/dev_close, so the wait is short in a correct
  application.

and the same in the comment above mana_reset_lock(). Or stop the
queues from contending with each other in the first place. The only
reason rx_intr takes the device-wide lock is to exclude teardown.
A process-shared pthread_rwlock_t would do that without the spin:
rx_intr takes the read lock with tryrdlock and returns -EBUSY on
failure, and control ops and the reset thread take the write lock.
Queues never block each other, and a writer of any kind fails the
call immediately. That fits better with the burst_state follow-up
for main than with this patch, so the wording fix is enough here.

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

* [PATCH v2] net/mana: do not block Rx interrupt calls during a reset
  2026-11-10 14:47       ` Stephen Hemminger
@ 2026-11-10 15:13         ` Orgad Shaneh
  0 siblings, 0 replies; 11+ messages in thread
From: Orgad Shaneh @ 2026-11-10 15:13 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, Wei Hu, Long Li, Aditya Garg

Rx interrupt enable and disable wait for reset_ops_lock and then
return -EBUSY if a reset is in progress. The reset thread holds that
lock across the teardown (mana_dev_stop(), the RESET_ENTER request
to secondary processes, mana_dev_close()) and across the re-probe,
and each request to the secondaries may take up to
MANA_MP_REQ_TIMEOUT_SEC. An lcore that calls rx_intr_enable or
rx_intr_disable during one of those steps blocks for that long,
only to get -EBUSY.

Retry the trylock while the device is active, yielding the CPU in
between, and return -EBUSY as soon as it is not. Outside a reset the
lock is held by another queue's Rx interrupt call or by a control
operation. The data path must already be stopped before
dev_stop/dev_close, so the wait is short in a correct application.

Suggested-by: Wei Hu <weh@microsoft.com>
Signed-off-by: Orgad Shaneh <orgads@gmail.com>
---
v2:
- Yield with sched_yield() instead of rte_pause(), so a preempted
  lock holder on the same CPU can run (Stephen).
- Fix the commit message and comment: outside a reset the lock is
  also held by control operations, so the wait is short only in an
  application that stops the data path before dev_stop/dev_close.

Applies on next-net for-main, on top of efa36b47334d ("net/mana: fix
Rx interrupt enable failing on lock contention"). Squash it into that
commit if you prefer.

Builds with meson -Dwerror=true (gcc 14, rdma-core 56). A MANA reset
cannot be triggered on demand on Azure, so the reset path is untested.

The rwlock (tryrdlock in rx_intr, write lock in control ops and the
reset thread) fits the burst_state follow-up for main; I'll look at
it there.

 drivers/net/mana/mana.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mana/mana.c b/drivers/net/mana/mana.c
index f624cde6b9..71a3fd20d7 100644
--- a/drivers/net/mana/mana.c
+++ b/drivers/net/mana/mana.c
@@ -2,6 +2,7 @@
  * Copyright 2022 Microsoft Corporation
  */
 
+#include <sched.h>
 #include <unistd.h>
 #include <dirent.h>
 #include <fcntl.h>
@@ -451,14 +452,24 @@ mana_reset_trylock(struct mana_priv *priv)
 }
 
 /*
- * Like mana_reset_trylock(), but wait for the lock. Used by Rx interrupt
- * enable/disable, which every queue calls on each sleep and wakeup: they
- * must not fail just because another queue holds the lock for a moment.
+ * Like mana_reset_trylock(), but wait while the device is active. Used by
+ * Rx interrupt enable/disable, which every queue calls on each sleep and
+ * wakeup: they must not fail just because another queue holds the lock
+ * for a moment. Outside a reset the lock is held by another queue's Rx
+ * interrupt call or by a control operation; the data path must be stopped
+ * before dev_stop/dev_close, so in a correct application the wait is
+ * short. Once a reset has started, fail at once instead of waiting for
+ * its teardown or re-probe step.
  */
 static int
 mana_reset_lock(struct mana_priv *priv)
 {
-	pthread_mutex_lock(&priv->reset_ops_lock);
+	while (pthread_mutex_trylock(&priv->reset_ops_lock)) {
+		if (rte_atomic_load_explicit(&priv->dev_state,
+		    rte_memory_order_relaxed) != MANA_DEV_ACTIVE)
+			return -EBUSY;
+		sched_yield();
+	}
 
 	if (rte_atomic_load_explicit(&priv->dev_state,
 	    rte_memory_order_acquire) != MANA_DEV_ACTIVE) {
-- 
2.52.0


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

end of thread, other threads:[~2026-11-12  8:07 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-02  7:08 [PATCH 1/2] net/mana: fix Rx interrupt vector beyond device count Orgad Shaneh
2026-11-02  7:08 ` [PATCH 2/2] net/mana: fix Rx interrupt enable failing on lock contention Orgad Shaneh
2026-11-02  8:12   ` Stephen Hemminger
2026-11-02  8:27     ` Orgad Shaneh
2026-11-09 11:25   ` [EXTERNAL] " Wei Hu
2026-11-09 14:02     ` Orgad Shaneh
2026-11-10 14:33       ` Stephen Hemminger
2026-11-09 22:21   ` Stephen Hemminger
2026-11-10  9:42     ` [PATCH] net/mana: do not block Rx interrupt calls during a reset Orgad Shaneh
2026-11-10 14:47       ` Stephen Hemminger
2026-11-10 15:13         ` [PATCH v2] " Orgad Shaneh

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.