On Thu, 3 Oct 2024 16:06:16 +0000 Taehee Yoo wrote: > The tcp-data-split-thresh option configures the threshold value of > the tcp-data-split. > If a received packet size is larger than this threshold value, a packet > will be split into header and payload. > The header indicates TCP header, but it depends on driver spec. > The bnxt_en driver supports HDS(Header-Data-Split) configuration at > FW level, affecting TCP and UDP too. > So, like the tcp-data-split option, If tcp-data-split-thresh is set, > it affects UDP and TCP packets. > > The tcp-data-split-thresh has a dependency, that is tcp-data-split > option. This threshold value can be get/set only when tcp-data-split > option is enabled. > > Example: > # ethtool -G <interface name> tcp-data-split-thresh <value> > > # ethtool -G enp14s0f0np0 tcp-data-split on tcp-data-split-thresh 256 > # ethtool -g enp14s0f0np0 > Ring parameters for enp14s0f0np0: > Pre-set maximums: > ... > TCP data split thresh: 256 > Current hardware settings: > ... > TCP data split: on > TCP data split thresh: 256 > > The tcp-data-split is not enabled, the tcp-data-split-thresh will > not be used and can't be configured. > > # ethtool -G enp14s0f0np0 tcp-data-split off > # ethtool -g enp14s0f0np0 > Ring parameters for enp14s0f0np0: > Pre-set maximums: > ... > TCP data split thresh: 256 > Current hardware settings: > ... > TCP data split: off > TCP data split thresh: n/a My reply to Sridhar was probably quite unclear on this point, but FWIW I do also have a weak preference to drop the "TCP" from the new knob. Rephrasing what I said here: https://lore.kernel.org/all/20240911173150.571bf93b@xxxxxxxxxx/ the old knob is defined as being about TCP but for the new one we can pick how widely applicable it is (and make it cover UDP as well). > The default/min/max values are not defined in the ethtool so the drivers > should define themself. > The 0 value means that all TCP and UDP packets' header and payload > will be split. > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 12f6dc567598..891f55b0f6aa 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 > + * @tcp_data_split_thresh: Threshold value of tcp-data-split > + * @tcp_data_split_thresh_max: Maximum allowed threshold of tcp-data-split-threshold Please wrap at 80 chars: ./scripts/checkpatch.pl --max-line-length=80 --strict $patch.. > static int rings_fill_reply(struct sk_buff *skb, > @@ -108,7 +110,13 @@ static int rings_fill_reply(struct sk_buff *skb, > (nla_put_u32(skb, ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN_MAX, > kr->tx_push_buf_max_len) || > nla_put_u32(skb, ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN, > - kr->tx_push_buf_len)))) > + kr->tx_push_buf_len))) || > + (kr->tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED && Please add a new ETHTOOL_RING_USE_* flag for this, or fix all the drivers which set ETHTOOL_RING_USE_TCP_DATA_SPLIT already and use that. I don't think we should hide the value when HDS is disabled. > + (nla_put_u32(skb, ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH, > + kr->tcp_data_split_thresh))) || nit: unnecessary brackets around the nla_put_u32() > + (kr->tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED && > + (nla_put_u32(skb, ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH_MAX, > + kr->tcp_data_split_thresh_max)))) > return -EMSGSIZE; > > return 0; > + if (tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH] && > + !(ops->supported_ring_params & ETHTOOL_RING_USE_TCP_DATA_SPLIT)) { here you use the existing flag, yet gve and idpf set that flag and will ignore the setting silently. They need to be changed or we need a new flag. > + NL_SET_ERR_MSG_ATTR(info->extack, > + tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH], > + "setting tcp-data-split-thresh is not supported"); > + return -EOPNOTSUPP; > + } > + > if (tb[ETHTOOL_A_RINGS_CQE_SIZE] && > !(ops->supported_ring_params & ETHTOOL_RING_USE_CQE_SIZE)) { > NL_SET_ERR_MSG_ATTR(info->extack, > @@ -196,9 +213,9 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info) > struct kernel_ethtool_ringparam kernel_ringparam = {}; > struct ethtool_ringparam ringparam = {}; > struct net_device *dev = req_info->dev; > + bool mod = false, thresh_mod = false; > struct nlattr **tb = info->attrs; > const struct nlattr *err_attr; > - bool mod = false; > int ret; > > dev->ethtool_ops->get_ringparam(dev, &ringparam, > @@ -222,9 +239,30 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info) > tb[ETHTOOL_A_RINGS_RX_PUSH], &mod); > ethnl_update_u32(&kernel_ringparam.tx_push_buf_len, > tb[ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN], &mod); > - if (!mod) > + ethnl_update_u32(&kernel_ringparam.tcp_data_split_thresh, > + tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH], > + &thresh_mod); > + if (!mod && !thresh_mod) > return 0; > > + if (kernel_ringparam.tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED && > + thresh_mod) { > + NL_SET_ERR_MSG_ATTR(info->extack, > + tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH], > + "tcp-data-split-thresh can not be updated while tcp-data-split is disabled"); > + return -EINVAL; > + } I'm not sure we need to reject changing the setting when HDS is disabled. Driver can just store the value until HDS gets enabled? WDYT? I don't have a strong preference. > + if (kernel_ringparam.tcp_data_split_thresh > > + kernel_ringparam.tcp_data_split_thresh_max) { > + NL_SET_ERR_MSG_ATTR_FMT(info->extack, > + tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH_MAX], > + "Requested tcp-data-split-thresh exceeds the maximum of %u", No need for the string, just NL_SET_BAD_ATTR() + ERANGE is enough > + kernel_ringparam.tcp_data_split_thresh_max); > + > + return -EINVAL; ERANGE > + } > + > /* ensure new ring parameters are within limits */ > if (ringparam.rx_pending > ringparam.rx_max_pending) > err_attr = tb[ETHTOOL_A_RINGS_RX];
