netdev - Re: [PATCH net] net: add net.core.qdisc_max

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CADVnQy=A-SDhGKVrWPWAsXm-tRjdZSD046o6MGfKxT5x_8ymKA@mail.gmail.com>
Date: Fri, 9 Jan 2026 10:15:30 -0500
From: Neal Cardwell <ncardwell@...gle.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, netdev@...r.kernel.org, 
	eric.dumazet@...il.com
Subject: Re: [PATCH net] net: add net.core.qdisc_max_burst

On Wed, Jan 7, 2026 at 5:42 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> In blamed commit, I added a check against the temporary queue
> built in __dev_xmit_skb(). Idea was to drop packets early,
> before any spinlock was acquired.
>
> if (unlikely(defer_count > READ_ONCE(q->limit))) {
>         kfree_skb_reason(skb, SKB_DROP_REASON_QDISC_DROP);
>         return NET_XMIT_DROP;
> }
>
> It turned out that HTB Qdisc has a zero q->limit.
> HTB limits packets on a per-class basis.
> Some of our tests became flaky.
>
> Add a new sysctl : net.core.qdisc_max_burst to control
> how many packets can be stored in the temporary lockless queue.
>
> Also add a new QDISC_BURST_DROP drop reason to better diagnose
> future issues.
>
> Thanks Neal !
>
> Fixes: 100dfa74cad9 ("net: dev_queue_xmit() llist adoption")
> Reported-and-bisected-by: Neal Cardwell <ncardwell@...gle.com>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> ---

Reviewed-by: Neal Cardwell <ncardwell@...gle.com>

Thanks, Eric! I really like this solution.

On Thu, Jan 8, 2026 at 12:25 AM Cong Wang <xiyou.wangcong@...il.com> wrote:
> Hm, if q->limit is the problem here, why not introduce a new Qdisc
> option for this?

Adding a new Qdisc option for this would be more complicated and
error-prone than having a single global limit with a robust,
well-chosen default like this.

Having this separate burst limit for each qdisc would make qdiscs even
more complex to understand and use correctly, particularly given that
some qdiscs (like htb) already have attributes with "burst" in the
names ("cburst" and "burst" for htb). Having a third htb limit with
"burst" in the name or description would make things even more
difficult for users.

The value that is needed for good performance is a function of the
number of CPUs on the system and the networking transmit workload. I'm
having trouble thinking of a good reason why we'd want to be able to
customize this on a per-qdisc basis. Making this a per-qdisc limit
would incorrectly imply that there's some reason we can imagine
wanting a different value for different qdiscs, and would incorrectly
imply that we believe qdisc developers and system administrators
deploying qdiscs should spend time thinking about how to tune this
parameter.

Eric wrote:
> An alternative would be to set q->limit to 1000 in HTB only.

There are at least 11 different qdiscs that currently have no way to
set the q->limit, so the q->limit ends up staying 0, and suffering
this problem. So with that alternative approach we'd have to change
all 11 of those qdiscs, and make sure that in the future new qdiscs
that enqueue skbs in children instead of the root qdisc remembered to
set q->limit to some big number as well.

So Eric's patch seems like the best approach by far among those
discussed in this thread.

Thanks!
neal