From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mail.toke.dk; dkim=none; arc=none (Message is not ARC signed); dmarc=fail (Used From Domain Record) header.from=toke.dk policy.dmarc=none From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= To: Jamal Hadi Salim , netdev@vger.kernel.org Cc: Jamal Hadi Salim , Jiri Pirko , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , Victor Nogueira , moeller0@gmx.de, cake@lists.bufferbloat.net, Sashiko In-Reply-To: References: Date: Thu, 08 Oct 2026 11:49:32 +0200 X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <87a4oottcj.fsf@toke.dk> MIME-Version: 1.0 Content-Type: text/plain Message-ID-Hash: SZXMZUTUOWNKC32WN2MOCKUSTSRSHYOD X-Message-ID-Hash: SZXMZUTUOWNKC32WN2MOCKUSTSRSHYOD X-MailFrom: toke@toke.dk X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list Subject: [Cake] Re: [PATCH net-next v5] net/sched: cap the accounted backlog before it can wrap List-Id: Cake - FQ_codel the next generation Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: > This is a follow-up to an issue found by Sashiko (nipa) during review of > commit d9ebd8f9aa8b ("net/sched: fq_codel: clamp default quantum and > mtu"), part of the quantum/mtu overflow series merged as a687f2ae995f. > Clamping the per-flow quantum and the CoDel mtu does not bound the > accounted backlog, so the per-flow backlog wrap the review flagged > remained. This, and the rest of the commit message is obtuse LLM vomit, and close to unreadable. Please rewrite the commit message without the verbosity, so it's actually something a human reader can comprehend. For the cake bits in particular: [..] > CAKE's SPLIT_GSO path accounts the sum of the segment lengths, which a > stab recomputes from skb->len and can exceed the pre-split > qdisc_pkt_len(). The list is summed before any segment is linked and > rejected as a whole before any flow state is resolved: the flow/host keys > are computed from the unsegmented packet, but the set-associative > resolution that commits q->tags[], the flow host indices, the way > counters and the host bulk-flow counts runs only after the whole > segment-sum admission test passes. A packet rejected by that test > therefore leaves no persistent state behind. The whole list is dropped > via to_free so the tail segment's sock_wfree() never runs under the qdisc > lock. A fraglist GSO is the exception: skb_segment_list() returns the > original skb as the list head carrying an extra reference, so segs == > skb; dropping the caller's reference and queueing the list once avoids > self-linking *to_free, which would otherwise free the same skb repeatedly > in the deferred teardown. I read this paragraph multiple times, and it's still not clear to me what it's trying to say. Also: > net/sched/sch_cake.c | 198 ++++++++++++++++++++++++++++---------- You're changing hundreds of lines of code which is partly a refactor, partly a fix for the actual overflow issue, and partly unrelated changes. Please split them into separate patches. [..] > +/* Compute the flow/host hash keys from the packet as received. Must run > + * before skb_gso_segment(), which rewrites an encapsulated GSO packet's > + * headers to the inner ones and would change the keys. Reads only the skb > + * and commits no qdisc state, so it may run before admission is known. > + */ And drop the weird LLM comments, please. If you split the series properly they are not needed. [..] > - skb_list_walk_safe(segs, segs, nskb) { > - skb_mark_not_on_list(segs); > - qdisc_skb_cb(segs)->pkt_len = segs->len; > - qdisc_skb_cb(segs)->pkt_segs = 1; > - cobalt_set_enqueue_time(segs, now); > - get_cobalt_cb(segs)->adjusted_len = cake_overhead(q, > - segs); > - flow_queue_add(flow, segs); > + skb_list_walk_safe(segs, seg, nskb) { The skb_list_walk_safe(segs, segs) thing is admittedly a bit weird, and fixing it is fine. But that's the "separate fix" I was referring to above, so split it out; it's just making the actual fix harder to comprehend here. -Toke pw-bot: cr