Re: [Cake] [PATCH net-next] net_sched: sch_cake: Add drop reasons

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

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 10 Dec 2024 09:42:55 +0100 Toke Høiland-Jørgensen wrote:
>> > While I initially agreed with making this generic, preserving the qdisc from
>> > where the drop came lets you safely inspect the cb block (timestamp, etc),
>> > format of which varies by qdisc. You also get insight as to which
>> > qdisc was dropping.
>> >
>> > Downside is we'll end up with SKB_DROP_REASON_XXX_OVERLIMIT for
>> > each of the qdiscs. Etc.  
>> 
>> Yeah, I agree that a generic "dropped by AQM" reason will be too generic
>> without knowing which qdisc dropped it.
>
> Why does type of the qdisc matter if the qdisc was overlimit?

Well, I was thinking you'd want to figure out which device it was
dropped from, but I guess you have skb->dev for that (and counters).

>> I guess any calls directly to kfree_skb_reason() from the qdisc will
>> provide the calling function, but for qdisc_drop_reason() the drop
>> will be deferred to __dev_queue_xmit(), so no way of knowing where
>> the drop came from, AFAICT?
>
> Can you tell me why I'd need to inspect the skb->cb[] in cake if packet
> is overlimit? Actually, none of the fields of the cb are initialized
> when the packet is dropped for overlimit, AFAIU.
>
> If someone is doing serious / advanced debug they mostly care about
> access to the qdisc and can trivially check if its ops match the
> expected symbol. (Speaking from experience, I've been debugging FQ
> packet loss on Nov 23rd.)
>
> If someone is just doing high level drop attribution having to list all
> possible qdiscs under "qdisc discard" is purely pain.
>
> Can we start with OVERLIMIT and CONGESTION as generic values and we can
> specialize if anyone has a clear need?

OK, I'll respin and drop CAKE from the names of those two...

-Toke