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

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:

quoted

Jakub Kicinski wrote:

quoted

On Wed, 12 Aug 2026 22:03:56 -0400 Willem de Bruijn wrote:  

quoted

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.

quoted

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

quoted

quoted

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.
  

quoted

 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.