On Wed, Oct 9, 2024 at 3:33 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote: > > 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). I'm not sure that I understand about "knob". The knob means the command "tcp-data-split-thresh"? If so, I would like to change from "tcp-data-split-thresh" to "header-data-split-thresh". > > > 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.. Thanks, I will fix this in v4 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() I will fix this too. > > > + (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. Okay, I would like to add the ETHTOOL_RING_USE_TCP_DATA_SPLIT_THRESH flag. Or ETHTOOL_RING_USE_HDS_THRESH, which indicates header-data-split thresh. If you agree with adding a new flag, how do you think about naming it? > > > + 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. I checked similar options, which are tx-push and tx-push-buff-len, updating tx-push-buff-len may not fail when tx-push is disabled. I think It's too strong condition check and it's not consistent with similar options. The disabling HDS option is going to be removed in v4 patch. I asked about how to handle hds_threshold when it is UNKNOWN mode in the previous patch thread. If the hds_threshold should follow rx-copybreak value in the UNKNOWN mode, this condition check is not necessary. > > > + 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 Thanks, I will fix it. > > > + kernel_ringparam.tcp_data_split_thresh_max); > > + > > + return -EINVAL; > > ERANGE I will fix it too. > > > + } > > + > > /* ensure new ring parameters are within limits */ > > if (ringparam.rx_pending > ringparam.rx_max_pending) > > err_attr = tb[ETHTOOL_A_RINGS_RX]; >
