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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Sep 12, 2024 at 12:47 AM Brett Creeley <bcreeley@xxxxxxx> 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@xxxxxxxxx>
> > ---
> >
> > 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





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux