From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mail.toke.dk; dkim=pass header.d=mojatatu.com header.i=@mojatatu.com header.a=rsa-sha256 header.s=google header.b=a5O755bR; arc=pass; dmarc=none Received: from mail-pf1-x42c.google.com (mail-pf1-x42c.google.com [IPv6:2607:f8b0:4864:20::42c]) by mail.toke.dk (Postfix) with ESMTPS id 56E8717915F1 for ; Thu, 08 Oct 2026 14:18:01 +0200 (CEST) Received: by mail-pf1-x42c.google.com with SMTP id d2e1a72fcca58-88b8f0a1bcdso2201990b3a.3 for ; Thu, 08 Oct 2026 05:18:01 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1791461879; cv=none; d=google.com; s=arc-20260327; b=enx0WrnZBC+Oub7jml9JHqTmPpoKRErbpRYx9Xg3/5T5Jh+nHg/g5vs3oLsLCOxcAF Hdm14NN8s4WZg+bLE5laRWQ9vjxx904l7yWknoWixeeSc0MC8cu1uZdohdTxyelVyoJv cCDU8On9aPRatZjgLHpZh+Gt3qeP9RDL+hrRR0S6z/xIj7pBTdibEDnMje0M+059XJje +EEassL5dq6xSPnPJkRvAcuYD3K8Cuw1lDKh8getvK4K+YAwQon2L28hw6cwfcxltaOO wPp0dYhsSCCKmPhgRDDHmGmiOqIAGjqEZBjwUJF3Uk20vP5gtY1wJfzIGLpbquj9qd+z eGNw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20260327; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=M2WT0JKZwEDOLnPzaszKYQRsSbuzXXJG5MpcBlmWQp8=; fh=oervVolf1hFGfUjO6v1e3wcQUL7UhtT6PF2qc088C5c=; b=Cr8u3KwqYvTkPuUnFePYej36jCYPXbIiDOSx73UbOZ92HEhVx9wbYgLxazC3QVmSs+ r9uKPAoW/0fs3vvmkDSyWX4KNIe22o1ziLMWA7jjNYvDinPU7poZC75wynEBCckaKW9g 238FrP/OHUPehe3i7ikUAbFLunQtsW22tGm9EoAHK1JfCeLkut7/C5tOP7ylog6TeIp/ hcsRlYdf62g61eEXXHz59dSgl1PaNCgq2qhbg27+aqV3jr7rBUtiKvTGdswmFnLoWpqx SQ/M+UCF9iOhW2ztiQntb9zAEme52MUwQyWFKyL4MewygyQ37C1c/lFjyMJgY2tF2g0i ZukA==; darn=lists.bufferbloat.net ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mojatatu.com; s=google; t=1791461879; x=1792066679; darn=lists.bufferbloat.net; h=content-transfer-encoding:content-type:cc:to:subject:message-id :date:from:in-reply-to:references:mime-version:from:to:cc:subject :date:message-id:reply-to:content-type; bh=M2WT0JKZwEDOLnPzaszKYQRsSbuzXXJG5MpcBlmWQp8=; b=a5O755bR/kYK3H9FTRkHw7KO8VtaAuPfsvxtUFVAT0lYDazevQPrx+EcGzjYfToyB+ hexHph3CS1g0Ee9HAlMmMZkx34hSp9dE2FbZxGFw0bG67xCVDIxQjHiTL1gDKl8Qm4p8 e4TNVxlPC96GXju7o6QZvtPK/5I/6VWyCIJNc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1791461879; x=1792066679; h=content-transfer-encoding:content-type:cc:to:subject:message-id :date:from:in-reply-to:references:mime-version:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=M2WT0JKZwEDOLnPzaszKYQRsSbuzXXJG5MpcBlmWQp8=; b=b4ENH8p2b0RfMoO5MZAFzyDfqhxFL1AesLakXEKf+gfYIxJ32O8h4PmcO03OGUPCQq OWJE9LSNhsJnaeaCJgTCuZ0YtissHEl1x3HqoTNXIKu0rd9N5uv0DVBmUlMzLoxuZkut Q4y5RvHgH5SEE3GHOzhWIZ8uhWhD4x/r42SALKWR8TPnit58aHOd+8cPkFeY1vm2YmqC yFz0mdfQFjVXIq8sjAgoPDvPw3eu1DknHoKpJW5doYp+IrlYYA5muH3dpLT9NJwbqYY0 FxiCtuJ8q8kvDNgV2rUe/O65Q6tggCYu083IBsBzzG008DmdJoyflqwwbhSZdmQYxwym lHHA== X-Forwarded-Encrypted: i=1; AKwUvByEsat+hs6G1sCj1mlEcJRT6yDr7mAt3mSiGi+/3LSnxyc4CCb/CA+i61v9UBOJmERV5GbN@lists.bufferbloat.net X-Gm-Message-State: AFuF++lQiN0eEWGjFT0HpPuKCUhpQb+XVRnd0lICHOHmCYP+tk4TmtRE RU5igM2wRt+nriipmm37fnAbDLKxkVFcwt3iVRmITru+mI6mgO3er0I3w69obX2FI7qdf0xMdFQ p9z+qDYYO9Wkzj6dMzQRIJ32/GeEo5EVoyWBE2xbS X-Gm-Gg: AYBFou1Mg6PUe82vICEDv9bmPzcHl60rOdXErYbWvSaKlMKa+NcpXQbZiXAF4urNsxg IMibOA5moBoQBAMF4xWhhi5fKqIHb/+Av2xBIbJfffxFbA4xgws8LfNbd3V+nhAMm77fY7qgMrn Lqd/i0cDK2dTVNRZbfpIk2G6NFNoIi5aeFLZOBUh7o9ssK+uJihJtNHMqHvQWx6Jvo8ShZvdS++ k7S5Ff32I7ZKp2dWIDVammKO4HFRmQZQjSYNS/OK7VoHQTcUFEI1UYhiBnkdEjEF/fGhJs6W87z DvA17ROrfEaL+CPwsTNtnXa1qC5Rkf72mAZ3wuL4limQU5xJtdRBWunLkL00lAw+6o7eaI2EI16 4j1HNPzNNUoSqVLEBSDaeI/8ZYmT58JXo+NAZj/U= X-Received: by 2002:a05:6a00:37c4:b0:892:597f:37d8 with SMTP id d2e1a72fcca58-892597f3814mr3610084b3a.15.1791461879228; Thu, 08 Oct 2026 05:17:59 -0700 (PDT) MIME-Version: 1.0 References: <87a4oottcj.fsf@toke.dk> In-Reply-To: <87a4oottcj.fsf@toke.dk> From: Jamal Hadi Salim Date: Thu, 8 Oct 2026 08:17:47 -0400 X-Gm-Features: AclHuK8XNtFRNjRvxFCSTrhybiQzTRjawx2i8ZtjcIlPD2lhimmqpObukZNvn8k Message-ID: To: =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= Cc: netdev@vger.kernel.org, Jiri Pirko , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , Victor Nogueira , moeller0@gmx.de, cake@lists.bufferbloat.net, Sashiko Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: GRNCDEVIIWLPNJP27BQ52MEFN4LEYV2S X-Message-ID-Hash: GRNCDEVIIWLPNJP27BQ52MEFN4LEYV2S X-MailFrom: jhs@mojatatu.com 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: On Thu, Oct 8, 2026 at 6:29=E2=80=AFAM Toke H=C3=B8iland-J=C3=B8rgensen wrote: > > > This is a follow-up to an issue found by Sashiko (nipa) during review o= f > > 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 ke= ys > > 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 qdi= sc > > lock. A fraglist GSO is the exception: skb_segment_list() returns the > > original skb as the list head carrying an extra reference, so segs =3D= =3D > > skb; dropping the caller's reference and queueing the list once avoids > > self-linking *to_free, which would otherwise free the same skb repeated= ly > > 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 r= un > > + * 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 =3D segs->len; > > - qdisc_skb_cb(segs)->pkt_segs =3D 1; > > - cobalt_set_enqueue_time(segs, now); > > - get_cobalt_cb(segs)->adjusted_len =3D cake_overhe= ad(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