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=WZCn3A0E; arc=pass; dmarc=none Received: from mail-pf1-x42f.google.com (mail-pf1-x42f.google.com [IPv6:2607:f8b0:4864:20::42f]) by mail.toke.dk (Postfix) with ESMTPS id 0AD4A18799A1 for ; Tue, 20 Oct 2026 12:09:10 +0200 (CEST) Received: by mail-pf1-x42f.google.com with SMTP id d2e1a72fcca58-8a070755c8cso779387b3a.0 for ; Tue, 20 Oct 2026 03:09:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1792490949; cv=none; d=google.com; s=arc-20260327; b=JFIo8DTzH3PQhjUTfk547srlIro9fLEfkbEM8n7B56pWX/yv29oB4DRiIvYq0OMnNe JUGjye4Mul8Qe9mLfx/lYPfKUJG2JtEdDVrWTfdSPDjRZ9x4eJJPGL4AnYYsEvFPhlxf B4JSSXaMD+zdk0ItSzPG+FFMEUKIVoJLi01C4ts0uWX9TyroLlt2YgYPIGYZU814R8ZL 13DAIOR2GhnGojfT2j7q7h1jkoRIrqUSeAFHQ7EL3xTK8tFnvjXNUtbjMth/2P4nVtDs h4gDQnw6XmCkA4JFSMGZS/W94XCT1qWJKoZCIm4fcIpzLh85dDzcm76X1h3GJxkV8ovY jIfQ== 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=qhnr10hYwvo+mGlvIXUR1+7EJxKovnVM/xD0YZCy14c=; fh=hsB7NAkeP1cX9DnPhGaPjXLdpZmWUJt/MieIjxsBY4Y=; b=CrUr+TYkgTtaMeVKVTcMsx3etcQFbXF3X2oM5lx+flcqWYg+hYg+xD8UAmYrmetITv 0+LloQfDHC8HSAHxHDDaQmw78dijv1/twbaQc0apRk3x1bBLOhWNS76DzD5A2BNzXKnM SjGBgyhMLbZYSLpIaDisQc+AzPtJYDR4/BQzvAZaYoyAx4Ek/G3y1TrpfkarI4LqWYmG AzhoKZZc3+VKGRP2jZkD0mhEiGMFdfE3fjnqys32/N3mC4rQDxmXfTMRadXlOm9wt3Gx XBrc4G5wrPeTHABV22YNje4VnGPATCAxONsjLuBz2rvX1C1JPgKbtReTAysUvVMfQdIq WEmg==; 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=1792490949; x=1793095749; 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=qhnr10hYwvo+mGlvIXUR1+7EJxKovnVM/xD0YZCy14c=; b=WZCn3A0EwzfeLAzycW9uAAkSglzT1X9zdEKKnPNEHKY2NYVfLOaleTDMq55P9lj0/e Ej4azVOZkbycIzJWBDUACqkMWggpu12j9OxfBi/bN269pF7qaBo6jB/ZEf7jgJPaedGO 829zJuWjUqXDrmscjT+S2Ejmzjj5HwRPvNIIk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1792490949; x=1793095749; 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=qhnr10hYwvo+mGlvIXUR1+7EJxKovnVM/xD0YZCy14c=; b=cp6zXs+rb/fvUGU/Av+8vf2w+VMcvCH8rh11yc7z/jyFwYnkNwr6e/ZWwxIVowrh30 9f/UU1iY59Sf5wGLYpe6CwFjmo47hbKAZaV5zLFbG3UCWct3NsrfCCgvH9zwNUC5bf5c K4rLCs57YTRGlINIykgo4g6G8/7+TSrBY+kzKgMeLSyuLjNfJa76zz3m9hODgTeo1wjD xk59u42RKsjH7Vk+V13TLJHdGEBmMUmmnTfBzQYNQvHaIdBvRdaTPHPru+bsBCI4tf7f sw88yhz3KNsn/TQYptI6PaTneyYGzDB3vC9ZpiVF/pPdauQlQkXjRTg9KLFjqVa/Bs5/ DpUA== X-Forwarded-Encrypted: i=1; AKwUvBzznTbbpxycsbwr/+ClVOak4VDjbo5aBOVy8PPZphIEMl9BkJGyRJig7A47mMkGlhT6N3/c@lists.bufferbloat.net X-Gm-Message-State: AFq9FYLbiWhfDRh8y9DtJn6Xb9VmKU+nsb/BmsCub+Lqn8F0WrQhd0+2 GhjNzGrX0RXUOZnqFEQxPDeqVIh/zTCE3j0NmZH7bdOC4ccv3TFjyodWJjgFFrXawKbG3YtuCuC qpj/EvsX7FoVFsULHCnicvmwir9Nvun+XfAq6Hbc2 X-Gm-Gg: AYBFou2PIPpQobJwGPD8tju6/+Bv7IJ6mBKL0BWcaPyh2bo9U8yBunV2UH9/LrEjJmN 8a2mn+T+C/YdMnFqiUJ+ikR6SsBLPZV5/LKNM1iNSgL+KWR/7kp8yFQhmHK7WBPMWE5DrqP41ui a1jdbgZkfCxn0X0H5qYuWhL8CXnrQ+gguyoWKTbD54itoWA58nhSXwfmy7+hqhK37alLpLYqBnV z2ciDxWmdufC+iRmnyzeYl09dlw7ybemKDEnB0THGlVUEtXszdgrJ/C3Fj1yTnV87+dpaa0QJ/+ YKQzY1AwETsmFclDMz4AseUQSDgUkRRpKJiiRqgM9CJ4HBXKXrz8lgSp6j6cHgUtlH48PjGAnI2 iLzAOsYAulALTlZ+RsHOqmP3UvMRG+O9IJsyf6pamDcQmqpZtjEVdQeYTv0OQmDuIDRPGxWevu+ aBvDSi8WWGp0ujvBk= X-Received: by 2002:a05:6a00:1822:b0:89e:37b1:b80d with SMTP id d2e1a72fcca58-8a04d04decamr7640049b3a.45.1792490948959; Tue, 20 Oct 2026 03:09:08 -0700 (PDT) MIME-Version: 1.0 References: <87o6cpwnh5.fsf@toke.dk> <878q3svjut.fsf@toke.dk> In-Reply-To: <878q3svjut.fsf@toke.dk> From: Jamal Hadi Salim Date: Tue, 20 Oct 2026 06:08:57 -0400 X-Gm-Features: AclHuK-Ykhr61fZX0TVqaXoTjZWGfzDVqiYE77ECXlrlYf5XxolAxmj-GohDtVI 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 , =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= , cake@lists.bufferbloat.net, Sashiko Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: DBGV4XKE5WNYUOWSBQOAH3SJPZ3NO2GD X-Message-ID-Hash: DBGV4XKE5WNYUOWSBQOAH3SJPZ3NO2GD 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 v7 4/4] net/sched: cake: hash before segmenting and reject an overlong split List-Id: Cake - FQ_codel the next generation Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue, Oct 20, 2026 at 4:47=E2=80=AFAM Toke H=C3=B8iland-J=C3=B8rgensen wrote: > > >> > + > >> > + segs =3D skb_gso_segment(skb, features & ~NETIF_F_GSO_= MASK); > >> > + if (IS_ERR_OR_NULL(segs)) > >> > + return qdisc_drop(skb, sch, to_free); > >> > + > >> > + /* The split path accounts the segment sum, not the > >> > + * stab-adjusted qdisc_pkt_len(), so check it here. > >> > + */ > >> > + skb_list_walk_safe(segs, seg, nskb) > >> > + slen +=3D seg->len; > >> > + > >> > + if (unlikely((u64)sch->qstats.backlog + slen > QDISC_M= AX_BACKLOG)) { > >> > + qdisc_qstats_overlimit(sch); > >> > + /* __skb_gso_segment() puts SKB_GSO_CB at cb > >> > + * offset 32, over tc_skb_cb.drop_reason; ever= y > >> > + * segment inherits it. Tag each one with the > >> > + * qdisc drop reason. > >> > + */ > >> > + skb_list_walk_safe(segs, seg, nskb) > >> > + tcf_set_qdisc_drop_reason(seg, > >> > + QDISC_DROP_O= VERLIMIT); > >> > + /* skb_gso_segment() moved sock_wfree to the t= ail > >> > + * segment, so defer the list to to_free rathe= r > >> > + * than free it under the qdisc lock. > >> > >> I don't think this part of the comment is needed - we do all the drops > >> using to_free, so the sock_wfree thing is irrelevant. > > > > I was trying to rationalize "the segs =3D=3D skb fraglist". How about: > > /* Fraglist GSO returns the original skb as head (segs =3D=3D skb), so > > drop our ref first */ > > I can drop it if it seems obvious > > Yeah, that bit is fine to keep :) > > >> > + * Fraglist GSO returns the original skb as he= ad > >> > + * (segs =3D=3D skb), so drop our ref first. > >> > + */ > >> > + if (unlikely(segs =3D=3D skb)) { > >> > + consume_skb(skb); > >> > + return qdisc_drop_all(segs, sch, to_fr= ee); > >> > + } > >> > + __qdisc_drop_all(segs, to_free); > >> > + return qdisc_drop_reason(skb, sch, to_free, > >> > + QDISC_DROP_OVERLIMIT)= ; > >> > >> Since you already set the drop reason above, there's not really any > >> reason to use qdisc_drop_reason() anymore; so this logic can be > >> simplified to simply: > >> > >> if (unlikely(segs =3D=3D skb)) > >> consume_skb(skb); > >> return qdisc_drop_all(segs, sch, to_free); > >> > > > > How does this look ? > > tcf_set_qdisc_drop_reason(skb, QDISC_DROP_OVERLIMIT); > > skb_list_walk_safe(segs, seg, nskb) > > tcf_set_qdisc_drop_reason(seg, QDISC_DROP_OVERLIMIT); > > > > if (unlikely(segs =3D=3D skb)) { > > consume_skb(skb); > > return qdisc_drop_all(segs, sch, to_free); > > } > > __qdisc_drop_all(segs, to_free); > > return qdisc_drop(skb, sch, to_free); > > Ah, right, got the 'segs' and 'skb' variables a bit confused, sorry > about that. > > But, thinking about it some more: > > In both branches of your code above, we have a drop of the skb, and a > drop_all of the segs. And the existing code already has an unconditional > consume_skb() after segmentation, where none of the post-segmentation > uses the 'skb' pointer. > > So wouldn't it be simpler to just add the consume_skb() unconditionally > immediately after the skb_gso_segment() call? Something like this, on > top of your patch: > > diff --git i/net/sched/sch_cake.c w/net/sched/sch_cake.c > index 730c90af14bd..20173aaf973c 100644 > --- i/net/sched/sch_cake.c > +++ w/net/sched/sch_cake.c > @@ -1819,13 +1819,17 @@ static s32 cake_enqueue(struct sk_buff *skb, stru= ct Qdisc *sch, > cake_hash_keys(skb, flow_mode, flow_override, host_override, &key= s); > > if (qdisc_pkt_segs(skb) > 1 && q->config->rate_flags & CAKE_FLAG_= SPLIT_GSO) { > - struct sk_buff *nskb, *seg; > netdev_features_t features =3D netif_skb_features(skb); > + struct sk_buff *nskb, *seg; > > segs =3D skb_gso_segment(skb, features & ~NETIF_F_GSO_MAS= K); > if (IS_ERR_OR_NULL(segs)) > return qdisc_drop(skb, sch, to_free); > > + /* we never use the original skb after splitting */ > + consume_skb(skb); > + skb =3D NULL; > + > /* The split path accounts the segment sum, not the > * stab-adjusted qdisc_pkt_len(), so check it here. > */ > @@ -1842,20 +1846,8 @@ static s32 cake_enqueue(struct sk_buff *skb, struc= t Qdisc *sch, > skb_list_walk_safe(segs, seg, nskb) > tcf_set_qdisc_drop_reason(seg, > QDISC_DROP_OVER= LIMIT); > - /* skb_gso_segment() moved sock_wfree to the tail > - * segment, so defer the list to to_free rather > - * than free it under the qdisc lock. > - * > - * Fraglist GSO returns the original skb as head > - * (segs =3D=3D skb), so drop our ref first. > - */ > - if (unlikely(segs =3D=3D skb)) { > - consume_skb(skb); > - return qdisc_drop_all(segs, sch, to_free)= ; > - } > - __qdisc_drop_all(segs, to_free); > - return qdisc_drop_reason(skb, sch, to_free, > - QDISC_DROP_OVERLIMIT); > + > + return qdisc_drop_all(segs, sch, to_free); > } > } > > @@ -1917,9 +1909,8 @@ static s32 cake_enqueue(struct sk_buff *skb, struct= Qdisc *sch, > WRITE_ONCE(b->backlogs[idx], b->backlogs[idx] + slen); > > qdisc_tree_reduce_backlog(sch, 1-numsegs, len-slen); > - consume_skb(skb); > } else { > - /* not splitting */ > + /* not splitting - skb pointer still valid */ > int ack_pkt_len =3D 0; > > cobalt_set_enqueue_time(skb, now); > > >> > + } > >> > + } > >> > + > >> > + /* Admitted: resolve the flow. The mode is read once above, so > >> > + * extraction and resolution agree if cake_change() intervenes= . > >> > + */ > >> > >> The last sentence here is just distracting. Please drop. > >> > > > > Ok.Will reword to: /* Admitted: resolve the flow. */ > > > > I wont wait for the 7 days for sashiko to respond if we can reach a > > quick consensus. > > Fine with me :) I will resend with your suggestions. pw-bot: cr cheers, jamal > -Toke