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

["Toke Høiland-Jørgensen" <toke@toke.dk>]

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:

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