Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Jakub Kicinski <kuba@kernel.org>]

On Tue,  5 Dec 2023 17:50:29 -0300 Victor Nogueira wrote:
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 4b84b72ebae8..f38c928a34aa 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3753,6 +3753,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
>  
>  	qdisc_calculate_pkt_len(skb, q);
>  
> +	tcf_set_drop_reason(skb, SKB_DROP_REASON_QDISC_DROP);
> +
>  	if (q->flags & TCQ_F_NOLOCK) {
>  		if (q->flags & TCQ_F_CAN_BYPASS && nolock_qdisc_is_empty(q) &&
>  		    qdisc_run_begin(q)) {
> @@ -3782,7 +3784,7 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
>  no_lock_out:
>  		if (unlikely(to_free))
>  			kfree_skb_list_reason(to_free,
> -					      SKB_DROP_REASON_QDISC_DROP);
> +					      tcf_get_drop_reason(to_free));
>  		return rc;
>  	}
>  
> @@ -3837,7 +3839,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
>  	}
>  	spin_unlock(root_lock);
>  	if (unlikely(to_free))
> -		kfree_skb_list_reason(to_free, SKB_DROP_REASON_QDISC_DROP);
> +		kfree_skb_list_reason(to_free,
> +				      tcf_get_drop_reason(to_free));

You stuff the drop reason into every skb but then only use the one from
the head? Herm. __qdisc_drop() only uses the next pointer can't we
overload the prev pointer to carry the drop reason. That means only
storing it if we already plan to drop the packet.

BTW I lack TC knowledge but struct tc_skb_cb is even more clsact
specific today than tcf_result. And reserving space for drop reason
in a state structure seems odd. Maybe that's just me.