[Cake] Re: [PATCH net-next v5] net/sched: cap the accounted backlog before it can wrap

[Jamal Hadi Salim <jhs@mojatatu.com>]

On Thu, Oct 8, 2026 at 6:29 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> > 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:
>

Ok, but there is a dilemma: The verbosity is an attempt to appease the
sashikos. The other day it complained that i didnt have a space after
a semi colon.
The more stuff i add the less it slows me back. It picks on the
accuracy of the commit text _and changelogs_ and prescribes what i
said or should have said. I can understand a future AI review would
benefit from that; there are probably a few humans that read the
commit logs after the patch has gone in.
So:
I have been more targetting the AI more than humans in these comments.


> [..]
>
> > 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.
>

In plain english, it says three things (applies to :
1) Splitting a GSO skb can account for more bytes than the original
packet, so we sum the whole segment list and test the total against
the ceiling before any segment is linked.
2) If it is over, we drop the entire list, and because no flow state
(tags, hash indices, counters) has been resolved yet, the reject
leaves nothing behind.
3) The fraglist sentence only picks which reference to drop for the
case skb_segment_list() returns the original skb, so the same skb is
not freed twice.

This text also applies to the codel piece.
Does re-writting as above sound better? I may please you but then
sashiko would ask me to change something in the wording ;->

> 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.
>

Like I said, it's a dilemma.

> [..]
> > -             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.


We have literally have about 100 followups to review from complaints
of "re-exsting issues" by sashikos and other ai officianados (there
are at least another 10 on cake) and the hard part is sifting through
and picking what is important enough to submit. And then deal with the
fallback from the sashikos turning transforming into bike-shedders.
I am trying to avoid sending many patches. But i could split this into:

- 0001 (prep, no behavior change): extract cake_tcf_classify() and the
fq_codel_classify() rewrite.
- 0002 (prep, no behavior change): skb_list_walk_safe(segs, seg) rename.
- 0003 (fix): the enqueue ceiling guard across fq_codel / codel / pie
/ fq_pie / dualpi2 / pfifo / RED / cake, plus the
qdisc_pkt_len_segs_init() producer clamp. pfifo stays here - RED reads
a grafted child's backlog, so it is the same overflow, not a separate
bug.
- 0004 (cake restructure): the cake_hash_keys() / cake_hash_resolve()
split and the split-GSO relocation.

Would that work?

cheers,
jamal
>
> pw-bot: cr