Re: [PATCH net-next v1] net: sched: sfq: add detailed drop reasons for monitoring

[Jesper Dangaard Brouer <hawk@kernel.org>]

On 21/01/2026 01.26, Jakub Kicinski wrote:
> On Fri, 16 Jan 2026 11:40:06 +0100 Jesper Dangaard Brouer wrote:
>> On 15/01/2026 13.23, 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_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_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.
>>
>> I noticed commit 5765c7f6e317 ("net_sched: sch_fq: add three
>> drop_reason") (Author: Eric Dumazet).
>>
>>    SKB_DROP_REASON_FQ_BAND_LIMIT: Per-band packet limit exceeded
>>    SKB_DROP_REASON_FQ_HORIZON_LIMIT: Packet timestamp too far in future
>>    SKB_DROP_REASON_FQ_FLOW_LIMIT: Per-flow packet limit exceeded
>>
>> Should I/we make SKB_DROP_REASON_QDISC_MAXDEPTH specific for SFQ ?
>> Like naming it = SKB_DROP_REASON_SFQ_MAXDEPTH ?
> 
> FWIW FLOW_LIMIT is more intuitive to me, but I'm mostly dealing with
> fq so probably because that's the param name there.
> 
> I'd prefer the reuse (just MAXDEPTH, I don't see equivalent of
> MAXFLOWS?). We assign multiple names the same values in the enum to
> avoid breaking FQ users.

I've taken a detailed look at how we consume this in production and what
Prometheus metrics that we generate. My conclusion is that, we want to
keep the Eric's approach of having qdisc specific drop-reason (that
relates to qdisc tunables), but extend this with a prefix
"SKB_DROP_REASON_QDISC_xxx". This is what I implemented in [v2] like
"SKB_DROP_REASON_QDISC_SFQ_MAXFLOWS".  The QDISC_ prefix enables pattern
matching for qdisc-related drops allowing monitoring tools to match this
for categorizing new/future drop reasons into same qdisc category.

For Prometheus drop_reason metrics the decision was to omit the
net_device label to keep the metric counts manageable and avoid
exploding the number of time series (lower cardinality, less storage).
Without qdisc-specific names in the enum, we'd be forced to add back the
net_device label, which would explode our time series count.

The FQ flow_limit and SFQ depth parameters have different semantics: FQ
uses exact flow classification while SFQ uses stochastic hashing. They
also have different defaults (FQ: 100 packets, SFQ: 127 packets). Having
the same drop_reason for both would obscure which qdisc's tunable needs
adjustment when analyzing production drops.

Having more enum numbers for drop reasons is essentially free, while
keeping metrics per net_device is expensive in terms of storage and
query performance.

--Jesper

[v2] 
https://lore.kernel.org/all/176885213069.1172320.14270453855073120526.stgit@firesoul/