diff mbox series

[net-next,v2,3/4] ethtool: Add support for configuring tcp-data-split-thresh

Message ID 20240911145555.318605-4-ap420073@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series bnxt_en: implement tcp-data-split ethtool command | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 21 this patch: 21
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 75 this patch: 75
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1715 this patch: 1715
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 97 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 6 this patch: 6
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2024-09-11--18-00 (tests: 763)

Commit Message

Taehee Yoo Sept. 11, 2024, 2:55 p.m. UTC
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@gmail.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(-)

Comments

Kory Maincent Sept. 11, 2024, 3:26 p.m. UTC | #1
On Wed, 11 Sep 2024 14:55:54 +0000
Taehee Yoo <ap420073@gmail.com> 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.

Could you add a patch to modify the specs accordingly?
The specs are located here: Documentation/netlink/specs/ethtool.yaml
You can use ./tools/net/ynl tool and these specs to test ethtool netlink
messages.

Use this to verify that your specs update are well written.
$ make -C tools/net/ynl

> diff --git a/Documentation/networking/ethtool-netlink.rst
> b/Documentation/networking/ethtool-netlink.rst index
> ba90457b8b2d..bb74e108c8c1 100644 ---
> a/Documentation/networking/ethtool-netlink.rst +++
> b/Documentation/networking/ethtool-netlink.rst @@ -892,6 +892,7 @@ Kernel
> response contents: ``ETHTOOL_A_RINGS_RX_PUSH``               u8      flag of
> RX Push mode ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN``       u32     size of TX
> push buffer ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN_MAX``   u32     max size of TX
> push buffer
> +  ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH`` u32     threshold of TDS
>    =======================================   ======
> =========================== 

It seems there is a misalignment here. You need two more '=='

>  ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT`` indicates whether the device is usable
> with @@ -927,18 +928,20 @@ Sets ring sizes like ``ETHTOOL_SRINGPARAM`` ioctl
> request. 
>  Request contents:
>  
> -  ====================================  ======  ===========================
> -  ``ETHTOOL_A_RINGS_HEADER``            nested  reply header
> -  ``ETHTOOL_A_RINGS_RX``                u32     size of RX ring
> -  ``ETHTOOL_A_RINGS_RX_MINI``           u32     size of RX mini ring
> -  ``ETHTOOL_A_RINGS_RX_JUMBO``          u32     size of RX jumbo ring
> -  ``ETHTOOL_A_RINGS_TX``                u32     size of TX ring
> -  ``ETHTOOL_A_RINGS_RX_BUF_LEN``        u32     size of buffers on the ring
> -  ``ETHTOOL_A_RINGS_CQE_SIZE``          u32     Size of TX/RX CQE
> -  ``ETHTOOL_A_RINGS_TX_PUSH``           u8      flag of TX Push mode
> -  ``ETHTOOL_A_RINGS_RX_PUSH``           u8      flag of RX Push mode
> -  ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN``   u32     size of TX push buffer
> -  ====================================  ======  ===========================
> +  =======================================   ======
> ===========================
> +  ``ETHTOOL_A_RINGS_HEADER``                nested  reply header
> +  ``ETHTOOL_A_RINGS_RX``                    u32     size of RX ring
> +  ``ETHTOOL_A_RINGS_RX_MINI``               u32     size of RX mini ring
> +  ``ETHTOOL_A_RINGS_RX_JUMBO``              u32     size of RX jumbo ring
> +  ``ETHTOOL_A_RINGS_TX``                    u32     size of TX ring
> +  ``ETHTOOL_A_RINGS_RX_BUF_LEN``            u32     size of buffers on the
> ring
> +  ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT``        u8      TCP header / data split
> +  ``ETHTOOL_A_RINGS_CQE_SIZE``              u32     Size of TX/RX CQE
> +  ``ETHTOOL_A_RINGS_TX_PUSH``               u8      flag of TX Push mode
> +  ``ETHTOOL_A_RINGS_RX_PUSH``               u8      flag of RX Push mode
> +  ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN``       u32     size of TX push buffer
> +  ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH`` u32     threshold of TDS
> +  =======================================   ======
> =========================== 

same here.
Taehee Yoo Sept. 11, 2024, 3:42 p.m. UTC | #2
On Thu, Sep 12, 2024 at 12:26 AM Kory Maincent
<kory.maincent@bootlin.com> wrote:
>

Hi Kory, Thank you so much for the review!

