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=IcyaM3wW; arc=pass; dmarc=none Received: from mail-pf1-x431.google.com (mail-pf1-x431.google.com [IPv6:2607:f8b0:4864:20::431]) by mail.toke.dk (Postfix) with ESMTPS id 4203E187526A for ; Mon, 19 Oct 2026 23:35:45 +0200 (CEST) Received: by mail-pf1-x431.google.com with SMTP id d2e1a72fcca58-868b3248b2cso1725436b3a.1 for ; Mon, 19 Oct 2026 14:35:45 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1792445744; cv=none; d=google.com; s=arc-20260327; b=GdMN2neVSupr9MTNmpqdYXFdjUVWpLgN8B69+gsXEDq6qtDYtCG/KIU/msaUK1GkX+ tPMHsCnOLnW/YCCd3e/f6kphyOxR6islDst9+J1ag2kbufRQ+oWzAYevcG6NKhxhIJvA NnNopRnRIER3C/j+2jQ7DYu1iKaoka6iOkwJbqeCtJw0c/hZp9FQSForPFDw8txUPgkd DcEh50hIKQRS3G+18utax/ZVzL1eFQ5S4BBra+/CadDNmN/2tErIqfjv+PmXRpnt5zMc 4rHmGc/vNW9MtWZQKQ5MWZ7DNxS4k5GWqX1zmSiFEgLWu4sQc5GLFNbj8CiZQpc4mDsd 1g9A== 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=rTup29isLPRYZMrCHMspM4k8Os1ZwUgbxx5EShHoFyI=; fh=RxiZyOKIsYQw6ZRLeIREN6xKSL63+Zg1SJsQq6MOluU=; b=EYYW1p9Z0FLQosGhVytCxBnuiielGmwg2vr1geQM/kLz+XoKyrwjQDXmetXY2X5LGA zkJrSI4LLWa1QAl8gtOAiY3bYOZabIAhN16YhTxjlGSFeJC4DRyUfP4SKb4FvYRhM4AC lwFHHxvmSaZYVutH4Nfp1Si73lTr1aRn83+x+JeAtIKL0tR6LMtZFqfqhDxv9v0Jzwhd ljS+MGtHqwv+dWbRPX5OflTIws9APRK8gTeiD37971In4Ira3HdcQ9nTtYvaPDBLUsKm 2jV9fK4mbxs9XIYax4/RSUwto1nQhFGB5/AGp1clDHmScxbYlZGVc3SwDS9jLL9nLbQN 6oUw==; 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=1792445744; x=1793050544; 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=rTup29isLPRYZMrCHMspM4k8Os1ZwUgbxx5EShHoFyI=; b=IcyaM3wWWacQGE96WyJlCFlSnQqrgu9IDLKrNiY4Mr8bZVYT5sTAqX3ftk5bZbrE+l ojJOB1dF9Rn0suy432s1gZzWnWK/kR0do809NTlKApYGEu9/kl2aGUindiizw/9LvK7W ztfc4uCffh+Or6gGgwPDU92HomSNeshGBP/a8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1792445744; x=1793050544; 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=rTup29isLPRYZMrCHMspM4k8Os1ZwUgbxx5EShHoFyI=; b=ZQVMnf/YAWCUzOpQhqJIPq7GJwEb7LSkJ36IGEqSbvCe233R+rCNW5ivehqESY8F3N 7vVKcDGqdkVPh4jwvbi/aQhURgreWDEN0b0ZEMjYEKJt3VTa3U7slyoxbGOHI1fGd4eE ooU7m8rw8+nTYZcpRf1gVBrq8Ow5GvjYzgiKWv92xvcay/myUD31I6KkNTKy/VIM1IR9 1FWKyLfM7eFQFqszrED3H8gbU3tC1EoScj7dJPB5bQvJcZmG6EK6+L7X4tNTluLLzCk8 oMExJjBqBNsbfZ20xxUWLffKYkXcx6Zs/jMaMG0YNM5XXVvl0UeyxBBCk8oAlpnkfrsG FRoQ== X-Forwarded-Encrypted: i=1; AKwUvBwqwGW4036iyFcZg1npfLyQAVqL0ieuL2Sh32hpg9wzJd4jq/+/OL6rB4qdBWNq+ITy/bUk@lists.bufferbloat.net X-Gm-Message-State: AFq9FYLKewe8x+z2zgHc8fQs3DRldlvaMSvAkwWi+2F82g/Tw9NLI8I/ zXAw/5lhaLaW98LaM/D29doXttOCojeTjncYzoK5531PGPjrjy5NaNHLaj4R2aFi4M3R3XVmvg9 /Ci4/e5u1eaYu7BXFwGnZiwFffEGlX98T0mka6xs9 X-Gm-Gg: AYBFou1EnWHQxVBgEZFTEnxM3nvNe0LeSOBJ9OntUJpldq4ImZBJLPdfnvQR/cQqlnq LKbIlPiA5D+cIUfp4T96nRgtnw9US8V2hdGXLKvd4gxbYaJs5Bd+I/l2HSgLOx1VZzi8Ydui5yu gcY9SxF2YKbZVHYmFIuVhvgmRTmMI1IvJAfi1+AYqaYV5IBy9345A3Gn1f+XgPHNayTK09e2pH1 /XPmKlp9Mhbgu4CWS/wuyP2FZbP6yRpYIiN5RPXDx8sP6r1yTBq55X/lmlfwyLtoxtulsp261hv W7hDkGNOd53UGDnSFCOb6OpD45ESci9aTGl9Agavc3AKm4lhjUybyh9uXRUqUkcmFt6fipALeVG 8kAUUNweLrzTd+Gkwk17cNQGpcbusqFYCOw== X-Received: by 2002:a05:6a00:2d93:b0:891:3e34:e06f with SMTP id d2e1a72fcca58-8a04c248605mr6757189b3a.44.1792445744177; Mon, 19 Oct 2026 14:35:44 -0700 (PDT) MIME-Version: 1.0 References: <87o6cpwnh5.fsf@toke.dk> In-Reply-To: <87o6cpwnh5.fsf@toke.dk> From: Jamal Hadi Salim Date: Mon, 19 Oct 2026 17:35:32 -0400 X-Gm-Features: AclHuK8BMeTl4BZ64Q5bXTV8MQ1sfhpPW3JZAVfYpgUa7kopAIfu68bKFzERd6Y 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: 7SD7E25AB7XRAZ6TSXB246NHRDXTM2IP X-Message-ID-Hash: 7SD7E25AB7XRAZ6TSXB246NHRDXTM2IP 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 Mon, Oct 19, 2026 at 2:32=E2=80=AFPM Toke H=C3=B8iland-J=C3=B8rgensen wrote: > > Jamal Hadi Salim writes: > > > The Fix: > > with CAKE_FLAG_SPLIT_GSO, cake computes the flow/host hash keys, then > > splits a GSO skb into segments it accounts individually. Key extraction > > must run on the unsegmented skb -- skb_gso_segment() can rewrite an > > encapsulated packet's headers to the inner ones -- and the resolution > > must wait until the packet is admitted, since it commits the > > set-associative tags, host indices and way counters that a later > > rejection would leave mutated (a GSO skb whose segmentation fails was > > dropped with that state already committed). > > > > Split cake_hash() into a side-effect-free cake_hash_keys(), run on the > > unsegmented skb, and cake_hash_resolve(), run only once the packet is > > admitted. Move the split-GSO path ahead of the resolution: sum the > > segment list, test the total against the ceiling before linking any > > segment, and drop the whole list on rejection so no persistent state is > > committed. The list is deferred to to_free so the tail segment's > > sock_wfree() never runs under the qdisc lock; a fraglist GSO, where > > skb_segment_list() returns the original skb as the head, drops the > > caller's reference and queues the list once instead of self-linking it. > > > > Tag every deferred segment with QDISC_DROP_OVERLIMIT: __skb_gso_segment= () > > keeps SKB_GSO_CB at cb offset 32, aliasing tc_skb_cb.drop_reason, and > > __copy_skb_header() copies it into every segment, so tagging only the > > head left the rest with GSO scratch (a headroom-sized value) as their > > drop reason, which __tcf_kfree_skb_list() would report as an unrelated > > core reason. > > > > Read q->config->flow_mode once with READ_ONCE() in cake_enqueue() and > > pass that value to both cake_hash_keys() and cake_hash_resolve(). > > cake_config_change() writes the mode before cake_change() takes > > sch_tree_lock(), and the split introduced two separate loads with the > > segmentation work between them; a concurrent reconfiguration could > > extract keys in one mode and resolve them in another. > > > > Link: https://lore.kernel.org/netdev/179231933068.3135.8034079263307798= 6@kernel.org/ > > Reviewed-by: Victor Nogueira > > Signed-off-by: Jamal Hadi Salim > > --- > > v6 -> v7: the split-GSO reject path now sets QDISC_DROP_OVERLIMIT on > > every deferred segment, not only on the head/original skb; the GSO cb > > aliases tc_skb_cb.drop_reason and is copied into every segment, so > > untagged segments carried GSO scratch as their drop reason. The > > flow_mode is likewise snapshotted once with READ_ONCE() and passed to > > both cake_hash_keys() and cake_hash_resolve(). (Sashiko nipa v6 > > review, gpt-6-astra-2-5, gpt-6-astra-5-9) > > --- > > net/sched/sch_cake.c | 130 ++++++++++++++++++++++++++++++++----------- > > 1 file changed, 99 insertions(+), 31 deletions(-) > > > > diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c > > index 91c33295b30b..730c90af14bd 100644 > > --- a/net/sched/sch_cake.c > > +++ b/net/sched/sch_cake.c > > @@ -706,19 +706,28 @@ static u16 cake_get_flow_quantum(struct cake_tin_= data *q, > > get_random_u16()) >> 16; > > } > > > > -static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *sk= b, > > - int flow_mode, u16 flow_override, u16 host_override) > > +struct cake_hash_keys { > > + u32 srchost_hash; > > + u32 dsthost_hash; > > + u32 flow_hash; > > +}; > > + > > +static void cake_hash_keys(const struct sk_buff *skb, int flow_mode, > > + u16 flow_override, u16 host_override, > > + struct cake_hash_keys *out) > > { > > bool hash_flows =3D (!flow_override && !!(flow_mode & CAKE_FLOW_F= LOWS)); > > bool hash_hosts =3D (!host_override && !!(flow_mode & CAKE_FLOW_H= OSTS)); > > bool nat_enabled =3D !!(flow_mode & CAKE_FLOW_NAT_FLAG); > > - u32 flow_hash =3D 0, srchost_hash =3D 0, dsthost_hash =3D 0; > > - u16 reduced_hash, srchost_idx, dsthost_idx; > > struct flow_keys keys, host_keys; > > bool use_skbhash =3D skb->l4_hash; > > > > + out->flow_hash =3D 0; > > + out->srchost_hash =3D 0; > > + out->dsthost_hash =3D 0; > > + > > if (unlikely(flow_mode =3D=3D CAKE_FLOW_NONE)) > > - return 0; > > + return; > > > > /* If both overrides are set, or we can use the SKB hash and nat = mode is > > * disabled, we can skip packet dissection entirely. If nat mode = is > > @@ -753,50 +762,64 @@ static u32 cake_hash(struct cake_tin_data *q, con= st struct sk_buff *skb, > > switch (host_keys.control.addr_type) { > > case FLOW_DISSECTOR_KEY_IPV4_ADDRS: > > host_keys.addrs.v4addrs.src =3D 0; > > - dsthost_hash =3D flow_hash_from_keys(&host_keys); > > + out->dsthost_hash =3D flow_hash_from_keys(&host_keys); > > host_keys.addrs.v4addrs.src =3D keys.addrs.v4addrs.src; > > host_keys.addrs.v4addrs.dst =3D 0; > > - srchost_hash =3D flow_hash_from_keys(&host_keys); > > + out->srchost_hash =3D flow_hash_from_keys(&host_keys); > > break; > > > > case FLOW_DISSECTOR_KEY_IPV6_ADDRS: > > memset(&host_keys.addrs.v6addrs.src, 0, > > sizeof(host_keys.addrs.v6addrs.src)); > > - dsthost_hash =3D flow_hash_from_keys(&host_keys); > > + out->dsthost_hash =3D flow_hash_from_keys(&host_keys); > > host_keys.addrs.v6addrs.src =3D keys.addrs.v6addrs.src; > > memset(&host_keys.addrs.v6addrs.dst, 0, > > sizeof(host_keys.addrs.v6addrs.dst)); > > - srchost_hash =3D flow_hash_from_keys(&host_keys); > > + out->srchost_hash =3D flow_hash_from_keys(&host_keys); > > break; > > > > default: > > - dsthost_hash =3D 0; > > - srchost_hash =3D 0; > > + out->dsthost_hash =3D 0; > > + out->srchost_hash =3D 0; > > } > > > > /* This *must* be after the above switch, since as a > > * side-effect it sorts the src and dst addresses. > > */ > > if (hash_flows && !use_skbhash) > > - flow_hash =3D flow_hash_from_keys(&keys); > > + out->flow_hash =3D flow_hash_from_keys(&keys); > > > > skip_hash: > > if (flow_override) > > - flow_hash =3D flow_override - 1; > > + out->flow_hash =3D flow_override - 1; > > else if (use_skbhash && (flow_mode & CAKE_FLOW_FLOWS)) > > - flow_hash =3D skb->hash; > > + out->flow_hash =3D skb->hash; > > if (host_override) { > > - dsthost_hash =3D host_override - 1; > > - srchost_hash =3D host_override - 1; > > + out->dsthost_hash =3D host_override - 1; > > + out->srchost_hash =3D host_override - 1; > > } > > > > if (!(flow_mode & CAKE_FLOW_FLOWS)) { > > if (flow_mode & CAKE_FLOW_SRC_IP) > > - flow_hash ^=3D srchost_hash; > > + out->flow_hash ^=3D out->srchost_hash; > > > > if (flow_mode & CAKE_FLOW_DST_IP) > > - flow_hash ^=3D dsthost_hash; > > + out->flow_hash ^=3D out->dsthost_hash; > > } > > +} > > + > > +/* Commit the flow into the set-associative table (q->tags[], host > > + * indices, bulk counters). Only after the packet is admitted. > > + */ > > +static u32 cake_hash_resolve(struct cake_tin_data *q, > > + const struct cake_hash_keys *keys, int flow_= mode) > > +{ > > + u32 flow_hash =3D keys->flow_hash, srchost_hash =3D keys->srchost= _hash, > > + dsthost_hash =3D keys->dsthost_hash; > > + u16 reduced_hash, srchost_idx, dsthost_idx; > > + > > + if (unlikely(flow_mode =3D=3D CAKE_FLOW_NONE)) > > + return 0; > > > > reduced_hash =3D flow_hash % CAKE_QUEUES; > > > > @@ -1761,13 +1784,17 @@ static s32 cake_enqueue(struct sk_buff *skb, st= ruct Qdisc *sch, > > { > > u32 idx, tin, prev_qlen, prev_backlog, drop_id; > > struct cake_sched_data *q =3D qdisc_priv(sch); > > + int flow_mode =3D READ_ONCE(q->config->flow_mode); > > This breaks the reverse x-mas tree ordering. Move the u32 defs below, > and keep an empty line after the qdisc_priv() resolution? > > > int len =3D qdisc_pkt_len(skb), ret; > > u16 flow_override, host_override; > > + struct sk_buff *segs =3D NULL; > > + struct cake_hash_keys keys; > > struct sk_buff *ack =3D NULL; > > ktime_t now =3D ktime_get(); > > struct cake_tin_data *b; > > struct cake_flow *flow; > > bool same_flow =3D false; > > + u64 slen =3D 0; > > > > /* Classify first: a terminal TC action consumes the packet. */ > > if (cake_tcf_classify(sch, skb, &flow_override, &host_override, &= ret)) { > > @@ -1784,12 +1811,59 @@ static s32 cake_enqueue(struct sk_buff *skb, st= ruct Qdisc *sch, > > QDISC_DROP_OVERLIMIT); > > } > > > > - /* choose flow to insert into */ > > + /* Select the tin and extract the keys before segmenting: > > + * skb_gso_segment() rewrites encapsulated headers, which > > + * changes the keys. > > + */ > > b =3D cake_select_tin(sch, skb); > > - idx =3D cake_hash(b, skb, q->config->flow_mode, flow_override, > > - host_override) + 1; > > + 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); > > Reverse x-mas tree, please. Grrr... > > + > > + 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); > > + > > + /* 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_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_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. > > 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 > > + * 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); > > 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); > > + } > > + } > > + > > + /* 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. cheers, jamal > > + idx =3D cake_hash_resolve(b, &keys, flow_mode); > > tin =3D (u32)(b - q->tins); > > - idx--; > > flow =3D &b->flows[idx]; > > > > /* ensure shaper state isn't stale */ > > @@ -1816,14 +1890,9 @@ static s32 cake_enqueue(struct sk_buff *skb, str= uct Qdisc *sch, > > if (unlikely(len > b->max_skblen)) > > WRITE_ONCE(b->max_skblen, len); > > > > - if (qdisc_pkt_segs(skb) > 1 && q->config->rate_flags & CAKE_FLAG_= SPLIT_GSO) { > > - struct sk_buff *segs, *nskb, *seg; > > - netdev_features_t features =3D netif_skb_features(skb); > > - unsigned int slen =3D 0, numsegs =3D 0; > > - > > - 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); > > + if (segs) { > > + struct sk_buff *nskb, *seg; > > + unsigned int numsegs =3D 0; > > > > skb_list_walk_safe(segs, seg, nskb) { > > skb_mark_not_on_list(seg); > > @@ -1837,7 +1906,6 @@ static s32 cake_enqueue(struct sk_buff *skb, stru= ct Qdisc *sch, > > qdisc_qlen_inc(sch); > > numsegs++; > > q->buffer_used +=3D seg->truesize; > > - slen +=3D seg->len; > > WRITE_ONCE(b->packets, b->packets + 1); > > } > > > > -- > > 2.43.0