Re: [PATCH net-next v6 4/9] net: ethtool: add support for configuring hds-thresh

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 18 Dec 2024 14:45:25 +0000 Taehee Yoo wrote:
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 4e451084d58a..4f407ce9eed1 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -78,6 +78,8 @@ enum {
>   * @cqe_size: Size of TX/RX completion queue event
>   * @tx_push_buf_len: Size of TX push buffer
>   * @tx_push_buf_max_len: Maximum allowed size of TX push buffer
> + * @hds_thresh: Threshold value of header-data-split-thresh
> + * @hds_thresh_max: Maximum allowed threshold of header-data-split-thresh

nit: s/allowed/supported/

> +u8 dev_xdp_sb_prog_count(struct net_device *dev)
> +{
> +	u8 count = 0;
> +	int i;
> +
> +	for (i = 0; i < __MAX_XDP_MODE; i++)
> +		if (dev->xdp_state[i].prog &&
> +		    !dev->xdp_state[i].prog->aux->xdp_has_frags)
> +			count++;
> +	return count;
> +}
> +EXPORT_SYMBOL_GPL(dev_xdp_sb_prog_count);

No need to export this, AFAICT, none of the callers can be built 
as a module.

> +	hds_config_mod = old_hds_config != kernel_ringparam.tcp_data_split;

Does it really matter if we modified the HDS setting for the XDP check?
Whether it was already set or the current config is asking for it to be
set having XDP SB and HDS is invalid, we can return an error.

> +	if (kernel_ringparam.tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
> +	    hds_config_mod && dev_xdp_sb_prog_count(dev)) {
> +		NL_SET_ERR_MSG(info->extack,

		NL_SET_ERR_MSG_ATTR(info->extack,
				    tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT],
				    ...

> +			       "tcp-data-split can not be enabled with single buffer XDP");
> +		return -EINVAL;
> +	}
> +
> +	if (kernel_ringparam.hds_thresh > kernel_ringparam.hds_thresh_max) {
> +		NL_SET_BAD_ATTR(info->extack,
> +				tb[ETHTOOL_A_RINGS_HDS_THRESH_MAX]);
> +		return -ERANGE;
> +	}

Can this condition not be handled by the big if "ladder" below?
I mean like this:

@@ -282,6 +276,8 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
                err_attr = tb[ETHTOOL_A_RINGS_RX_JUMBO];
        else if (ringparam.tx_pending > ringparam.tx_max_pending)
                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_MAX];
        else
                err_attr = NULL;
        if (err_attr) {

>  	/* ensure new ring parameters are within limits */
>  	if (ringparam.rx_pending > ringparam.rx_max_pending)
>  		err_attr = tb[ETHTOOL_A_RINGS_RX];