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

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

Mohsin Bashir wrote:
> 
> 
> On 7/22/26 1:43 PM, Willem de Bruijn wrote:
> > 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? 

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