Message-ID: <willemdebruijn.kernel.22b9558f6c8d6@gmail.com>
Date: Wed, 07 Jan 2026 17:37:51 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Mahdi Faramarzpour <mahdifrmx@...il.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, 
 netdev@...r.kernel.org, 
 davem@...emloft.net, 
 dsahern@...nel.org, 
 edumazet@...gle.com, 
 pabeni@...hat.com, 
 horms@...nel.org
Subject: Re: [PATCH net-next] udp: add drop count for packets in
 udp_prod_queue

Mahdi Faramarzpour wrote:
> On Wed, Jan 7, 2026 at 6:39 PM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > Mahdi Faramarzpour wrote:
> > > On Tue, Jan 6, 2026 at 10:52 PM Willem de Bruijn
> > > <willemdebruijn.kernel@...il.com> wrote:
> > > >
> > > > Mahdi Faramarzpour wrote:
> > > > > On Tue, Jan 6, 2026 at 5:24 AM Jakub Kicinski <kuba@...nel.org> wrote:
> > > > > >
> > > > > > On Mon,  5 Jan 2026 15:17:32 +0330 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").
> > > >
> > > > Can you give some rationale why the existing counters are insufficient
> > > > and why you chose to change then number of counters you suggest
> > > > between revisions of your patch?
> > > >
> > > The difference between revisions is due to me realizing that the only error the
> > > udp_rmem_schedule returns is ENOBUFS, which is mapped to UDP_MIB_MEMERRORS
> > > (refer to function __udp_queue_rcv_skb), and thus UDP_MIB_RCVBUFERRORS
> > > need not increase.
> >
> > I see. Please make such a note in the revision changelog. See also
> >
> > https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#changes-requested
> >
> Ok.
> 
> > > > This code adds some cost to the hot path. The blamed commit added
> > > > drop counters, most likely weighing the value of counters against
> > > > their cost. I don't immediately see reason to revisit that.
> > > >
> > > AFAIU the drop_counter is per socket, while the counters added in this
> > > patch correspond
> > > to /proc/net/{snmp,snmp6} pseudofiles. This patch implements the todo
> > > comment added in
> > > the blamed commit.
> >
> > Ah indeed.
> >
> > The entire logic can be inside the unlikely(to_drop) branch right?
> > No need to initialize the counters in the hot path, or do the
> > skb->protocol earlier?
> >
> Right.
> 
> > The previous busylock approach could also drop packets at this stage
> > (goto uncharge_drop), and the skb is also dropped if exceeding rcvbuf.
> > Neither of those conditions update SNMP stats. I'd like to understand
> > what makes this case different.
> >
> The difference comes from the intermediate udp_prod_queue which contains
> packets from calls to __udp_enqueue_schedule_skb that reached this branch:
> 
>     if (!llist_add(&skb->ll_node, &udp_prod_queue->ll_root))
>         return 0;
> 
> these packets might be dropped in batch later by the call that reaches the
> unlikely(to_drop) branch, and thus SNMP stats must increase. Note that such
> packets are only dropped due to the ENOBUFS returned from udp_rmem_schedule.

Understood.

The difference with the other drops is that those are on the skb that
is being passed to __udp_enqueue_schedule_skb, and are accounted to
the SNMP stats in the caller when __udp_enqueue_schedule_skb returns
with an error.

The skbs queued here cannot be accounted that way, so require
additional separate SNMP adds.

> > > > > >
> > > > > > You must not submit more than one version of a patch within a 24h
> > > > > > period.
> > > > > Hi Jakub and sorry for the noise, didn't know that. Is there any way to check
> > > > > my patch against all patchwork checks ,specially the AI-reviewer
> > > > > before submitting it?
> > > >
> > > > See https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
> > > >
> > > thanks.
> >
> >

Powered by blists - more mailing lists