> On Wed, 11 Sep 2024 14:55:54 +0000
> Taehee Yoo <ap420073@gmail.com> 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.
>
> Could you add a patch to modify the specs accordingly?
> The specs are located here: Documentation/netlink/specs/ethtool.yaml
> You can use ./tools/net/ynl tool and these specs to test ethtool netlink
> messages.
>
> Use this to verify that your specs update are well written.
> $ make -C tools/net/ynl

Thanks a lot! I will add a patch for ethtool.yaml.

>
> > diff --git a/Documentation/networking/ethtool-netlink.rst
> > b/Documentation/networking/ethtool-netlink.rst index
> > ba90457b8b2d..bb74e108c8c1 100644 ---
> > a/Documentation/networking/ethtool-netlink.rst +++
> > b/Documentation/networking/ethtool-netlink.rst @@ -892,6 +892,7 @@ Kernel
> > response contents: ``ETHTOOL_A_RINGS_RX_PUSH``               u8      flag of
> > RX Push mode ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN``       u32     size of TX
> > push buffer ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN_MAX``   u32     max size of TX
> > push buffer
> > +  ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH`` u32     threshold of TDS
> >    =======================================   ======
> > ===========================
>
> It seems there is a misalignment here. You need two more '=='
>
> >  ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT`` indicates whether the device is usable
> > with @@ -927,18 +928,20 @@ Sets ring sizes like ``ETHTOOL_SRINGPARAM`` ioctl
> > request.
> >  Request contents:
> >
> > -  ====================================  ======  ===========================
> > -  ``ETHTOOL_A_RINGS_HEADER``            nested  reply header
> > -  ``ETHTOOL_A_RINGS_RX``                u32     size of RX ring
> > -  ``ETHTOOL_A_RINGS_RX_MINI``           u32     size of RX mini ring
> > -  ``ETHTOOL_A_RINGS_RX_JUMBO``          u32     size of RX jumbo ring
> > -  ``ETHTOOL_A_RINGS_TX``                u32     size of TX ring
> > -  ``ETHTOOL_A_RINGS_RX_BUF_LEN``        u32     size of buffers on the ring
> > -  ``ETHTOOL_A_RINGS_CQE_SIZE``          u32     Size of TX/RX CQE
> > -  ``ETHTOOL_A_RINGS_TX_PUSH``           u8      flag of TX Push mode
> > -  ``ETHTOOL_A_RINGS_RX_PUSH``           u8      flag of RX Push mode
> > -  ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN``   u32     size of TX push buffer
> > -  ====================================  ======  ===========================
> > +  =======================================   ======
> > ===========================
> > +  ``ETHTOOL_A_RINGS_HEADER``                nested  reply header
> > +  ``ETHTOOL_A_RINGS_RX``                    u32     size of RX ring
> > +  ``ETHTOOL_A_RINGS_RX_MINI``               u32     size of RX mini ring
> > +  ``ETHTOOL_A_RINGS_RX_JUMBO``              u32     size of RX jumbo ring
> > +  ``ETHTOOL_A_RINGS_TX``                    u32     size of TX ring
> > +  ``ETHTOOL_A_RINGS_RX_BUF_LEN``            u32     size of buffers on the
> > ring
> > +  ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT``        u8      TCP header / data split
> > +  ``ETHTOOL_A_RINGS_CQE_SIZE``              u32     Size of TX/RX CQE
> > +  ``ETHTOOL_A_RINGS_TX_PUSH``               u8      flag of TX Push mode
> > +  ``ETHTOOL_A_RINGS_RX_PUSH``               u8      flag of RX Push mode
> > +  ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN``       u32     size of TX push buffer
> > +  ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH`` u32     threshold of TDS
> > +  =======================================   ======
> > ===========================
>
> same here.

Thanks, I will fix this too.

>
> --
> Köry Maincent, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com

Thanks a lot!
Taehee Yoo
Brett Creeley Sept. 11, 2024, 3:47 p.m. UTC | #3
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@gmail.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
> 
>
Taehee Yoo Sept. 11, 2024, 4:03 p.m. UTC | #4
On Thu, Sep 12, 2024 at 12:47 AM Brett Creeley <bcreeley@amd.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@gmail.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".

>
> 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
Brett Creeley Sept. 11, 2024, 4:06 p.m. UTC | #5
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@amd.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@gmail.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
Samudrala, Sridhar Sept. 11, 2024, 4:51 p.m. UTC | #6
On 9/11/2024 9:55 AM, 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.

