Re:Re: [PATCH] virtio_net: add rx-alloc-fail counter

At 2026-08-13 07:34:27, "Jakub Kicinski" <[email protected]> wrote:
>On Wed, 12 Aug 2026 09:22:54 +0800 Longjun Tang wrote:
>> From: tanglongjun <[email protected]>
>> 
>> Count buffer and skb allocation failures on the rx queue and
>> report them via rx-alloc-fail in netdev qstats.
>
>You are not plumbing the new stat into virtnet_get_base_stats()
>This may be an entirely reasonable choice if you can't / don't 
>want to retain the stats for the entire device (e.g. when queues
>get disabled). But please explain the choice in the commit msg.
>
>Please explain your motivation / what made you write this patch.
>TBH the patches from KylinOS addresses are often slop so it would
>be useful to understand whether your choices here are guided by
>production experience, or not.

In my work, i once found that try_fill_recv returning ENOMEM led to
the queue having no available descriptors. To address this, i wanted
to provide some stats to inform user when it occurs. Fortunately, 
rx-alloc-fail in Documentation/netlink/specs/netdev.yaml fit the bill,

Regarding why i didn't into virtnet_get_base_stats, TBH i didn't give it
much thought. If you think it's necessary ,i can add it in the next version.


>Last but not least, AI suggests:
>
>> @@ -1917,8 +1919,10 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
>>  	}
>>  
>>  	skb = virtnet_build_skb(buf, buflen, xdp.data - buf, len);
>> -	if (unlikely(!skb))
>> +	if (unlikely(!skb)) {
>> +		u64_stats_inc(&stats->alloc_fail);
>>  		goto err;
>> +	}
>>  
>>  	if (metasize)
>>  		skb_metadata_set(skb, metasize);
>The commit message says the patch counts "buffer and skb allocation
>failures on the rx queue", and the uAPI description in
>Documentation/netlink/specs/netdev.yaml for rx-alloc-fail says "Number of
>times skb or buffer allocation failed on the Rx datapath".  Can several
>allocation failure sites on the same datapath still be missed?
>In this same function the earlier buffer allocation failure is not counted:
>	xdp_page = xdp_linearize_page(dev, rq, &num_buf, page,
>				      offset, header_offset,
>				      &tlen);
>	if (!xdp_page)
>		goto err_xdp;
>and xdp_linearize_page() returns NULL exactly on allocation failure:
>	page = page_pool_alloc_pages(rq->page_pool, GFP_ATOMIC);
>	if (!page)
>		return NULL;
>The mergeable XDP path has the same shape in receive_mergeable_xdp():
>mergeable_xdp_get_buf() returns NULL when its page_pool_alloc_pages() call
>fails, and that goes to err_xdp counting only xdp_drops/drops.
>The AF_XDP receive path is not instrumented at all.  xsk_construct_skb(),
>called from virtnet_receive_xsk_small() and virtnet_receive_xsk_merge(),
>fails like this:
>	skb = napi_alloc_skb(&rq->napi, size);
>	if (unlikely(!skb)) {
>		xsk_buff_free(xdp);
>		return NULL;
>	}
>and xsk_append_merge_buffer() drops the remaining buffers when
>napi_alloc_frag() or virtnet_skb_append_frag() fails, again without
>touching alloc_fail.
>So with an XSK pool bound to a queue, or with an XDP program that needs
>linearization, can rx-alloc-fail stay at 0 while memory pressure is
>actually causing the drops?
>-- 
>pw-bot: cr