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] [day] [month] [year] [list]

Message-ID: <87sequ5ytk.fsf@toke.dk>
Date: Wed, 11 Dec 2024 10:55:03 +0100
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Dave Taht <dave.taht@...il.com>, 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

Jakub Kicinski <kuba@...nel.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