What about non-tcp/udp packets? Are they are not split?
It is possible that they may be split at L3 payload for IP/IPV6 packets 
and L2 payload for non-ip packets.
So instead of calling this option as tcp-data-split-thresh, can we call 
it header-data-split-thresh?


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

Even the existing 'tcp-data-split' name is misleading. Not sure if it 
will be possible to change this now.
Jakub Kicinski Sept. 12, 2024, 12:31 a.m. UTC | #7
On Wed, 11 Sep 2024 11:51:42 -0500 Samudrala, Sridhar wrote:
> On 9/11/2024 9:55 AM, 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.  
> 
> What about non-tcp/udp packets? Are they are not split?
> It is possible that they may be split at L3 payload for IP/IPV6 packets 
> and L2 payload for non-ip packets.
> So instead of calling this option as tcp-data-split-thresh, can we call 
> it header-data-split-thresh?

This makes sense.

> > 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.  
> 
> Even the existing 'tcp-data-split' name is misleading. Not sure if it 
> will be possible to change this now.

It's not misleading, unless you think that it is something else than 
it is. 

  ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT`` indicates whether the device
  is usable with page-flipping TCP zero-copy receive
  (``getsockopt(TCP_ZEROCOPY_RECEIVE)``). If enabled the device is
  configured to place frame headers and data into separate buffers. 
  The device configuration must make it possible to receive full memory
  pages of data, for example because MTU is high enough or through
  HW-GRO.

If you use this for more than what's stated in the documentation
that's on you. More granular "what gets split and what doesn't"
control should probably go into an API akin to how we configure
RSS hashing fields. But I'm not sure anyone actually cares about
other protocols at this stage, so...
Samudrala, Sridhar Sept. 12, 2024, 3:42 p.m. UTC | #8
On 9/11/2024 7:31 PM, Jakub Kicinski wrote:
> On Wed, 11 Sep 2024 11:51:42 -0500 Samudrala, Sridhar wrote:
>> On 9/11/2024 9:55 AM, 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.
>>
>> What about non-tcp/udp packets? Are they are not split?
>> It is possible that they may be split at L3 payload for IP/IPV6 packets
>> and L2 payload for non-ip packets.
>> So instead of calling this option as tcp-data-split-thresh, can we call
>> it header-data-split-thresh?
> 
> This makes sense.
> 
>>> 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.
>>
>> Even the existing 'tcp-data-split' name is misleading. Not sure if it
>> will be possible to change this now.
> 
> It's not misleading, unless you think that it is something else than
> it is.
> 
>    ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT`` indicates whether the device
>    is usable with page-flipping TCP zero-copy receive
>    (``getsockopt(TCP_ZEROCOPY_RECEIVE)``). If enabled the device is
>    configured to place frame headers and data into separate buffers.
>    The device configuration must make it possible to receive full memory
>    pages of data, for example because MTU is high enough or through
>    HW-GRO.
> 
> If you use this for more than what's stated in the documentation
> that's on you. More granular "what gets split and what doesn't"
> control should probably go into an API akin to how we configure
> RSS hashing fields. But I'm not sure anyone actually cares about
> other protocols at this stage, so...

OK, as the the main use case for header split is tcp zero copy receive 
at this time and the documentation is also explicitly calling out TCP, 
this should be fine and we can introduce API to configure header split 
behavior for non-tcp protocols in future if required.
diff mbox series

Patch

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index ba90457b8b2d..bb74e108c8c1 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -892,6 +892,7 @@  Kernel response contents:
   ``ETHTOOL_A_RINGS_RX_PUSH``               u8      flag of RX Push mode
   ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN``       u32     size of TX push buffer
   ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN_MAX``   u32     max size of TX push buffer
+  ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH`` u32     threshold of TDS
   =======================================   ======  ===========================
 
 ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT`` indicates whether the device is usable with
@@ -927,18 +928,20 @@  Sets ring sizes like ``ETHTOOL_SRINGPARAM`` ioctl request.
 
 Request contents:
 
