Re: [PATCH net v2] net/packet: defer vmalloc TX_RING free until skbs finish

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Willem de Bruijn wrote:
> 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/
> > 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;
> > +};
> 
> > @@ -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.
> 
> > +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

To be clear, these are questions, not blocking issues.

Reviewed-by: Willem de Bruijn <willemb@google.com>