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 Cc: netdev@vger.kernel.org, Jiri Pirko , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , Victor Nogueira , =?utf-8?B?Qmo=?= =?utf-8?B?w7ZybiBUw7ZwZWw=?= , cake@lists.bufferbloat.net, Sashiko In-Reply-To: References: <87o6cpwnh5.fsf@toke.dk> Date: Tue, 20 Oct 2026 10:47:54 +0200 X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <878q3svjut.fsf@toke.dk> MIME-Version: 1.0 Content-Type: text/plain X-MailFrom: toke@toke.dk X-Mailman-Rule-Hits: implicit-dest X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header Message-ID-Hash: F2D26OUAB6RO7FYZPOTNC42DWSDSTJMH X-Message-ID-Hash: F2D26OUAB6RO7FYZPOTNC42DWSDSTJMH X-Mailman-Approved-At: Tue, 20 Oct 2026 10:58:56 +0200 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: >> > + >> > + segs = 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 += seg->len; >> > + >> > + if (unlikely((u64)sch->qstats.backlog + slen > QDISC_MAX_BACKLOG)) { >> > + qdisc_qstats_overlimit(sch); >> > + /* __skb_gso_segment() puts SKB_GSO_CB at cb >> > + * offset 32, over tc_skb_cb.drop_reason; every >> > + * 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_OVERLIMIT); >> > + /* 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. >> >> 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 == skb fraglist". How about: > /* Fraglist GSO returns the original skb as head (segs == 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 head >> > + * (segs == skb), so drop our ref first. >> > + */ >> > + if (unlikely(segs == 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); >> >> 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 == 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 == 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, struct Qdisc *sch, cake_hash_keys(skb, flow_mode, flow_override, host_override, &keys); if (qdisc_pkt_segs(skb) > 1 && q->config->rate_flags & CAKE_FLAG_SPLIT_GSO) { - struct sk_buff *nskb, *seg; netdev_features_t features = netif_skb_features(skb); + struct sk_buff *nskb, *seg; segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK); 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 = 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, struct Qdisc *sch, skb_list_walk_safe(segs, seg, nskb) tcf_set_qdisc_drop_reason(seg, QDISC_DROP_OVERLIMIT); - /* 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 == skb), so drop our ref first. - */ - if (unlikely(segs == 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 = 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 :) -Toke