-  ====================================  ======  ===========================
-  ``ETHTOOL_A_RINGS_HEADER``            nested  reply header
-  ``ETHTOOL_A_RINGS_RX``                u32     size of RX ring
-  ``ETHTOOL_A_RINGS_RX_MINI``           u32     size of RX mini ring
-  ``ETHTOOL_A_RINGS_RX_JUMBO``          u32     size of RX jumbo ring
-  ``ETHTOOL_A_RINGS_TX``                u32     size of TX ring
-  ``ETHTOOL_A_RINGS_RX_BUF_LEN``        u32     size of buffers on the ring
-  ``ETHTOOL_A_RINGS_CQE_SIZE``          u32     Size of TX/RX CQE
-  ``ETHTOOL_A_RINGS_TX_PUSH``           u8      flag of TX Push mode
-  ``ETHTOOL_A_RINGS_RX_PUSH``           u8      flag of RX Push mode
-  ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN``   u32     size of TX push buffer
-  ====================================  ======  ===========================
+  =======================================   ======  ===========================
+  ``ETHTOOL_A_RINGS_HEADER``                nested  reply header
+  ``ETHTOOL_A_RINGS_RX``                    u32     size of RX ring
+  ``ETHTOOL_A_RINGS_RX_MINI``               u32     size of RX mini ring
+  ``ETHTOOL_A_RINGS_RX_JUMBO``              u32     size of RX jumbo ring
+  ``ETHTOOL_A_RINGS_TX``                    u32     size of TX ring
+  ``ETHTOOL_A_RINGS_RX_BUF_LEN``            u32     size of buffers on the ring
+  ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT``        u8      TCP header / data split
+  ``ETHTOOL_A_RINGS_CQE_SIZE``              u32     Size of TX/RX CQE
+  ``ETHTOOL_A_RINGS_TX_PUSH``               u8      flag of TX Push mode
+  ``ETHTOOL_A_RINGS_RX_PUSH``               u8      flag of RX Push mode
+  ``ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN``       u32     size of TX push buffer
+  ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH`` u32     threshold of TDS
+  =======================================   ======  ===========================
 
 Kernel checks that requested ring sizes do not exceed limits reported by
 driver. Driver may impose additional constraints and may not support all
@@ -954,6 +957,10 @@  A bigger CQE can have more receive buffer pointers, and in turn the NIC can
 transfer a bigger frame from wire. Based on the NIC hardware, the overall
 completion queue size can be adjusted in the driver if CQE size is modified.
 
+``ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH`` specifies the threshold value of
+tcp data split feature. If tcp-data-split is enabled and a received packet
+size is larger than this threshold value, header and data will be split.
+
 CHANNELS_GET
 ============
 
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 12f6dc567598..5f3d0a231e53 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -78,6 +78,7 @@  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
  */
 struct kernel_ethtool_ringparam {
 	u32	rx_buf_len;
@@ -87,6 +88,7 @@  struct kernel_ethtool_ringparam {
 	u32	cqe_size;
 	u32	tx_push_buf_len;
 	u32	tx_push_buf_max_len;
+	u32	tcp_data_split_thresh;
 };
 
 /**
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 283305f6b063..2be2d1840e7f 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -364,6 +364,7 @@  enum {
 	ETHTOOL_A_RINGS_RX_PUSH,			/* u8 */
 	ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN,		/* u32 */
 	ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN_MAX,		/* u32 */
+	ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH,		/* u32 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_RINGS_CNT,
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 203b08eb6c6f..d8dad0d10c8d 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -455,7 +455,7 @@  extern const struct nla_policy ethnl_features_set_policy[ETHTOOL_A_FEATURES_WANT
 extern const struct nla_policy ethnl_privflags_get_policy[ETHTOOL_A_PRIVFLAGS_HEADER + 1];
 extern const struct nla_policy ethnl_privflags_set_policy[ETHTOOL_A_PRIVFLAGS_FLAGS + 1];
 extern const struct nla_policy ethnl_rings_get_policy[ETHTOOL_A_RINGS_HEADER + 1];
-extern const struct nla_policy ethnl_rings_set_policy[ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN_MAX + 1];
+extern const struct nla_policy ethnl_rings_set_policy[ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH + 1];
 extern const struct nla_policy ethnl_channels_get_policy[ETHTOOL_A_CHANNELS_HEADER + 1];
 extern const struct nla_policy ethnl_channels_set_policy[ETHTOOL_A_CHANNELS_COMBINED_COUNT + 1];
 extern const struct nla_policy ethnl_coalesce_get_policy[ETHTOOL_A_COALESCE_HEADER + 1];
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");
+		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;
+	}
+
 	/* ensure new ring parameters are within limits */
 	if (ringparam.rx_pending > ringparam.rx_max_pending)
 		err_attr = tb[ETHTOOL_A_RINGS_RX];