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

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

On 16/01/2026 12.00, Toke Høiland-Jørgensen wrote:
> Jesper Dangaard Brouer <hawk@kernel.org> writes:
> 
>> Hi Eric,
>>
>> I need an opinion on naming for drop_reasons below.
>>
>> 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 ?
>>
>> Currently SKB_DROP_REASON_QDISC_MAXDEPTH is only used in SFQ, but it
>> might be usable in other qdisc as well.  Except that I noticed the
>> meaning of SKB_DROP_REASON_FQ_FLOW_LIMIT which is basically the same.
>> This made me think that perhaps I should also make it qdisc specific.
>> I'm considering adding a per-flow limit to fq_codel as I'm seeing prod
>> issues with the global 10240 packet limit. This also need a similar flow
>> depth limit drop reason. I'm undecided which way to go, please advice.
> 
> IMO, we should be reusing drop reasons where it makes sense (so
> s/FQ/QDISC/ SKB_DROP_REASON_FQ_FLOW_LIMIT), but not sure if these are
> considered UAPI (i.e., can we change the name of the existing one)?
> 

The UAPI definition for SKB_DROP_REASON's is interesting. In this patch
(and Eric's commit) we insert in the middle of enum skb_drop_reason (on
purpose), this shows the enum numbers are not UAPI.  This is because we
want to force users to use BTF info in running kernel to resolve these IDs.
The resolved name are IMHO UAPI as e.g. our Rust code (Cc Yan) match on
these names and configures different sampling rates. So, changing name
SKB_DROP_REASON_FQ_FLOW_LIMIT have the change of breaking some userspace
tool consuming these.

Production wise, it would be easier to have SKB_DROP_REASON_SFQ_MAXDEPTH
and SKB_DROP_REASON_FQ_FLOW_LIMIT separate as the qdisc is part of the
name.  Then our Rust code don't have to also decode the net_device to
identify the qdisc involved.

--Jesper