On 06/02/2026 17.31, Toke Høiland-Jørgensen wrote:

Jesper Dangaard Brouer [off-list ref] writes:

quoted

On 06/02/2026 10.31, Toke Høiland-Jørgensen wrote:

quoted

Jesper Dangaard Brouer [off-list ref] writes:

quoted

Create new enum qdisc_drop_reason and trace_qdisc_drop tracepoint
for qdisc layer drop diagnostics with direct qdisc context visibility.

The new tracepoint includes qdisc handle, parent, kind (name), and
device information. Existing SKB_DROP_REASON_QDISC_DROP is retained
for backwards compatibility via kfree_skb_reason().

Convert FQ, FQ_CoDel, CoDel, SFB, and pfifo_fast to use the new
infrastructure.

Signed-off-by: Jesper Dangaard Brouer <hawk@kernel.org>

[...]

quoted

quoted

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index a7b7abd66e21..3d8d284e05c8 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h

@@ -68,12 +68,6 @@
   	FN(SECURITY_HOOK)		\
   	FN(QDISC_DROP)			\
   	FN(QDISC_BURST_DROP)		\
-	FN(QDISC_OVERLIMIT)		\
-	FN(QDISC_CONGESTED)		\
-	FN(CAKE_FLOOD)			\
-	FN(FQ_BAND_LIMIT)		\
-	FN(FQ_HORIZON_LIMIT)		\
-	FN(FQ_FLOW_LIMIT)		\
   	FN(CPU_BACKLOG)			\
   	FN(XDP)				\
   	FN(TC_INGRESS)			\

@@ -371,8 +365,10 @@ enum skb_drop_reason {
   	/** @SKB_DROP_REASON_SECURITY_HOOK: dropped due to security HOOK */
   	SKB_DROP_REASON_SECURITY_HOOK,
   	/**
-	 * @SKB_DROP_REASON_QDISC_DROP: dropped by qdisc when packet outputting (
-	 * failed to enqueue to current qdisc)
+	 * @SKB_DROP_REASON_QDISC_DROP: dropped by qdisc during enqueue or
+	 * dequeue. More specific drop reasons are available via the
+	 * qdisc:qdisc_drop tracepoint, which also provides qdisc handle
+	 * and name for identifying the source.

IIUC, this is not needed, see below:

[...]

quoted

@@ -37,6 +37,24 @@
   const struct Qdisc_ops *default_qdisc_ops = &pfifo_fast_ops;
   EXPORT_SYMBOL(default_qdisc_ops);
   
+void tcf_kfree_skb_list(struct sk_buff *skb, struct Qdisc *q,
+			struct netdev_queue *txq,
+			struct net_device *dev)
+{
+	while (unlikely(skb)) {
+		struct sk_buff *next = skb->next;
+		enum qdisc_drop_reason reason = tcf_get_qdisc_drop_reason(skb);
+
+		prefetch(next);
+		/* Catch wrong enum: skb_drop_reason vs qdisc_drop_reason */
+		WARN_ON_ONCE(reason && reason < __QDISC_DROP_REASON);
+		trace_qdisc_drop(q, txq, dev, skb, reason);
+		kfree_skb_reason(skb, SKB_DROP_REASON_QDISC_DROP);

AFAIU, the idea here is that you just pass the same 'reason' value to
kfree_skb_reason(). Because of the subsys shift and offset, these will
all be unique values, so anyone just watching the old tracepoint will
get QDISC_DROP_* reasons with no context, and if you want the context
you listen to the qdisc_drop tracepoint.

I'm already changing this code in V3, as syzbot revealed that some
existing skb_drop_reason's do arrive here, which is the TC filter drop
reasons (SKB_DROP_REASON_TC_*).  We primarily need subsys encoding as
we share the SKB->cb area.

This is the new code, that pass 'reason' through:

   /* TC classifier and qdisc share drop_reason storage.
    * Check subsystem mask to identify qdisc drop reasons,
    * else pass through skb_drop_reason set by TC classifier.
    */
   if ((reason & SKB_DROP_REASON_SUBSYS_MASK) == __QDISC_DROP_REASON) {
   	trace_qdisc_drop(q, txq, dev, skb, reason);
   	skb_reason = SKB_DROP_REASON_QDISC_DROP;
   } else {
   	skb_reason = (enum skb_drop_reason)reason;
   }
   kfree_skb_reason(skb, skb_reason);

I do realize that the reason is now a unique value, that in principle
could be passed through, *BUT* that will break existing userspace
tools.  E.g. the `perf trace -e skb:kfree_skb` cannot decode these to
strings. (The net/core/drop_monitor.c also need special handling.)

Ah, that's a bit unfortunate. I assumed the tools would handle these
correctly automatically :(

I've sent a [v3] with this code change:
  [v3] 
https://lore.kernel.org/all/177039500964.2258217.2989656069254156812.stgit@firesoul/ (local)

quoted

I actually want to give userspace a *single* SKB_DROP_REASON_QDISC_DROP
reason (that existing tools already knows).

Leaking extra qdisc drop reasons is not helpful to consume as it is not
actionable without knowing the qdisc. A QDISC_DROP_OVERLIMIT from FQ is
very different from one in CAKE - knowing just the reason without the
qdisc type/config is insufficient for debugging. The trace_qdisc_drop
tracepoint provides both the reason AND the qdisc context together

Depends; if you only have one of those installed on your system it'll do
just fine :)

In any case we're changing things here (removing reasons, or changing
them). I would lean towards having more information available (i.e.,
passing through the reason), but if common tools can't decode them,
that's a bit of a bummer.

I suppose it'll be too much churn to start out with the single reason,
then fix the tools to understand subsystem drop reasons, then change it
again?

IMHO it makes sense to merge this patchset, and then we can work on
adding support on tools to understand subsystem drop reasons. (After
which we can let these subsystem drop reasons fall-through.)

If I let "reason" fall-through (in above code) this "perf script" shows
this (0x30004) when listening to the current skb:kfree_skb tracepoint:

  skb:kfree_skb: skbaddr=0xffff8a9a54864ee8 rx_sk=(nil) protocol=2048 
location=tcf_kfree_skb_list+0x53 reason: 0x30004

The new qdisc tracepoint gets decodes (same event) like this:

  qdisc:qdisc_drop: drop ifindex=8 kind=sfq handle=0x10000 
parent=0xFFFFFFFF skbaddr=0xffff8a9a5f751ae8 reason=FLOW_LIMIT


--Jesper