Re: [PATCH net-next] net: Add lockdep asserts to ____napi_schedule().

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2022-03-17 19:48:51 [-0600], Jason A. Donenfeld wrote:
> Hi Sebastian,
Hi Jason,

> I stumbled upon this commit when noticing a new failure in WireGuard's
> test suite:
…
> [    1.339289] WARNING: CPU: 0 PID: 11 at ../../../../../../../../net/core/dev.c:4268 __napi_schedule+0xa1/0x300
…
> [    1.352417]  wg_packet_decrypt_worker+0x2ac/0x470
…
> Sounds like wg_packet_decrypt_worker() might be doing something wrong? I
> vaguely recall a thread where you started looking into some things there
> that seemed non-optimal, but I didn't realize there were correctness
> issues. If your patch is correct, and wg_packet_decrypt_worker() is
> wrong, do you have a concrete idea of how we should approach fixing
> wireguard? Or do you want to send a patch for that?

In your case it is "okay" since that ptr_ring_consume_bh() will do BH
disable/enable which forces the softirq to run. It is not obvious. What
about the following:

diff --git a/drivers/net/wireguard/receive.c b/drivers/net/wireguard/receive.c
index 7b8df406c7737..26ffa3afa542e 100644
--- a/drivers/net/wireguard/receive.c
+++ b/drivers/net/wireguard/receive.c
@@ -502,15 +502,21 @@ void wg_packet_decrypt_worker(struct work_struct *work)
 	struct crypt_queue *queue = container_of(work, struct multicore_worker,
 						 work)->ptr;
 	struct sk_buff *skb;
+	unsigned int packets = 0;
 
-	while ((skb = ptr_ring_consume_bh(&queue->ring)) != NULL) {
+	local_bh_disable();
+	while ((skb = ptr_ring_consume(&queue->ring)) != NULL) {
 		enum packet_state state =
 			likely(decrypt_packet(skb, PACKET_CB(skb)->keypair)) ?
 				PACKET_STATE_CRYPTED : PACKET_STATE_DEAD;
 		wg_queue_enqueue_per_peer_rx(skb, state);
-		if (need_resched())
+		if (!(++packets % 4)) {
+			local_bh_enable();
 			cond_resched();
+			local_bh_disable();
+		}
 	}
+	local_bh_enable();
 }
 
 static void wg_packet_consume_data(struct wg_device *wg, struct sk_buff *skb)

It would decrypt 4 packets in a row and then after local_bh_enable() it
would invoke wg_packet_rx_poll() (assuming since it is the only napi
handler in wireguard) and after that it will attempt cond_resched() and
then continue with the next batch.

> Jason

Sebastian