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

[Stephen Hemminger <stephen@networkplumber.org>]

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.