Re: [PATCH net-next v6 1/4] net/sched: cake, fq_codel: return the classifier result to the caller
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-18 10:28:49
Thank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you respin. Low priority / nit picks for your consideration: - [Low] The new comment above fq_codel_classify() in net/sched/sch_fq_codel.c gives two reasons the function returns true… -- net/sched: cake, fq_codel: return the classifier result to the caller This patch changes the classifier helpers in cake and fq_codel so they no longer compute the flow hash. The cake helper now returns the filter's flow/host overrides. The fq_codel helper now returns either a 1-based flow index or a request to hash, so the caller picks the flow. It is preparation for later accounting changes, and classification behaviour should stay the same.
quoted hunk ↗ jump to hunk
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c index e6c87a32950f9..4cf3d379b8e95 100644 --- a/net/sched/sch_fq_codel.c +++ b/net/sched/sch_fq_codel.c@@ -73,22 +73,35 @@ 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 (a terminal TC action consumed it, or the + * result carried no class ID); the reason is then in *qerr. Otherwise *idx + * is the 1-based flow index, or *do_hash asks the caller to use + * fq_codel_hash(). + */
[Severity: Low]
This isn't a bug, but the comment above fq_codel_classify() lists only two
cases where it returns true. The code returns true in two more cases.
First, if no filter in the chain matches, tcf_classify_qdisc() returns a
negative value. The if (result >= 0) block is skipped and the function hits
the final return true.
Second, a filter can return a class ID whose minor is above q->flows_cnt.
One example is classid 1:2000 with the default 1024 flows. That fails this
check and also falls through to return true:
if (TC_H_MIN(res.classid) > 0 &&
TC_H_MIN(res.classid) <= q->flows_cnt) {
Neither case is a terminal action, and the second one does carry a class
ID. The behaviour matches the old helper, which returned 0 in both cases.
Only the new comment is incomplete.
The commit message says follow-up accounting patches build on this helper.
Could the comment say that true is returned whenever no valid class in
1..flows_cnt was selected (no match, minor 0, or minor above flows_cnt)?
Otherwise a reader may assume these packets fall back to hashing, as they
do in cake_tcf_classify(), when fq_codel drops them as BYPASS drops.
quoted hunk ↗ jump to hunk
+static bool fq_codel_classify(struct sk_buff *skb, struct Qdisc *sch, + unsigned int *idx, bool *do_hash, int *qerr) {
[ ... ]
quoted hunk ↗ jump to hunk
- 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; }
[ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-BA27.v6.r0.20261014054826%40mojatatu.com