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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1eec50e5-6a6d-4ad8-a3ad-b0bbb8e72724@amd.com>
Date: Wed, 11 Sep 2024 08:47:32 -0700
From: Brett Creeley <bcreeley@....com>
To: Taehee Yoo <ap420073@...il.com>, 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
Cc: 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 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.

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
> 
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