Re: [PATCH net 1/2] netpoll: use a raw lock for the deferred transmit queue

[netdev-bot+sashiko]

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] At this commit, queue_process() in net/core/netpoll.c still calls
  HARD_TX_LOCK(dev, txq, smp_processor_id()) (line 169) right after…
- [Low] The patch adds a raw_spin_lock_irqsave(&npinfo->txq_lock) to
  every netpoll send, through netpoll_txq_empty()…

--

netpoll: use a raw lock for the deferred transmit queue

This patch adds a raw spinlock, txq_lock, to struct netpoll_info. All
accesses to npinfo->txq now go through small helpers that take this lock
and use the unlocked sk_buff_head operations, so the queue can be touched
with hard IRQs disabled on PREEMPT_RT. On cleanup, the queue is spliced
to a private list before its skbs are freed.

> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index fe1e0cda5d6b..e0cfcb05468e 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -79,6 +79,68 @@ static netdev_tx_t netpoll_start_xmit(struct sk_buff *skb,
>  	return status;
>  }
>  
> +/*
> + * Transmit paths can access txq with hard IRQs disabled. Use a raw lock
> + * because the skb queue lock may sleep on PREEMPT_RT.
> + */
> +static bool netpoll_txq_empty(struct netpoll_info *npinfo)
> +{
> +	unsigned long flags;
> +	bool empty;
> +
> +	raw_spin_lock_irqsave(&npinfo->txq_lock, flags);
> +	empty = skb_queue_empty(&npinfo->txq);
> +	raw_spin_unlock_irqrestore(&npinfo->txq_lock, flags);
> +
> +	return empty;
> +}

[Severity: Low]
Is this lock needed here? Before the patch, __netpoll_send_skb() checked
the queue without a lock:

    if (skb_queue_len(&npinfo->txq) == 0 && !netpoll_owner_active(dev)) {

and the direct transmit fast path took no txq lock at all. With this
patch, every netpoll send takes txq_lock.

The empty check and the later netpoll_txq_queue_tail() are separate
critical sections, so the result of the check is only advisory in either
case. The writers (__skb_insert() and __skb_unlink()) already use
WRITE_ONCE().

Would skb_queue_empty_lockless() be enough here?

The new lock acquisition can also hang netconsole's atomic panic path.
Netconsole's write_atomic (CON_NBCON_ATOMIC_UNSAFE) is only called from
nbcon_atomic_flush_unsafe() at the end of vpanic(). That final flush
could spin forever on txq_lock in two cases:

  - the panic runs on a CPU that an NMI interrupted while it held
    txq_lock in netpoll_txq_dequeue(), netpoll_txq_queue_head() or
    netpoll_txq_queue_tail()

  - another CPU was NMI-stopped inside one of those critical sections,
    for example by crash_smp_send_stop() on x86 with
    crash_kexec_post_notifiers set and no kdump image loaded

In either case the panic log would not make it out over netconsole.

This does not block the panic_timeout reboot. In vpanic(),
emergency_restart() runs before nbcon_atomic_flush_unsafe(), so the hang
can only happen with panic_timeout == 0. The same atomic path already
takes other locks that are not NMI-safe: slab locks from
alloc_skb(GFP_ATOMIC), skb_dequeue() on nt->skb_pool, and driver locks
through netpoll_poll_dev().

[ ... ]

> @@ -107,7 +169,7 @@ static void queue_process(struct work_struct *work)
>  		HARD_TX_LOCK(dev, txq, smp_processor_id());
>  		if (netif_xmit_frozen_or_stopped(txq) ||
>  		    !dev_xmit_complete(netpoll_start_xmit(skb, dev, txq))) {
> -			skb_queue_head(&npinfo->txq, skb);
> +			netpoll_txq_queue_head(npinfo, skb);
>  			HARD_TX_UNLOCK(dev, txq);
>  			local_irq_restore(flags);

[Severity: Low]
The commit message says:

    The delayed transmit worker has the same problem when it requeues a busy
    skb with skb_queue_head() after disabling interrupts.

Does the worker actually reach this requeue on PREEMPT_RT? queue_process()
calls local_irq_save() and then HARD_TX_LOCK() before the requeue. For
devices without lltx, that becomes:

queue_process()
  local_irq_save(flags)
  HARD_TX_LOCK(dev, txq, smp_processor_id())
    __netif_tx_lock()
      spin_lock(&txq->_xmit_lock)

On PREEMPT_RT this spin_lock() is rt_spin_lock() with IRQs disabled. So
the worker would seem to splat on the tx lock before it gets to
netpoll_txq_queue_head().

The next commit in the series, "netpoll: avoid blocking on the transmit
lock in queue_process", replaces HARD_TX_LOCK() with HARD_TX_TRYLOCK()
and requeues on failure. The final state of the series is fine, and this
patch is no worse than the baseline.

Could the commit message say that the worker hits HARD_TX_LOCK() first,
and that the follow-up patch fixes that?

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