| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20241008113314.243f7c36@kernel.org>
Date: Tue, 8 Oct 2024 11:33:14 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Taehee Yoo <ap420073@...il.com>
Cc: davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com,
almasrymina@...gle.com, netdev@...r.kernel.org, linux-doc@...r.kernel.org,
donald.hunter@...il.com, corbet@....net, michael.chan@...adcom.com,
kory.maincent@...tlin.com, andrew@...n.ch, maxime.chevallier@...tlin.com,
danieller@...dia.com, hengqi@...ux.alibaba.com, ecree.xilinx@...il.com,
przemyslaw.kitszel@...el.com, hkallweit1@...il.com, ahmed.zaki@...el.com,
paul.greenwalt@...el.com, rrameshbabu@...dia.com, idosch@...dia.com,
asml.silence@...il.com, kaiyuanz@...gle.com, willemb@...gle.com,
aleksander.lobakin@...el.com, dw@...idwei.uk, sridhar.samudrala@...el.com,
bcreeley@....com
Subject: Re: [PATCH net-next v3 3/7] net: ethtool: add support for
configuring tcp-data-split-thresh
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@kernel.org/
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];
Powered by blists - more mailing lists