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

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

[Jesper Dangaard Brouer]

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@kernel.org>
---
v3:
- Removed SFQ_DR() macro per upstream feedback
- Use full drop reason names (SKB_DROP_REASON_QDISC_SFQ_*) for easier grepping
- Split long lines to stay under 80 characters

v2:
- Changed to hierarchical naming: QDISC_SFQ_MAXFLOWS and QDISC_SFQ_MAXDEPTH
- Added QDISC_ prefix for pattern matching across qdisc subsystems
- Added rationale paragraph explaining userspace monitoring benefits

v1:
- Initial submission with QDISC_MAXFLOWS and QDISC_MAXDEPTH
---
 include/net/dropreason-core.h |   12 ++++++++++++
 net/sched/sch_sfq.c           |   11 +++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index a7b7abd66e21..92c99169bb97 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -70,6 +70,8 @@
 	FN(QDISC_BURST_DROP)		\
 	FN(QDISC_OVERLIMIT)		\
 	FN(QDISC_CONGESTED)		\
+	FN(QDISC_SFQ_MAXFLOWS)		\
+	FN(QDISC_SFQ_MAXDEPTH)		\
 	FN(CAKE_FLOOD)			\
 	FN(FQ_BAND_LIMIT)		\
 	FN(FQ_HORIZON_LIMIT)		\
