[Cake] Re: [PATCH net-next v8 1/4] net/sched: cake: return the classifier result to the caller

[Jamal Hadi Salim <jhs@mojatatu.com>]

On Tue, Oct 20, 2026 at 1:45 PM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>
> > The Fix:
> > cake runs an optional external classifier from its enqueue path and then
> > computes the flow hash inside the helper. Have the helper return the
> > filter's overrides instead, so the caller owns the flow selection.
> >
> > This is preparation for the accounting changes that follow. Packet
> > classification and flow selection are unchanged.
> >
> > Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> One nit below, in case you need to respin for other reasons; but I guess
> I can live with it as-is, so:
>

If sashiko comes up with something code-specific that is meaningful, I
will send another round and will factor this in.

In this and other series i learnt one small lesson that cost a lot of churn:
If sashiko complains about some pre-existing issue, I shouldnt jump in
and say "that is sensible, why not add it on the next update" ;->
It then becomes a recursion of sorts, sashiko starts asking for more
and more ;->
My temptation is to put as many small related issues as possible in
one series instead of sending one at a time and wrestling in the mud
with sashiko every week..

cheers,
jamal

> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
>
>
> > ---
> >
> > v7 -> v8: cake-only.  fq_codel_classify() is line-for-line the same as
> > fq_pie_classify() and fq_pie is not changed by this series, so fq_codel
> > keeps its original classify shape; the fq_codel backlog guard is placed
> > inline in sch_fq_codel.c in 0003 instead.  (Toke Høiland-Jørgensen, v7
> > review)
> > ---
> >  net/sched/sch_cake.c | 35 ++++++++++++++++++++++-------------
> >  1 file changed, 22 insertions(+), 13 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).
> > + */
>
> nit: I still think this comment is superfluous and should just be removed :/
>
> > +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];
> > --
> > 2.43.0