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

[Sebastian Andrzej Siewior]

On 2026-10-02 01:30:45 [+0200], Karl Mehltretter wrote:
> > >   BUG: sleeping function called from invalid context
> > >   in_atomic(): 0, irqs_disabled(): 1, non_block: 0
> > >   rt_spin_lock
> > >   skb_queue_tail
> > >   netpoll_send_skb
> > 
> > How is this possible? netpoll is only used by netconsole right? And this
> > is CON_NBCON so it only prints threaded. What is the missing piece?
> > 
> 
> The call is from the NBCON printer thread, but netpoll_send_skb()
> explicitly disables hard interrupts around __netpoll_send_skb():
> 
>         local_irq_save(flags);
>         ret = __netpoll_send_skb(np, skb);
>         local_irq_restore(flags);
> 
> When direct transmission cannot complete, __netpoll_send_skb() calls
> skb_queue_tail() before interrupts are restored. Its spinlock can sleep
> on PREEMPT_RT despite the caller being a thread.

We have netconsole as a user of netpoll. The netconsole user is nbcon.
There are users such as macvlan and dsa and this looks like not a real
user but just forwarding the netpoll packet. According to the history
for macvlan, it is just there to forward the netconsole packets so it
appears to check out.

So it is just netconsole which is NBCON but has CON_NBCON_ATOMIC_UNSAFE.

In the ::write_thread() case the interrupts are disabled due invoking
::device_lock(). In the ::write_atomic() the lock function might not be
invoked but due to the nature of the situation the interrupts will be
disabled anyway. So disabling interrupts looks like a lazy way of
letting everyone know that ::ndo_start_xmit() will be invoked from
netpoll/ netconsole.
I don't see anything that would mandate disabling interrupts in
__netpoll_send_skb() except BH need to be disabled before
HARD_TX_TRYLOCK(). 

I would suggest to untangle that local-irq-disable assumption. Then we
end up with the ::write_atomic callback on PREEMPT_RT which will raise
warnings. But those will appear only on panic() and as the last console
due to CON_NBCON_ATOMIC_UNSAFE so everything will go according to the
plan.
On !RT the whole __netpoll_send_skb() will be with disabled interrupts
due to the console lock. On RT it won't and I don't know if anything
down the call chain assumes that.

To illustrate my idea a bit

diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index 1c6b1eec5efd6..3853611f672be 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -71,6 +71,9 @@ void netpoll_zap_completion_queue(void);
 unsigned int netpoll_get_carrier_timeout(void);
 
 #ifdef CONFIG_NETPOLL
+
+DECLARE_PER_CPU(atomic_t, netpoll_active);
+
 static inline void *netpoll_poll_lock(struct napi_struct *napi)
 {
 	struct net_device *dev = napi->dev;
@@ -96,7 +99,7 @@ static inline void netpoll_poll_unlock(void *have)
 
 static inline bool netpoll_tx_running(struct net_device *dev)
 {
-	return irqs_disabled();
+	return atomic_read(this_cpu_ptr(&netpoll_active)) > 0;
 }
 
 #else
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index fe1e0cda5d6bf..8ecdac601ca13 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -73,7 +73,9 @@ static netdev_tx_t netpoll_start_xmit(struct sk_buff *skb,
 		}
 	}
 
+	/* local_irq_save() ? */
 	status = netdev_start_xmit(skb, dev, txq, false);
+	/* local_irq_restore() ? */
 
 out:
 	return status;
@@ -258,7 +260,8 @@ static int netpoll_owner_active(struct net_device *dev)
 	return 0;
 }
 
-/* call with IRQ disabled */
+DEFINE_PER_CPU(atomic_t, netpoll_active) = ATOMIC_INIT(0);
+
 static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
 {
 	netdev_tx_t status = NETDEV_TX_BUSY;
@@ -268,12 +271,11 @@ static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
 	/* It is up to the caller to keep npinfo alive. */
 	struct netpoll_info *npinfo;
 
-	lockdep_assert_irqs_disabled();
-
 	dev = np->dev;
 	/* npinfo->txq belongs to np->dev, so retries must stay bound to it. */
 	skb->dev = dev;
-	rcu_read_lock();
+	rcu_read_lock_bh();
+	atomic_inc(this_cpu_ptr(&netpoll_active));
 	npinfo = rcu_dereference_bh(dev->npinfo);
 
 	if (!npinfo || !netif_running(dev) || !netif_device_present(dev)) {
@@ -306,11 +308,6 @@ static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
 
 			udelay(USEC_PER_POLL);
 		}
-
-		WARN_ONCE(!irqs_disabled(),
-			"netpoll_send_skb_on_dev(): %s enabled interrupts in poll (%pS)\n",
-			dev->name, dev->netdev_ops->ndo_start_xmit);
-
 	}
 
 	if (!dev_xmit_complete(status)) {
@@ -319,7 +316,8 @@ static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
 	}
 	ret = NETDEV_TX_OK;
 out:
-	rcu_read_unlock();
+	atomic_dec(this_cpu_ptr(&netpoll_active));
+	rcu_read_unlock_bh();
 	return ret;
 }
 
@@ -332,9 +330,7 @@ netdev_tx_t netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
 		dev_kfree_skb_irq(skb);
 		ret = NET_XMIT_DROP;
 	} else {
-		local_irq_save(flags);
 		ret = __netpoll_send_skb(np, skb);
-		local_irq_restore(flags);
 	}
 	return ret;
 }


and this did not even see the compiler. Plus queue_process() has been
ignored.

> Thanks,
> Karl

Sebastian