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