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=l6jgX+lL; arc=pass; dmarc=none Received: from mail-pz2-x2a.google.com (mail-pz2-x2a.google.com [IPv6:2607:f8b0:4864:3b::2a]) by mail.toke.dk (Postfix) with ESMTPS id A284E17199E6 for ; Tue, 29 Sep 2026 09:59:28 +0200 (CEST) Received: by mail-pz2-x2a.google.com with SMTP id d2e1a72fcca58-882c2bcef77so1398062b3a.3 for ; Tue, 29 Sep 2026 00:59:28 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1790668766; cv=none; d=google.com; s=arc-20260327; b=Sf0BnXmrVcFKCa1f3bjY+NyDGrqzeXXmvujOKY5qwXygIHJOBzMd2uNM+nF2ZPnArh VqJxmvn8nyeMVdBf/zpVCpnMw2rFe7nvw00mx5xXKlFLbaOlRhK+vKEz+vdbeKLZY2Df PJ2WZBoZJtaxts5bIEt69YzF20hPhbKGbcDDKhSkTCgUGDfg3gzknYEhJ8oCjLEnrsKU +7zSUorQ5iak0e9D+T6sKSylZtdBi7HPBWomsSY8FIGwz/5JQIgjA2E/EdQQ3BhBEVYW dUjF8L3loiidEwULB2MbJWnAkc47P0t7bKajylXp38tlKFEOAYAvpB2+rcj13w8m9NzI ZO6w== 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=KPz3exU3bEoQdiHlrFqJmk2ezPovXeSnJsGmP91aYCs=; fh=owuCw7zvB1A4FUvUrt/XxCLh2DOZljasadykvE6ASn4=; b=p+JmwhG6+xOY6Zas5m0+8gkdsGcBjR+C8Ay9vdpPqiHNLqJNOPRyWOSmkFRVaSiJsD eBQdbO2fk5lG4pg8FNNaO9eQ4D4c8kaVyFmf0tspCIyK0JTQ2O32IKYtatIr1o8nonhu nJF71Rh33paYFhhJqqC4gaQBSSEMQcVw26uw3mjbUsCK/IL2qDVV0TKGLE+AeOD/mTA8 wKjZaKk/7uFdbx5r9zuqPsBiEsLhFKvcKuMKgmIYgo1DsyOuxNtGs4towRMKGsGTz2p4 emiFv8A7s1+92Lm2K/6WPo1vDUvgyNUjB2s+oWGfb3FyjWXK/E3KYMgKn4BrhN6zkI1o BRJw==; 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=1790668766; x=1791273566; 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=KPz3exU3bEoQdiHlrFqJmk2ezPovXeSnJsGmP91aYCs=; b=l6jgX+lLPuf7w5oWb+koIroEAivhAkD37NoS/V3ujrQ6uC0F8W4u6ZJDhIh6YNFYfW vymgZsuQfQpqmUTDXtTbJFJEjCAZEDB2kccBdgDsx3Sdzy4kv1fb7I+lbe1f1ItA9uJN ADQIkHmfPGdOntUxsMcqYilGR+dXddmtnyQLA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1790668766; x=1791273566; 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=KPz3exU3bEoQdiHlrFqJmk2ezPovXeSnJsGmP91aYCs=; b=am8811D69v+6ioYFx/MPFnmyuj99wMVEk7sfX6TSrcTsnjfX0CuDm15WxoGROelBrl /+6OCNHostpWqT3WpKXm2yyAoTEjLn4X1PUcUvj6XaW9DQax5VP6g146CoBvtw1f1CUa /mAtcpcnBUsUwOLwGd4Y2s4zSgRaedpYOLXTrqnuYmsbwVBJPI4svtaG4YpeGHLCdfJr fOIpTtoG3iO3Rs99bpDgKUdpaNiFQTeK4eCLLk3hNwAhuTyxggm0KMaKlU7yl6kkO6/G DK9f5ToTIaGbKja7CiWoXZMBPI6OkEWTBhq4ZRWNXgfJNSzc1jRP5gwm6+8J7mLaFGJn /HLQ== X-Forwarded-Encrypted: i=1; AKwUvByN8LURyHtGPY7CBJrtt3+2f/as2yWoHcgy0PaNmqYT4ZPHNUIGWzVy9gZHCFS4bJal4FdJ@lists.bufferbloat.net X-Gm-Message-State: AFuF++nSD+hP3nBjjMSQaJtxTPl+NjLzPsZqvieYDPLpN3gq5RBdmzDV a/mEyoyrUH5FgDCN3sDnM8agyxSjJrpqxWtHy9JZJDkhL9vb2/j2KpbPD+mBFe4ASFOiOUL/TSI XTg38CWjPjhL4vbkvYuq4yjQR2dRtk54r52SHYQ0MJwj23fq3SVDcmg== X-Gm-Gg: AYBFou0iMAUmIwv5JMr5lNPsV0NpGAGUNBHrNXDUsfNxMyL+o3rIn+FKYizdlVIXulT sFdGI3OE1w3rZ9n2d3sYTPPMPbnHilTeZ6mdOn9N45yMopc61xN6ecWNv5pbF6chwVeFgNH/+VB XWWAAAl+NNFVZ4Z59AsrqCvJ1tjm1FzDLp9aEjUTMBWw28eYKUFleQZL0nPQ56pTji5gWJPgaIb d5jJ8m/j6ll9IxSQVuPr1f58bowGt8Gouy/ey0AlBXv92/AS78A85JYalb2Hz4brwkZY63AgXaz 626Ruf8erTALtcjsVZqgYF4gSuX7GnKKFa6rxZ32kTN78rP4KzKdRSqxJPMsWF2F1rKO7+PQ8pt 7t8oqfa/RcX1LqiagHp81WYWN8d5DyyIHYQ== X-Received: by 2002:a05:6a21:a394:b0:3dd:a007:cb2d with SMTP id adf61e73a8af0-3de26db6f57mr10473666637.41.1790668765702; Tue, 29 Sep 2026 00:59:25 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Jamal Hadi Salim Date: Tue, 29 Sep 2026 03:59:14 -0400 X-Gm-Features: AclHuK-XjHMnvK2h7VZJ_VfwefFe2EjB9zY891AR4NBwsMK0bxiTSEY--zE7rLg Message-ID: To: Eric Dumazet Cc: netdev@vger.kernel.org, Jiri Pirko , "David S . Miller" , Jakub Kicinski , Paolo Abeni , Simon Horman , Victor Nogueira , hybris , =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= , moeller0@gmx.de, cake@lists.bufferbloat.net, Sashiko Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: Y6WSFJZAK3EKHAQIC72LBGKNTQRW5BFB X-Message-ID-Hash: Y6WSFJZAK3EKHAQIC72LBGKNTQRW5BFB 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 v2] 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 Mon, Sep 28, 2026 at 8:34=E2=80=AFAM Eric Dumazet = wrote: > > On Sat, Sep 26, 2026 at 7:49=E2=80=AFPM Jamal Hadi Salim wrote: > > > > fq_codel, cake, codel, pie, fq_pie and dualpi2 account every enqueued > > packet's stab-adjusted length into a 32-bit sch->qstats.backlog. > > fq_codel/codel uses it do decide if they should drop a packet at deq; > > cake uses it to prune the longest-flow heap from per-flow backlogs; > > pie and fq_pie use it to make early drop decisions and, dualpi2 decides > > must_drop() on it. RED can can decide on a child's backlog based on it. > > A crafted tc size table (TC_STAB) inflates qdisc_pkt_len() up to > > QDISC_PKT_LEN_MAX (1 MiB), so a few thousand packets wrap the counter > > mod 2^32. The AQM algo then reads a small backlog and makes the wrong > > drop decision, and the dequeue-side subtractions keep the counter corru= pt. > > > > Fix: > > Drop at enqueue once the accounted backlog would cross QDISC_MAX_BACKLO= G > > (U32_MAX - QDISC_PKT_LEN_MAX), the largest backlog one more maximum-siz= e > > packet cannot wrap. This follows the existing bfifo/gred approach > > (safe because its limit is checked against the accounted packet length)= ; > > the fixed qdiscs' limits are packet counts or otherwise do not bound > > the aggregate bytes, so they need the byte bound here. > > Hi Jamal, > Most of these look reasonable. pw-bot: cr cheeers, jamal > Thanks for reworking this for v2. A few comments on the implementation: > 1. QDISC_MAX_BACKLOG check > Since QDISC_MAX_BACKLOG is already defined as > (U32_MAX - QDISC_PKT_LEN_MAX), it already reserves QDISC_PKT_LEN_MAX > of headroom below U32_MAX for the incoming packet. Doing: > if (unlikely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) > > QDISC_MAX_BACKLOG)) > accounts for the incoming packet size twice and forces a 64-bit addition > at every call site. > Why not just check unlikely(sch->qstats.backlog > QDISC_MAX_BACKLOG) > (or provide a small helper in include/net/sch_generic.h)? > > > 2. sch_cake.c (cake_enqueue) > There are a few issues with how cake_enqueue() is handled: > - The first check is placed after cake_classify(), which has already > modified the packet's DSCP (cake_handle_diffserv()) and updated > set-associative hash state and host bulk-flow counters in cake_hash() > (srchost_bulk_flow_count / dsthost_bulk_flow_count) assuming the > packet will be enqueued into flow. > - In the CAKE_FLAG_SPLIT_GSO branch, the second check runs after lines > 1780-1801 have already updated b->max_skblen, shaper timestamps > (time_next_packet), qstats.overlimits, and scheduled &q->watchdog. > - Walking segs a second time on every GSO packet just to sum slen is > unnecessary, and calling both kfree_skb_list_reason(segs, ...) (under > the qdisc lock instead of via to_free) and qdisc_drop_reason(skb, ...) > triggers duplicate drop tracepoints for both the segments and the > parent GSO skb. > Since QDISC_MAX_BACKLOG already leaves QDISC_PKT_LEN_MAX (1 MiB) of > headroom, a single check at the very beginning of cake_enqueue() before > cake_classify() is sufficient and avoids touching the GSO split path > altogether. > > 3. sch_fq_codel.c (fq_codel_enqueue) > Can we move the backlog check before fq_codel_classify() (or in the > !q->filter_list fast path before fq_codel_hash()) so we do not compute > the flow hash for packets we are about to drop? > Also, please mention in the commit message that wrapping q->backlogs[i] > to 0 in fq_codel causes fq_codel_drop() to leave idx =3D 0 and dereferenc= e > a NULL flow->head if flow 0 is empty. > > 4. sch_red.c and sch_fifo.c (pfifo) > In red_enqueue(), red_calc_qavg() reads child->qstats.backlog. In your > commit message example, child->qstats.backlog wrapped because the child > was a packet-limited pfifo (limit 100000). > Shouldn't pfifo_enqueue() and pfifo_tail_enqueue() in > net/sched/sch_fifo.c also guard against qstats.backlog wrapping when > used standalone or under other classful qdiscs? > > Thanks!