| 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: <cf1881d6-9234-42ae-aa50-31ce45f427c8@amd.com>
Date: Wed, 11 Sep 2024 09:06:03 -0700
From: Brett Creeley <bcreeley@....com>
To: Taehee Yoo <ap420073@...il.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
edumazet@...gle.com, corbet@....net, michael.chan@...adcom.com,
netdev@...r.kernel.org, linux-doc@...r.kernel.org, ecree.xilinx@...il.com,
przemyslaw.kitszel@...el.com, andrew@...n.ch, hkallweit1@...il.com,
kory.maincent@...tlin.com, ahmed.zaki@...el.com, paul.greenwalt@...el.com,
rrameshbabu@...dia.com, idosch@...dia.com, maxime.chevallier@...tlin.com,
danieller@...dia.com, aleksander.lobakin@...el.com
Subject: Re: [PATCH net-next v2 3/4] ethtool: Add support for configuring
tcp-data-split-thresh
On 9/11/2024 9:03 AM, Taehee Yoo wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, Sep 12, 2024 at 12:47 AM Brett Creeley <bcreeley@....com> wrote:
>
> Hi Brett,
> Thanks a lot for your review!
>
>>
>>
>>
>> On 9/11/2024 7:55 AM, Taehee Yoo wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> 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:
>>> ...
>>> 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:
>>> ...
>>> Current hardware settings:
>>> ...
>>> TCP data split: off
>>> TCP data split thresh: n/a
>>>
>>> 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.
>>> Users should consider the overhead due to this feature.
>>>
>>> Signed-off-by: Taehee Yoo <ap420073@...il.com>
>>> ---
>>>
>>> v2:
>>> - Patch added.
>>>
>>> Documentation/networking/ethtool-netlink.rst | 31 +++++++++++--------
>>> include/linux/ethtool.h | 2 ++
>>> include/uapi/linux/ethtool_netlink.h | 1 +
>>> net/ethtool/netlink.h | 2 +-
>>> net/ethtool/rings.c | 32 +++++++++++++++++---
>>> 5 files changed, 51 insertions(+), 17 deletions(-)
>>>
>>
>> <snip>
>>
>>> diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
>>> index b7865a14fdf8..0b68ea316815 100644
>>> --- a/net/ethtool/rings.c
>>> +++ b/net/ethtool/rings.c
>>> @@ -61,7 +61,8 @@ static int rings_reply_size(const struct ethnl_req_info *req_base,
>>> nla_total_size(sizeof(u8)) + /* _RINGS_TX_PUSH */
>>> nla_total_size(sizeof(u8))) + /* _RINGS_RX_PUSH */
>>> nla_total_size(sizeof(u32)) + /* _RINGS_TX_PUSH_BUF_LEN */
>>> - nla_total_size(sizeof(u32)); /* _RINGS_TX_PUSH_BUF_LEN_MAX */
>>> + nla_total_size(sizeof(u32)) + /* _RINGS_TX_PUSH_BUF_LEN_MAX */
>>> + nla_total_size(sizeof(u32)); /* _RINGS_TCP_DATA_SPLIT_THRESH */
>>> }
>>>
>>> static int rings_fill_reply(struct sk_buff *skb,
>>> @@ -108,7 +109,10 @@ 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 &&
>>> + (nla_put_u32(skb, ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH,
>>> + kr->tcp_data_split_thresh))))
>>> return -EMSGSIZE;
>>>
>>> return 0;
>>> @@ -130,6 +134,7 @@ const struct nla_policy ethnl_rings_set_policy[] = {
>>> [ETHTOOL_A_RINGS_TX_PUSH] = NLA_POLICY_MAX(NLA_U8, 1),
>>> [ETHTOOL_A_RINGS_RX_PUSH] = NLA_POLICY_MAX(NLA_U8, 1),
>>> [ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN] = { .type = NLA_U32 },
>>> + [ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH] = { .type = NLA_U32 },
>>> };
>>>
>>> static int
>>> @@ -155,6 +160,14 @@ ethnl_set_rings_validate(struct ethnl_req_info *req_info,
>>> return -EOPNOTSUPP;
>>> }
>>>
>>> + if (tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH] &&
>>> + !(ops->supported_ring_params & ETHTOOL_RING_USE_TCP_DATA_SPLIT)) {
>>> + NL_SET_ERR_MSG_ATTR(info->extack,
>>> + tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH],
>>> + "setting TDS threshold is not supported");
>>
>> Small nit.
>>
>> Here you use "TDS threshold", but based on the TCP data split extack
>> message, it seems like it should be the following for consistency:
>>
>> "setting TCP data split threshold 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 +209,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 +235,20 @@ 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 think using the userspace command line argument names makes sense for
>> this extack message.
>
> I agree, that using "TDS" is not good for users.
> I will use "tcp-data-split-threshold" instead of "TDS threshold".
Sorry, just to clarify, I think the way you have it in this message is
okay IMO. It's the other message where there's a slight inconsistency
compared with the pre-existing extack message.
Thanks,
Brett
>
>>
>> Thanks,
>>
>> Brett
>>
>>> + }
>>> +
>>> /* ensure new ring parameters are within limits */
>>> if (ringparam.rx_pending > ringparam.rx_max_pending)
>>> err_attr = tb[ETHTOOL_A_RINGS_RX];
>>> --
>>> 2.34.1
>>>
>>>
>
> Thanks a lot!
> Taehee Yoo
Powered by blists - more mailing lists