Re: [PATCH net-next v8 1/6] net: rtnetlink: add pacing_offload_horizon attribute to net_device

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Jakub Kicinski wrote:
> Swapping in the conversation from v6, sorry, not sure why I missed your
> reply..
> 
> On Mon, 17 Aug 2026 22:44:30 -0400 Willem de Bruijn wrote:
> > Jakub Kicinski wrote:
> > > On Wed, 12 Aug 2026 22:03:56 -0400 Willem de Bruijn wrote:  
> > > > The 'max_pacing_offload_horizon' field of 'struct net_device' represents
> > > > the maximum pacing offload horizon supported by the device.
> > > > 
> > > > Add a new field 'pacing_offload_horizon' to store the active pacing
> > > > offload horizon.
> > > > 
> > > > The new attribute is initialized to 0 (disabled) and can be set from
> > > > userspace via RTM_SETLINK up to dev->max_pacing_offload_horizon. This
> > > > new default off behavior does not cause regressions, as no driver yet
> > > > advertises max_pacing_offload_horizon.
> > > > 
> > > > The attribute is omitted from the newlink request spec, because the
> > > > value may need to be bound by a device maximum that first needs to be
> > > > negotiated with firmware, as is the case for the idpf driver in this
> > > > series.
> > > > 
> > > > Make both fields u32, to maintain net_device cacheline layout. This
> > > > expresses up to 4s of pacing offload, which is sufficient.
> > > > 
> > > > Update the YNL specification ('rt-link.yaml') to add the
> > > > 'pacing-offload-horizon' attribute and include it in link-all-attrs.  
> > > 
> > > Forgive my slowness but I don't get how the new param squares against
> > > TCA_FQ_OFFLOAD_HORIZON. IIRC in v5 review I asked something like "should 
> > > this new option be a boolean" because the exact time horizon already
> > > exists in the qdisc uAPI. As AI points out (among other things),
> > > the two params are not synced in anyway. User can configure qdisc
> > > offload higher than the device level one.  
> > 
> > They cannot. Or at least that sure is the intent.
> > 
> > After this patch fq tests against active limit
> > dev->pacing_offload_horizon:
> > 
> > -               if (offload_horizon <= qdisc_dev(sch)->max_pacing_offload_horizon) {
> > +               if (offload_horizon <=
> > +                   READ_ONCE(qdisc_dev(sch)->pacing_offload_horizon)) {
> >                         WRITE_ONCE(q->offload_horizon, offload_horizon);
> > 
> > A manual test to replace the root qdisc with fq offload_horizon 50ms
> > seems to verify this: the command fails unless a device limit of >= 50ms
> > is configured.
> 
> The other way around. Configure the Qdisc and device to horizon of 100ms
> Then lower the device horizon to 50ms. Now the qdisc has a longer horizon
> than the device.
> 
> > Perhaps I don't understand how dev->pacing_offload_horizon
> > would function as a boolean.
> 
> The only uses of the new value are:
>  - as the qdisc bound, replacing the max_ value
>    -> Leave the qdisc as is, let qdisc config define the active horizon
>  - in the driver

I see your point now, thanks. A flag NETIF_F_PACING_OFFLOAD?

The two configurable offload_horizon fields is definitely redundant.
I do not want to ship idpf with the feature on by default, because of
SO_TXTIME. But a boolean will do.

Plus, a netdevice_notifier in FQ to clear q->offload_horizon
- when this feature flips to off or
- when dev->max_pacing_hardware_offload changes to a value smaller
  than then configured q->offload_horizon (e.g., on device reset).

> +static void idpf_tx_splitq_set_txtime(const struct sk_buff *skb,
> +				      const struct idpf_tx_queue *tx_q,
> +				      struct idpf_tx_splitq_params *tx_params)
> +{
> +	const int offload_slack_ns = 400;
> +	u64 ts, now, horizon;
> +
> +	horizon = READ_ONCE(skb->dev->pacing_offload_horizon);
> +	if (!horizon)
> +		return;
> 
>    -> which already functions as a boolean, hence my suggestion of boolean
> 
> > > In fact any non-zero value of
> > > the device one acts the same - hence the bool question.
> > >
> > >
> > > Why do we need both? How are you going to use this new knob?
> > > The commit msg explains the what not the why.
> > > 
> > > My naive understanding is that the main missing piece is a handshake
> > > between the driver and qdisc to tell the driver that the qdisc is
> > > indeed offloading pacing on queue X. And therefore the driver should
> > > pay attention to the timestamps. This does not require uAPI changes.
> > >   
> > > >  python3 tools/net/ynl/pyynl/cli.py \  
> > > 
> > > uber-nit: python3 tools/net/ynl/pyynl/cli.py -> ynl
> > > (the CLI is named ynl when packaged for end users)  
> > 
> > Should this also then point to the (default) installed spec path:
> > 
> >        ynl --spec /usr/local/share/ynl/specs/rt-link.yaml 
> 
> Use:
> 
>   ynl --family rt-link
> 
> The expectation is that the person copy/pasting from the commit message
> already has the kernel and user space updated to include your changes.
> Also it's easier to read the shorter format..

Will do. Definitely a lot cleaner.