Re: [PATCH net-next v2 2/3] net: sched: tbf: pass all params to offload users

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: sched: tbf: pass all params to offload users

This commit exposes additional TBF parameters (limit, buffer, mtu, and peak
rate) to hardware offload drivers so they have finer-grained control over
queue configuration.

> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 3bd08d7f39c1..d73c04e3d919 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -1040,8 +1040,12 @@ enum tc_tbf_command {
>  };
>  
>  struct tc_tbf_qopt_offload_replace_params {
> +	u32		limit;
> +	u32		max_size;
> +	s64		buffer;
> +	s64		mtu;

Are buffer and mtu the most clear variable names for the offload API? 

In sch_tbf.c, these fields store token bucket depths converted to nanoseconds,
rather than raw configuration bytes. Exposing time values under names that
typically imply byte sizes could mislead driver authors.

>  	struct psched_ratecfg rate;
> -	u32 max_size;
> +	struct psched_ratecfg peak;
>  	struct gnet_stats_queue *qstats;
>  };

[ ... ]

> diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
> index 4576111fe075..8a1110d59dec 100644
> --- a/net/sched/sch_tbf.c
> +++ b/net/sched/sch_tbf.c
> @@ -153,8 +153,12 @@ static void tbf_offload_change(struct Qdisc *sch,
>  	qopt.command = TC_TBF_REPLACE;
>  	qopt.handle = sch->handle;
>  	qopt.parent = sch->parent;
> -	qopt.replace_params.rate = q->rate;
> +	qopt.replace_params.limit = q->limit;
>  	qopt.replace_params.max_size = q->max_size;

Does passing q->max_size here provide the driver with the original burst size?

The software TBF overwrites q->max_size with min(burst, pburst) when a peak
rate is configured. As a result, the offload API only passes this overwritten
value, meaning a driver attempting to offload a dual-rate TBF is not given
the original burst size in bytes and would have to reverse-engineer it from
the nanosecond buffer value.

> +	qopt.replace_params.buffer = q->buffer;
> +	qopt.replace_params.mtu = q->mtu;

Will this cause the yt921x offload (introduced in the next patch) to
unconditionally reject all offloads?

The yt921x driver attempts to reject dual-rate offloads by checking if p->mtu
or p->peak.rate_bytes_ps are set. However, the tc userspace tool always
populates the mtu parameter with a default value (like 2047 bytes) for legacy
rate table compatibility.

Since tc always sets mtu, q->mtu will be non-zero, causing the yt921x driver
to return -EOPNOTSUPP and reject the configuration. 

Should the offload API explicitly pass the raw burst and pburst parameters in
bytes instead?

> +	qopt.replace_params.rate = q->rate;
> +	qopt.replace_params.peak = q->peak;
>  	qopt.replace_params.qstats = &sch->qstats;
>  
>  	dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TBF, &qopt);