netdev - Re: [Cake] [PATCH net-next] net_sched: sch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <C2055FB9-8EA4-486D-9654-84ED422A4A0C@gmail.com>
Date: Tue, 10 Dec 2024 11:10:57 +0200
From: Jonathan Morton <chromatix99@...il.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: Dave Taht <dave.taht@...il.com>,
 Jakub Kicinski <kuba@...nel.org>,
 Jiri Pirko <jiri@...nulli.us>,
 netdev@...r.kernel.org,
 Jamal Hadi Salim <jhs@...atatu.com>,
 cake@...ts.bufferbloat.net,
 Eric Dumazet <edumazet@...gle.com>,
 Simon Horman <horms@...nel.org>,
 Cong Wang <xiyou.wangcong@...il.com>,
 Paolo Abeni <pabeni@...hat.com>,
 "David S. Miller" <davem@...emloft.net>
Subject: Re: [Cake] [PATCH net-next] net_sched: sch_cake: Add drop reasons

> On 10 Dec, 2024, at 10:42 am, Toke Høiland-Jørgensen via Cake <cake@...ts.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