Re: [PATCH net-next v2 1/6] net: sched: introduce qdisc-specific drop reason tracing

["Toke Høiland-Jørgensen" <toke@redhat.com>]

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

> 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>
> ---
>  include/net/dropreason-core.h  |   42 ++----------------
>  include/net/dropreason-qdisc.h |   94 ++++++++++++++++++++++++++++++++++++++++
>  include/net/dropreason.h       |    6 +++
>  include/net/sch_generic.h      |   36 ++++++++++-----
>  include/trace/events/qdisc.h   |   51 ++++++++++++++++++++++
>  net/core/dev.c                 |    8 ++-
>  net/sched/sch_cake.c           |    6 +--
>  net/sched/sch_codel.c          |    5 +-
>  net/sched/sch_dualpi2.c        |    8 +--
>  net/sched/sch_fq.c             |    7 +--
>  net/sched/sch_fq_codel.c       |    4 +-
>  net/sched/sch_fq_pie.c         |    4 +-
>  net/sched/sch_generic.c        |   22 +++++++++
>  net/sched/sch_gred.c           |    4 +-
>  net/sched/sch_pie.c            |    4 +-
>  net/sched/sch_red.c            |    4 +-
>  net/sched/sch_sfb.c            |    4 +-
>  17 files changed, 225 insertions(+), 84 deletions(-)
>  create mode 100644 include/net/dropreason-qdisc.h
>
> 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:

[...]

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

-Toke