Re: [PATCH net-next v2 1/7] net: ethtool: add hardware pacing offload support to rings

On 7/23/26 2:29 PM, Willem de Bruijn wrote:

Mohsin Bashir wrote:

quoted

On 7/22/26 1:43 PM, Willem de Bruijn wrote:

quoted

From: Willem de Bruijn <willemb@google.com>

Add two ethtool rings operations:

- get pacing offload horizon: active and max
- set pacing offload horizon: active

Replace u64 max_pacing_offload_horizon with u32 active and max fields.
This occupies the same original 8 bytes. Reduce precision from nsec to
usec, which is sufficient.

Update the FQ scheduler to test against the active limit instead of
max. It is the administrator responsibility to set the active limit
before installing FQ.

Assisted-by: Gemini:gemini-3
Signed-off-by: Willem de Bruijn <willemb@google.com>

If I understand it correctly, the horizon lives in dev and is read at Tx
time, so it shouldn't require touching ring config. But some drivers
reallocate rings unconditionally in set_ringparam. Can we highlight
somewhere (maybe in commit message) that a horizon-only change is meant
to be non-disruptive? I may be missing the underlying reason though.

What do you mean by underlying reason?

So I meant, beyond just exposing the horizon through ethtool, if there 
is no reason to touch ring config while updating horizon, a note about 
it being non-disruptive would help.

quoted

quoted

@@ -281,6 +301,9 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
   		err_attr = tb[ETHTOOL_A_RINGS_TX];
   	else if (kernel_ringparam.hds_thresh > kernel_ringparam.hds_thresh_max)
   		err_attr = tb[ETHTOOL_A_RINGS_HDS_THRESH];
+	else if (kernel_ringparam.pacing_offload_horizon >
+		 kernel_ringparam.max_pacing_offload_horizon)
+		err_attr = tb[ETHTOOL_A_RINGS_PACING_OFFLOAD_HORIZON];
   	else
   		err_attr = NULL;
   	if (err_attr) {

@@ -302,6 +325,9 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
   
   	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
   					      &kernel_ringparam, info->extack);
+	if (!ret)
+		dev->pacing_offload_horizon = kernel_ringparam.pacing_offload_horizon;

Since a driver may be reading dev->pacing_offload_horizon on the hotpath
without a lock, should we use WRITE_ONCE() here? and have the driver use
READ_ONCE()?

Will fix in v3