@@ -390,6 +392,16 @@ enum skb_drop_reason {
 	 * due to congestion.
 	 */
 	SKB_DROP_REASON_QDISC_CONGESTED,
+	/**
+	 * @SKB_DROP_REASON_QDISC_SFQ_MAXFLOWS: dropped by SFQ qdisc when the
+	 * maximum number of flows is exceeded.
+	 */
+	SKB_DROP_REASON_QDISC_SFQ_MAXFLOWS,
+	/**
+	 * @SKB_DROP_REASON_QDISC_SFQ_MAXDEPTH: dropped by SFQ qdisc when a flow
+	 * exceeds its maximum queue depth limit.
+	 */
+	SKB_DROP_REASON_QDISC_SFQ_MAXDEPTH,
 	/**
 	 * @SKB_DROP_REASON_CAKE_FLOOD: dropped by the flood protection part of
 	 * CAKE qdisc AQM algorithm (BLUE).
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 96eb2f122973..ecc1028bdac8 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -302,7 +302,7 @@ static unsigned int sfq_drop(struct Qdisc *sch, struct sk_buff **to_free)
 		sfq_dec(q, x);
 		sch->q.qlen--;
 		qdisc_qstats_backlog_dec(sch, skb);
-		qdisc_drop(skb, sch, to_free);
+		qdisc_drop_reason(skb, sch, to_free, SKB_DROP_REASON_QDISC_OVERLIMIT);
 		return len;
 	}
 
@@ -363,7 +363,8 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
 	if (x == SFQ_EMPTY_SLOT) {
 		x = q->dep[0].next; /* get a free slot */
 		if (x >= SFQ_MAX_FLOWS)
-			return qdisc_drop(skb, sch, to_free);
+			return qdisc_drop_reason(skb, sch, to_free,
+						 SKB_DROP_REASON_QDISC_SFQ_MAXFLOWS);
 		q->ht[hash] = x;
 		slot = &q->slots[x];
 		slot->hash = hash;
@@ -420,14 +421,16 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
 	if (slot->qlen >= q->maxdepth) {
 congestion_drop:
 		if (!sfq_headdrop(q))
-			return qdisc_drop(skb, sch, to_free);
+			return qdisc_drop_reason(skb, sch, to_free,
+					 SKB_DROP_REASON_QDISC_SFQ_MAXDEPTH);
 
 		/* We know we have at least one packet in queue */
 		head = slot_dequeue_head(slot);
 		delta = qdisc_pkt_len(head) - qdisc_pkt_len(skb);
 		sch->qstats.backlog -= delta;
 		slot->backlog -= delta;
-		qdisc_drop(head, sch, to_free);
+		qdisc_drop_reason(head, sch, to_free,
+				  SKB_DROP_REASON_QDISC_SFQ_MAXDEPTH);
 
 		slot_queue_add(slot, skb);
 		qdisc_tree_reduce_backlog(sch, 0, delta);

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

[Paolo Abeni]

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@kernel.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.

Paolo

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

[Jesper Dangaard Brouer]

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@kernel.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

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

[Toke Høiland-Jørgensen]

Jesper Dangaard Brouer <hawk@kernel.org> writes:

> 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@kernel.org>

Okay, so I went looking for ways to get the information (which qdisc did
the drop come from) with a generic drop reason and... it's not exactly
trivial?

My thought was that you could get it from the location, but that ends up
being in the generic __dev_queue_xmit:

           ping-117101  [017] b.s2. 155651.231372: kfree_skb: skbaddr=0000000019495eb6 rx_sk=0000000000000000 protocol=34525 location=__dev_queue_xmit+0xcbd/0xee0 reason: QDISC_DROP

So you can't actually use the kfree_skb trace point to go backwards to
the qdisc.

Given that we have a sometimes-NULL socket parameter to the kfree_skb
tracepoint, I think it would make sense to augment it with a 'dev'
parameter as well, but that would require some restructuring of the drop
code, I guess.

Another option would be to add new stats members to sfq for each of
these drop reasons? Assuming you're only after the counts, that would
allow you to get those without adding new drop reasons to the core code?

OTOH, since we already have per-qdisc specific drop reasons for FQ and
CAKE, and adding more enums is cheap, I am also OK with adding these; so
with that in mind:

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>

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

[Jesper Dangaard Brouer]

On 30/01/2026 09.40, Toke Høiland-Jørgensen wrote:
> Jesper Dangaard Brouer <hawk@kernel.org> writes:
> 
>> 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@kernel.org>
> 
> Okay, so I went looking for ways to get the information (which qdisc did
> the drop come from) with a generic drop reason and... it's not exactly
> trivial?
> 

(Correct, it is actually not possible to get the qdisc from the call
location (see below), because we don't have the txq.)

> My thought was that you could get it from the location, but that ends up
> being in the generic __dev_queue_xmit:
> 
>             ping-117101  [017] b.s2. 155651.231372: kfree_skb: skbaddr=0000000019495eb6 rx_sk=0000000000000000 protocol=34525 location=__dev_queue_xmit+0xcbd/0xee0 reason: QDISC_DROP
> 
> So you can't actually use the kfree_skb trace point to go backwards to
> the qdisc.

Thanks you for providing this example. Showing what info is provided by
the tracepoint to existing tools.  (The location=__dev_queue_xmit is
key, but realize this is actually __dev_xmit_skb being inlined)


> Given that we have a sometimes-NULL socket parameter to the kfree_skb
> tracepoint, I think it would make sense to augment it with a 'dev'
> parameter as well, but that would require some restructuring of the drop
> code, I guess.
> 

Adding 'dev' parameter to drop tracepoint is IMHO a can-of-worms.
I.e. do you mean the ingress or egress device?
The skb->dev pointer changes meaning ingress or egress depending on
where in the network stack code we are.  And sometimes it is not even a
net_device pointer, see the double union[1] skb->dev belongs to.
Besides having the 'dev' doesn't help my use-case, I need the qdisc.

  [1] 
https://elixir.bootlin.com/linux/v6.18.6/source/include/linux/skbuff.h#L886-L900

I've been looking over the code multiple times. A possible alternative
is to have a new tracepoint for qdisc drops (meaning it no-longer calls
trace_kfree_skb / skb:kfree_skb). We could argue that qdisc drops have
it's own meaning like "consume".  Would this be acceptable?

This will allow tracepoint to have it's own enum set of
qdisc_drop_reasons.  This new tracepoint should have the same arguments
as __dev_xmit_skb, and we know the 'dev' is egress :

  static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
				 struct net_device *dev,
				 struct netdev_queue *txq)

Doing this work involves removing all the qdisc specific drop_reason,
except SKB_DROP_REASON_QDISC_DROP.  Is that acceptable?

I need some buy-in from maintainers Paolo/Jakub/Eric if you want me to
go down this road.  It is a lot of churn and restructuring for
something, that might be overkill.  Current patch solves my use-case,
but it would be cool/powerful to have qdisc pointer readable by BPF,
although I don't have a use-case in mind.


> Another option would be to add new stats members to sfq for each of
> these drop reasons? Assuming you're only after the counts, that would
> allow you to get those without adding new drop reasons to the core code?
>

I'm fine with someone adding more stats members to SFQ, but it doesn't
solve my use-case.  As mentioned before, we also sample packets and have
different sample rates based on drop_reasons.  We also use this as a
cmdline troubleshooting tool when debugging a prod issue, e.g. just
using perf record.


> OTOH, since we already have per-qdisc specific drop reasons for FQ and
> CAKE, and adding more enums is cheap, I am also OK with adding these; so
> with that in mind:
> 
> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>

Thanks a lot for your review.

--Jesper