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 , netdev@vger.kernel.org Cc: Jamal Hadi Salim , Jiri Pirko , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , Victor Nogueira , =?utf-8?B?QmrDtnJuIFTDtnBlbA==?= , cake@lists.bufferbloat.net, Sashiko In-Reply-To: References: Date: Mon, 19 Oct 2026 20:11:40 +0200 X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <87wlrdwof7.fsf@toke.dk> MIME-Version: 1.0 Content-Type: text/plain Message-ID-Hash: D6IDK4PSBSS6X3DRIBPCXBGDTH3GOGEH X-Message-ID-Hash: D6IDK4PSBSS6X3DRIBPCXBGDTH3GOGEH X-MailFrom: toke@toke.dk 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 1/4] net/sched: cake, fq_codel: return the classifier result to the caller List-Id: Cake - FQ_codel the next generation Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Jamal Hadi Salim writes: > The Fix: > cake and fq_codel run an optional external classifier from their enqueue > path and then compute the flow hash inside the helper. Have the helper > return the filter's overrides (cake) or a 1-based flow index or a request > to hash (fq_codel) instead, so the caller owns the flow selection. > > This is preparation for the accounting changes that follow. Packet > classification and flow selection are unchanged. It's not clear to me why this change is needed at all for fq_codel? fq_codel_hash just does a reciprocal_scale() of the skb hash, it doesn't mutate any qdisc state. So why can't fq_codel_classify not just stay as it is? > Reviewed-by: Victor Nogueira > Signed-off-by: Jamal Hadi Salim > --- > net/sched/sch_cake.c | 35 +++++++++++++++++++----------- > net/sched/sch_fq_codel.c | 47 +++++++++++++++++++++++++++++----------- > 2 files changed, 56 insertions(+), 26 deletions(-) > > diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c > index dc93267029e7..6432d10c992e 100644 > --- a/net/sched/sch_cake.c > +++ b/net/sched/sch_cake.c > @@ -1712,18 +1712,24 @@ static struct cake_tin_data *cake_select_tin(struct Qdisc *sch, > return &qd->tins[tin]; > } > > -static u32 cake_classify(struct Qdisc *sch, struct cake_tin_data **t, > - struct sk_buff *skb, int flow_mode, int *qerr) > +/* Run the qdisc's optional external classifier. Returns true when a > + * terminal TC action consumed the packet, with the reason in *qerr; > + * otherwise *flow and *host hold the filter's overrides (0 if none). > + */ This comment is unnecessary, and just repeating what the code does. Please drop. > +static bool cake_tcf_classify(struct Qdisc *sch, struct sk_buff *skb, > + u16 *flow, u16 *host, int *qerr) > { > struct cake_sched_data *q = qdisc_priv(sch); > struct tcf_proto *filter; > struct tcf_result res; > - u16 flow = 0, host = 0; > int result; > > + *flow = 0; > + *host = 0; > + > filter = rcu_dereference_bh(q->filter_list); > if (!filter) > - goto hash; > + return false; > > *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; > result = tcf_classify_qdisc(skb, filter, &res, false); > @@ -1737,17 +1743,15 @@ static u32 cake_classify(struct Qdisc *sch, struct cake_tin_data **t, > *qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN; > fallthrough; > case TC_ACT_SHOT: > - return 0; > + return true; > } > #endif > if (TC_H_MIN(res.classid) <= CAKE_QUEUES) > - flow = TC_H_MIN(res.classid); > + *flow = TC_H_MIN(res.classid); > if (TC_H_MAJ(res.classid) <= (CAKE_QUEUES << 16)) > - host = TC_H_MAJ(res.classid) >> 16; > + *host = TC_H_MAJ(res.classid) >> 16; > } > -hash: > - *t = cake_select_tin(sch, skb); > - return cake_hash(*t, skb, flow_mode, flow, host) + 1; > + return false; > } > > static void cake_reconfigure(struct Qdisc *sch); > @@ -1758,20 +1762,25 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch, > u32 idx, tin, prev_qlen, prev_backlog, drop_id; > struct cake_sched_data *q = qdisc_priv(sch); > int len = qdisc_pkt_len(skb), ret; > + u16 flow_override, host_override; > struct sk_buff *ack = NULL; > ktime_t now = ktime_get(); > struct cake_tin_data *b; > struct cake_flow *flow; > bool same_flow = false; > > - /* choose flow to insert into */ > - idx = cake_classify(sch, &b, skb, q->config->flow_mode, &ret); > - if (idx == 0) { > + /* Classify first: a terminal TC action consumes the packet. */ > + if (cake_tcf_classify(sch, skb, &flow_override, &host_override, &ret)) { > if (ret & __NET_XMIT_BYPASS) > qdisc_qstats_drop(sch); > __qdisc_drop(skb, to_free); > return ret; > } > + > + /* choose flow to insert into */ > + b = cake_select_tin(sch, skb); > + idx = cake_hash(b, skb, q->config->flow_mode, flow_override, > + host_override) + 1; > tin = (u32)(b - q->tins); > idx--; > flow = &b->flows[idx]; > diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c > index e6c87a32950f..011b8fc54d84 100644 > --- a/net/sched/sch_fq_codel.c > +++ b/net/sched/sch_fq_codel.c > @@ -73,22 +73,38 @@ static unsigned int fq_codel_hash(const struct fq_codel_sched_data *q, > return reciprocal_scale(skb_get_hash(skb), q->flows_cnt); > } > > -static unsigned int fq_codel_classify(struct sk_buff *skb, struct Qdisc *sch, > - int *qerr) > +/* Run the qdisc's optional external classifier. Returns true when the > + * caller must drop the packet, with the reason in *qerr: a terminal TC > + * action consumed it, or no valid class in 1..q->flows_cnt was > + * selected (no filter matched, the result carried no class ID, or its > + * minor was above q->flows_cnt). Unlike cake, a packet with no valid > + * class is dropped here (a BYPASS drop) rather than falling back to > + * hashing. Otherwise *idx is the 1-based flow index, or *do_hash asks > + * the caller to use fq_codel_hash(). > + */ > +static bool fq_codel_classify(struct sk_buff *skb, struct Qdisc *sch, > + unsigned int *idx, bool *do_hash, int *qerr) > { > struct fq_codel_sched_data *q = qdisc_priv(sch); > struct tcf_proto *filter; > struct tcf_result res; > int result; > > + *idx = 0; > + *do_hash = false; > + > if (TC_H_MAJ(skb->priority) == sch->handle && > TC_H_MIN(skb->priority) > 0 && > - TC_H_MIN(skb->priority) <= q->flows_cnt) > - return TC_H_MIN(skb->priority); > + TC_H_MIN(skb->priority) <= q->flows_cnt) { > + *idx = TC_H_MIN(skb->priority); > + return false; > + } > > filter = rcu_dereference_bh(q->filter_list); > - if (!filter) > - return fq_codel_hash(q, skb) + 1; > + if (!filter) { > + *do_hash = true; > + return false; > + } > > *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; > result = tcf_classify_qdisc(skb, filter, &res, false); > @@ -101,13 +117,16 @@ static unsigned int fq_codel_classify(struct sk_buff *skb, struct Qdisc *sch, > *qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN; > fallthrough; > case TC_ACT_SHOT: > - return 0; > + return true; > } > #endif > - if (TC_H_MIN(res.classid) <= q->flows_cnt) > - return TC_H_MIN(res.classid); > + if (TC_H_MIN(res.classid) > 0 && > + TC_H_MIN(res.classid) <= q->flows_cnt) { > + *idx = TC_H_MIN(res.classid); > + return false; > + } > } > - return 0; > + return true; > } > > /* helper functions : might be changed when/if skb use a standard list_head */ > @@ -188,17 +207,19 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch, > struct fq_codel_sched_data *q = qdisc_priv(sch); > unsigned int idx, prev_backlog, prev_qlen; > struct fq_codel_flow *flow; > - int ret; > + bool do_hash = false; > unsigned int pkt_len; > bool memory_limited; > + int ret; > > - idx = fq_codel_classify(skb, sch, &ret); > - if (idx == 0) { > + if (fq_codel_classify(skb, sch, &idx, &do_hash, &ret)) { > if (ret & __NET_XMIT_BYPASS) > qdisc_qstats_drop(sch); > __qdisc_drop(skb, to_free); > return ret; > } > + if (do_hash) > + idx = fq_codel_hash(q, skb) + 1; > idx--; > > codel_set_enqueue_time(skb); > -- > 2.43.0