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 change extends struct tc_tbf_qopt_offload_replace_params with
limit, max_size, buffer, mtu and a peak ratecfg, and populates them
from tbf_sched_data in tbf_offload_change() so offload drivers can see
all the tbf parameters.

> 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;

The buffer and mtu fields are stored in tbf_sched_data in nanoseconds
(see tbf_change() in net/sched/sch_tbf.c where they are derived via
PSCHED_TICKS2NS(qopt->buffer) and psched_l2t_ns()), but they are
exposed here as bare s64 buffer / s64 mtu right next to max_size
which is a byte count.

Would it be worth renaming these to buffer_ns / mtu_ns, or adding
kerneldoc to describe their unit?

A driver author reading this struct and seeing mtu adjacent to
max_size might reasonably assume mtu is a byte MTU and program
hardware accordingly.

>  	struct psched_ratecfg rate;
> -	u32 max_size;
> +	struct psched_ratecfg peak;

tbf_change() does memset(&peak, 0, sizeof(peak)) when no peakrate is
configured, and the qdisc internally uses tbf_peak_present(q) which
checks q->peak.rate_bytes_ps != 0 to distinguish absent from present.

With peak exposed to offload drivers, every driver now has to
reproduce this convention implicitly (e.g. by checking
p->peak.rate_bytes_ps).

Would an explicit indicator such as a bool peak_present field, or at
least kerneldoc spelling out the rate_bytes_ps == 0 means absent
contract, make this safer for future offload consumers?

>  	struct gnet_stats_queue *qstats;
>  };
>  

[ ... ]
-- 
pw-bot: cr