Re: [PATCH net-next] udp: add drop count for packets in udp_prod_queue

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

Paolo Abeni wrote:
> On 1/22/26 10:26 PM, Willem de Bruijn wrote:
> > Mahdi Faramarzpour wrote:
> >> This commit adds SNMP drop count increment for the packets in
> >> per NUMA queues which were introduced in commit b650bf0977d3
> >> ("udp: remove busylock and add per NUMA queues"). note that SNMP
> >> counters are incremented currently by the caller for skb. And
> >> that these skbs on the intermediate queue cannot be counted
> >> there so need similar logic in their error path.
> >>
> >> Signed-off-by: Mahdi Faramarzpour <mahdifrmx@gmail.com>
> >> ---
> >> v5:
> >>   - check if drop counts are non-zero before increasing countrers
> >> v4: https://lore.kernel.org/netdev/20260108102950.49417-1-mahdifrmx@gmail.com/
> >>   - move all changes to unlikely(to_drop) branch
> >> v3: https://lore.kernel.org/netdev/20260105114732.140719-1-mahdifrmx@gmail.com/
> >>   - remove the unreachable UDP_MIB_RCVBUFERRORS code
> >> v2: https://lore.kernel.org/netdev/20260105071218.10785-1-mahdifrmx@gmail.com/
> >>   - change ENOMEM to ENOBUFS
> >> v1: https://lore.kernel.org/netdev/20260104105732.427691-1-mahdifrmx@gmail.com/
> >> ---
> >>  net/ipv4/udp.c | 19 ++++++++++++++++++-
> >>  1 file changed, 18 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> >> index ffe074cb5..41cf8f7ab 100644
> >> --- a/net/ipv4/udp.c
> >> +++ b/net/ipv4/udp.c
> >> @@ -1793,14 +1793,31 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb)
> >>  	}
> >>  
> >>  	if (unlikely(to_drop)) {
> >> +		int err_ipv4 = 0;
> >> +		int err_ipv6 = 0;
> > 
> > nit: whitespace between variable definition and code block
> > 
> >>  		for (nb = 0; to_drop != NULL; nb++) {
> >>  			skb = to_drop;
> >> +			if (skb->protocol == htons(ETH_P_IP))
> >> +				err_ipv4++;
> >> +			else
> >> +				err_ipv6++;
> > 
> > No need for separate counters.
> > 
> > All counters will either update IPv4 or IPv6 stats. Similar to how
> > the caller of _udp_enqueue_schedule_sk is either __udp_queue_rcv_skb
> > or __udpv6_queue_rcv_skb and chooses the SNMP stat based on that.
> > 
> > Can check sk_family == PF_INET6 once.
> 
> I think that doing the SNMP accounting in __udp_enqueue_schedule_skb()
> (for `to_drop`), __udp_queue_rcv_skb() and __udpv6_queue_rcv_skb() (for
> `skb`) is a little confusing and possible error prone in the long run.
> 
> I'm wondering if something alike the following (completely untested, not
> even built! just to give the idea) would be better?

I don't see the error prone issue with the simpler patch.
But SGTM if you prefer this.
 
> Note that the additional argument in __udp_enqueue_schedule_skb() could
> be avoided piggybacking the `to_drop` return argument into the
> __udp_enqueue_schedule_skb() return value.