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

[Jakub Kicinski <kuba@kernel.org>]

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

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