Re: [PATCH net 2/2] netpoll: avoid blocking on the transmit lock in queue_process

[netdev-bot+sashiko@kernel.org]

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

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

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] On PREEMPT_RT the patch does not fix the bug it claims to fix.
- [High] net: severe latency in netpoll queue_process on lock contention

--

netpoll: avoid blocking on the transmit lock in queue_process

This patch replaces HARD_TX_LOCK() with HARD_TX_TRYLOCK() in
queue_process() to avoid a sleeping function called from invalid context
splat on PREEMPT_RT. If the transmit lock is contended, the skb is put back
at the head of the deferred queue and the work is rescheduled after
HZ / 10.

> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index e0cfcb05468e..a3c94f0b3f76 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -166,7 +166,13 @@ static void queue_process(struct work_struct *work)
>  			skb_set_queue_mapping(skb, q_index);
>  		}
>  		txq = netdev_get_tx_queue(dev, q_index);
> -		HARD_TX_LOCK(dev, txq, smp_processor_id());
> +		if (!HARD_TX_TRYLOCK(dev, txq)) {
> +			netpoll_txq_queue_head(npinfo, skb);
> +			local_irq_restore(flags);
> +
> +			schedule_delayed_work(&npinfo->tx_work, HZ / 10);
> +			return;
> +		}
>  		if (netif_xmit_frozen_or_stopped(txq) ||
>  		    !dev_xmit_complete(netpoll_start_xmit(skb, dev, txq))) {

[Severity: High]
Is HZ / 10 the right backoff for a failed trylock?

The existing HZ/10 retry further down covers a frozen or stopped queue,
or a driver returning NETDEV_TX_BUSY. That usually means the ring is
full and will take some time to drain.

A failed HARD_TX_TRYLOCK() here means something much smaller: another
CPU is inside ndo_start_xmit() on the same txq. That lock is normally
held for only a few microseconds.

With this change, every time queue_process() races with regular
traffic on that queue, the deferred skb waits 100ms. So does everything
queued behind it in npinfo->txq.

__netpoll_send_skb() only transmits directly when npinfo->txq is empty.
That means new netconsole messages keep being appended to the deferred
queue for the whole backoff as well.

On a busy interface this can cut netconsole down to a handful of
packets per second per queue. The deferred queue keeps growing and
console output falls further and further behind. Anything still sitting
in npinfo->txq when the machine hangs is never sent, and those are
exactly the messages netconsole users care about most.

This also changes behaviour on !PREEMPT_RT kernels. There,
HARD_TX_LOCK() was not a problem and simply waited out the short
critical section.

Could the retry on trylock failure be much shorter, for example a delay
of 0 or 1 jiffy? Alternatively, would it be better to avoid the trylock
altogether by not running this path with hard IRQs disabled, so the
normal tx lock can be taken?

[Severity: High]
Does this fully fix the PREEMPT_RT splat described in the commit message?

The earlier local_irq_save(flags) in queue_process() is unchanged. That
means hard IRQs are still disabled when the driver's ndo_start_xmit() runs
after a successful trylock:

queue_process()
  local_irq_save(flags);
  HARD_TX_TRYLOCK(dev, txq)
  netpoll_start_xmit()
    netdev_start_xmit()
      ndo_start_xmit()

Several non-lltx drivers take a spinlock_t in their xmit path. On
PREEMPT_RT a spinlock_t is an rtmutex. For example:

drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_xmit() {
    ...
	spin_lock(&ring->lock);
    ...
}

rtl8139_start_xmit() takes spin_lock_irqsave(&tp->lock, flags), and
macb_start_xmit() takes spin_lock_irqsave(&queue->tx_ptr_lock, flags).
HARD_TX_TRYLOCK() takes no core lock for lltx drivers, and those drivers do
their own spinlock_t locking, so they have the same problem.

On PREEMPT_RT, rt_spin_lock()->__rt_spin_lock() calls
rtlock_might_resched(), and __might_resched() prints "BUG: sleeping
function called from invalid context" whenever irqs_disabled() is true.
Won't the same splat still fire, just one frame deeper?

  rt_spin_lock
  bcmgenet_xmit
  netpoll_start_xmit
  queue_process

If the driver lock is contended, for example by threaded NAPI TX reclaim on
RT, rtlock_slowlock() can actually block while hard IRQs are disabled.

The trylock only seems to silence the txq lock case, because
__rt_spin_trylock() skips rtlock_might_resched(). The sibling path
__netpoll_send_skb() already uses HARD_TX_TRYLOCK() under local_irq_save(),
and it still runs driver xmit and netpoll_poll_dev() with IRQs disabled.

Documentation/networking/netdevices.rst says ndo_start_xmit "will be called
with interrupts disabled by netconsole". On RT, that conflicts with drivers
that use a plain spinlock_t in xmit.

queue_process() runs from a workqueue in process context. Would it be
better to stop disabling hard IRQs here instead of changing the lock
primitive? For example, it could use local_bh_disable() plus the normal tx
lock, as the regular xmit path does.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928064239.32456-1-kmehltretter%40gmail.com