Re: [PATCH net-next v3 05/18] xdp: Use nested-BH locking for system_page_pool

On 2025-05-02 16:33:10 [+0200], Toke Høiland-Jørgensen wrote:

quoted

@@ -751,16 +751,13 @@ struct sk_buff *xdp_build_skb_from_zc(struct xdp_buff *xdp)
 	local_lock_nested_bh(&system_page_pool.bh_lock);
 	pp = this_cpu_read(system_page_pool.pool);
 	data = page_pool_dev_alloc_va(pp, &truesize);
-	if (unlikely(!data)) {
-		local_unlock_nested_bh(&system_page_pool.bh_lock);
-		return NULL;
-	}
+	if (unlikely(!data))
+		goto out;
 
 	skb = napi_build_skb(data, truesize);
 	if (unlikely(!skb)) {
 		page_pool_free_va(pp, data, true);
-		local_unlock_nested_bh(&system_page_pool.bh_lock);
-		return NULL;
+		goto out;
 	}
 
 	skb_mark_for_recycle(skb);

@@ -778,15 +775,16 @@ struct sk_buff *xdp_build_skb_from_zc(struct xdp_buff *xdp)
 
 	if (unlikely(xdp_buff_has_frags(xdp)) &&
 	    unlikely(!xdp_copy_frags_from_zc(skb, xdp, pp))) {
-		local_unlock_nested_bh(&system_page_pool.bh_lock);
 		napi_consume_skb(skb, true);
-		return NULL;
+		skb = NULL;
 	}
+
+out:
 	local_unlock_nested_bh(&system_page_pool.bh_lock);
-
-	xsk_buff_free(xdp);
-
-	skb->protocol = eth_type_trans(skb, rxq->dev);
+	if (skb) {
+		xsk_buff_free(xdp);
+		skb->protocol = eth_type_trans(skb, rxq->dev);
+	}

I had in mind moving the out: label (and the unlock) below the
skb->protocol assignment, which would save the if(skb) check; any reason
we can't call xsk_buff_free() while holding the lock?

We could do that, I wasn't entirely sure about xsk_buff_free(). It is
just larger scope but nothing else so far.

I've been staring at xsk_buff_free() and the counterparts such as
xsk_buff_alloc_batch() and I didn't really figure out what is protecting
the list. Do we rely on the fact that this is used once per-NAPI
instance within RX-NAPI and never somewhere else?

-Toke

Sebastian