| 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: <CAMArcTXvMi_QWsYFgt8TJcQQz6=edoGs3-5th=4mKaHO_X+hhQ@mail.gmail.com>
Date: Wed, 9 Oct 2024 23:25:55 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
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 Wed, Oct 9, 2024 at 3:33 AM Jakub Kicinski <kuba@...nel.org> 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@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).
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];
>
Powered by blists - more mailing lists