Message-ID: <10ae780e-07a3-42bd-8f05-cf4f7721a325@kernel.org>
Date: Thu, 29 Jan 2026 20:11:07 +0100
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
 Eric Dumazet <eric.dumazet@...il.com>, "David S. Miller"
 <davem@...emloft.net>
Cc: bpf@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
 Toke Høiland-Jørgensen <toke@...e.dk>,
 horms@...nel.org, jiri@...nulli.us, edumazet@...gle.com,
 xiyou.wangcong@...il.com, jhs@...atatu.com, carges@...udflare.com,
 kernel-team@...udflare.com
Subject: Re: [PATCH net-next v3] net: sched: sfq: add detailed drop reasons
 for monitoring



On 29/01/2026 11.36, Paolo Abeni wrote:
> On 1/23/26 1:18 PM, Jesper Dangaard Brouer wrote:
>> Add specific drop reasons to SFQ qdisc to improve packet drop
>> observability and monitoring capabilities. This change replaces
>> generic qdisc_drop() calls with qdisc_drop_reason() to provide
>> granular metrics about different drop scenarios in production
>> environments.
>>
>> Two new drop reasons are introduced:
>>
>> - SKB_DROP_REASON_QDISC_SFQ_MAXFLOWS: Used when a new flow cannot
>>    be created because the maximum number of flows (flows parameter)
>>    has been reached and no free flow slots are available.
>>
>> - SKB_DROP_REASON_QDISC_SFQ_MAXDEPTH: Used when a flow's queue
>>    length exceeds the per-flow depth limit (depth parameter),
>>    triggering either tail drop or head drop depending on headdrop
>>    configuration.
>>
>> The existing SKB_DROP_REASON_QDISC_OVERLIMIT is used in sfq_drop()
>> when the overall qdisc limit is exceeded and packets are dropped
>> from the longest queue.
>>
>> The naming uses qdisc-specific drop reasons tied to SFQ tunables,
>> following the pattern established by Eric's FQ commit 5765c7f6e317
>> ("net_sched: sch_fq: add three drop_reason") which introduced
>> FQ_BAND_LIMIT, FQ_HORIZON_LIMIT, and FQ_FLOW_LIMIT.
>>
>> The new drop reasons are inserted in the middle of the enum after
>> SKB_DROP_REASON_QDISC_CONGESTED to group all qdisc-related reasons
>> together. While drop reason enum values are not UAPI, the enum
>> names implicitly become UAPI as userspace tools rely on BTF to
>> resolve names to values. This makes middle-insertion safe as long
>> as names remain stable.
>>
>> These detailed drop reasons enable production monitoring systems
>> to distinguish between different SFQ drop scenarios and generate
>> specific metrics for:
>> - Flow table exhaustion (flows exceeded)
>> - Per-flow congestion (depth limit exceeded)
>> - Global qdisc congestion (overall limit exceeded)
>>
>> This granular visibility allows operators to identify capacity
>> planning needs, detect traffic patterns, and optimize SFQ
>> configuration based on real-world drop patterns.
>>
>> Signed-off-by: Jesper Dangaard Brouer <hawk@...nel.org>
> 
> Given the lack of endorsing from any 3rd party, the possible BPF
> alternative and the previous controversial conversation, I'm also not
> going to apply this change, sorry.


I agree that we need someone else "3rd party" to review and ACK this
patch *before* it can be applied. I talked to Toke and Simon yesterday,
and they have promised to take a look.  So, I ask for some time for
them to review it.

I think this patch is simply following the same pattern as existing
code, that adds similar drop_reasons like FQ and CAKE did.  I
unfortunately think it complicated the discussion that I explained *my*
complicated/advanced use-case. I think these drop_reasons are generally
practical for others to use.  It makes is really easy to troubleshoot
qdisc code issues for debugging, where we want these drop_reason to
quickly identify the code (and not be too general).

Like Eric shows in commit 5765c7f6e317 this is also usable from perf:
  perf record -a -e skb:kfree_skb sleep 1; perf script


--Jesper

Powered by blists - more mailing lists