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

[Jonathan Morton <chromatix99@gmail.com>]

> On 10 Dec, 2024, at 10:42 am, Toke Høiland-Jørgensen via Cake <cake@lists.bufferbloat.net> wrote:
> 
>>> On Mon, 09 Dec 2024 13:02:18 +0100 Toke Høiland-Jørgensen wrote:
>>>> Add three qdisc-specific drop reasons for sch_cake:
>>>> 
>>>> 1) SKB_DROP_REASON_CAKE_CONGESTED
>>>>    Whenever a packet is dropped by the CAKE AQM algorithm because
>>>>    congestion is detected.
>>>> 
>>>> 2) SKB_DROP_REASON_CAKE_FLOOD
>>>>    Whenever a packet is dropped by the flood protection part of the
>>>>    CAKE AQM algorithm (BLUE).
>>>> 
>>>> 3) SKB_DROP_REASON_CAKE_OVERLIMIT
>>>>    Whenever the total queue limit for a CAKE instance is exceeded and a
>>>>    packet is dropped to make room.
>>> 
>>> Eric's patch was adding fairly FQ-specific reasons, other than flood
>>> this seems like generic AQM stuff, no? From a very quick look the
>>> congestion looks like fairly standard AQM, overlimit is also typical
>>> for qdics?
>> 
>> 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. 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?

Would it make sense to be able to extract a "generic" code by applying a bitmask?  Leave code space for "qdisc specific" reasons within that mask.  Then people who don't care about qdisc internals can still reliably interpret the codes, even for future qdiscs.

 - Jonathan Morton