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

[Mohsin Bashir <mohsin.bashr@gmail.com>]

On 7/23/26 2:29 PM, Willem de Bruijn wrote:
> 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?
> 

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.

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