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

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

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

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

[David Yang]

On Thu, May 7, 2026 at 9:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >       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.

I'm not an expert on TBF so I don't know if it's fine to expose the
internal of schedulers to the drivers, also this would require changes
on struct tbf_sched_data.

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

These are carbon copies of struct tbf_sched_data, I see no reason to
rename just here.

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

No similar logic is found for struct flow_action_police.

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

[Jakub Kicinski]

On Thu, 7 May 2026 11:11:58 +0800 David Yang wrote:
> > >  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.  
> 
> These are carbon copies of struct tbf_sched_data, I see no reason to
> rename just here.

Driver API has broader exposure and more potential for
misunderstandings. AI's naming suggestion makes sense to me.