Re: [PATCH net v2] net/packet: defer vmalloc TX_RING free until skbs finish
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2026-08-18 14:06:55
Also in:
stable
Kyle Zeng wrote:
AF_PACKET TX_RING skbs keep a raw pointer to their ring frame. The skb page references preserve page-backed ring blocks after pg_vec is freed, but they do not preserve a vmalloc mapping.
Claude shows an interesting case where this page-backed statement does not hold: if the entire skb is linear. Not for this patch, but a similar case.
tpacket_destruct_skb() currently drops the pending reference before writing the timestamp and TP_STATUS_AVAILABLE to the frame. Move the decrement after those stores. The smp_wmb() in __packet_set_status() orders the frame stores before the decrement. Also recheck pending TX frames under pg_vec_lock before non-closing ring replacement, so a racing send cannot add a pending skb between the initial check and the ring swap. Ring allocation can produce a mixture of page-backed and vmalloc-backed blocks. Allocate deferred-work storage during TX ring setup when the first vmalloc-backed block is encountered, and keep its pointer in the pg_vec allocation header. If allocation fails, return -ENOMEM from ring setup. On socket close, a non-NULL pointer identifies a vmalloc-backed vector without a scan. If TX skbs remain, defer the whole vector to system_long_wq. After pg_vec is detached, a late destructor can skip the pending decrement. Use socket write-memory accounting as the deferred lifetime gate instead: an skb remains charged through its final sock_wfree(), after all ring-frame accesses. The delayed work retains a socket reference and reschedules itself until no TX skbs remain. Move pending_refcnt release to packet_sock_destruct() so late skb destructors and deferred cleanup can safely use it after packet_release(). Page-backed teardown remains synchronous, and no lock is added to the TX completion hot path.
This one patch combines multiple fixes. If and only if a respin is needed, it may be good to break it up to help understanding.
Fixes: b013840810c2 ("packet: use percpu mmap tx frame pending refcount")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/netdev/20260721015824.45829-1-kylebot@openai.com/ (local)
Suggested-by: Eric Dumazet <edumazet@google.com>
Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Assisted-by: Codex:gpt-5.6-sol+struct packet_pg_vec {
+ struct packet_pg_vec_free *deferred;
+ unsigned int order;
+ unsigned int len;
+ struct pgv pg_vec[] __counted_by(len);
+};
+
+struct packet_pg_vec_free {
+ struct delayed_work work;
+ struct sock *sk;
+ struct packet_pg_vec *vec;
+};quoted hunk ↗ jump to hunk
@@ -4382,7 +4399,46 @@ static void free_pg_vec(struct pgv *pg_vec, unsigned int order, +static void packet_free_pg_vec_work(struct work_struct *work) +{ + struct packet_pg_vec_free *deferred; + struct packet_pg_vec *vec; + struct sock *sk; + + deferred = container_of_const(to_delayed_work(work), + struct packet_pg_vec_free, work); + vec = deferred->vec; + sk = deferred->sk; + if (sk_wmem_alloc_get(sk)) { + queue_delayed_work(system_long_wq, &deferred->work, 1);
Can this keep requeueing itself? Claude suggests using pending ring count as gate.
quoted hunk ↗ jump to hunk
+static void packet_free_tx_ring(struct sock *sk, struct pgv *pg_vec, + unsigned int order, unsigned int len) +{ + struct packet_pg_vec_free *deferred; + struct packet_pg_vec *vec; + + vec = container_of_const(pg_vec, struct packet_pg_vec, pg_vec[0]); + deferred = vec->deferred; + if (!deferred || !sk_wmem_alloc_get(sk)) { + free_pg_vec(pg_vec, order, len); + return; + } + + /* A detached ring's pending count can miss late skb destructors. */ + deferred->sk = sk; + sock_hold(sk); + queue_delayed_work(system_long_wq, &deferred->work, 0); } static char *alloc_one_pg_vec_page(unsigned long order)@@ -4410,20 +4466,35 @@ static char *alloc_one_pg_vec_page(unsigned long order) return NULL; } -static struct pgv *alloc_pg_vec(struct tpacket_req *req, int order) +static struct pgv *alloc_pg_vec(struct tpacket_req *req, int order, bool tx_ring) { unsigned int block_nr = req->tp_block_nr; + struct packet_pg_vec *vec; struct pgv *pg_vec; int i; - pg_vec = kzalloc_objs(struct pgv, block_nr, GFP_KERNEL | __GFP_NOWARN); - if (unlikely(!pg_vec)) - goto out; + vec = kzalloc_flex(*vec, pg_vec, block_nr, GFP_KERNEL | __GFP_NOWARN); + if (unlikely(!vec)) + return NULL; + vec->order = order; + vec->len = block_nr; + pg_vec = vec->pg_vec; for (i = 0; i < block_nr; i++) { pg_vec[i].buffer = alloc_one_pg_vec_page(order); if (unlikely(!pg_vec[i].buffer)) goto out_free_pgvec; + + if (tx_ring && !vec->deferred && + is_vmalloc_addr(pg_vec[i].buffer)) { + vec->deferred = kzalloc_obj(*vec->deferred, + GFP_KERNEL | __GFP_NOWARN); + if (!vec->deferred) + goto out_free_pgvec; + vec->deferred->vec = vec;
The nested structures are fairly complex. Would it make sense to avoid the separate packet_pg_vec_free, fold that into packet_pg_vec and use a different field to identify whether vmalloc backed pages are used. Maybe sk, or even a new boolean field has_vmalloc, for readability