Message-ID: <ZzZ_7SMsVnC5Wc2y@JRM7P7Q02P>
Date: Thu, 14 Nov 2024 17:55:41 -0500
From: Andy Gospodarek <andrew.gospodarek@...adcom.com>
To: Taehee Yoo <ap420073@...il.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
	edumazet@...gle.com, almasrymina@...gle.com,
	donald.hunter@...il.com, corbet@....net, michael.chan@...adcom.com,
	andrew+netdev@...n.ch, hawk@...nel.org, ilias.apalodimas@...aro.org,
	ast@...nel.org, daniel@...earbox.net, john.fastabend@...il.com,
	dw@...idwei.uk, sdf@...ichev.me, asml.silence@...il.com,
	brett.creeley@....com, linux-doc@...r.kernel.org,
	netdev@...r.kernel.org, kory.maincent@...tlin.com,
	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, rrameshbabu@...dia.com, idosch@...dia.com,
	jiri@...nulli.us, bigeasy@...utronix.de, lorenzo@...nel.org,
	jdamato@...tly.com, aleksander.lobakin@...el.com,
	kaiyuanz@...gle.com, willemb@...gle.com, daniel.zahka@...il.com
Subject: Re: [PATCH net-next v5 0/7] bnxt_en: implement tcp-data-split and
 thresh option

On Wed, Nov 13, 2024 at 05:32:14PM +0000, Taehee Yoo wrote:
> This series implements header-data-split-thresh ethtool command.
> This series also implements backend of tcp-data-split and
> header-data-split-thresh ethtool command for bnxt_en driver.
> These ethtool commands are mandatory options for device memory TCP.
> 
> NICs that use the bnxt_en driver support tcp-data-split feature named
> HDS(header-data-split).
> But there is no implementation for the HDS to enable by ethtool.
> Only getting the current HDS status is implemented and the HDS is just
> automatically enabled only when either LRO, HW-GRO, or JUMBO is enabled.
> The hds_threshold follows the rx-copybreak value but it wasn't
> changeable.
> 
> Currently, bnxt_en driver enables tcp-data-split by default but not
> always work.
> There is hds_threshold value, which indicates that a packet size is
> larger than this value, a packet will be split into header and data.
> hds_threshold value has been 256, which is a default value of
> rx-copybreak value too.
> The rx-copybreak value hasn't been allowed to change so the
> hds_threshold too.
> 
> This patchset decouples hds_threshold and rx-copybreak first.
> and make tcp-data-split, rx-copybreak, and
> header-data-split-thresh configurable independently.
> 
> But the default configuration is the same.
> The default value of rx-copybreak is 256 and default
> header-data-split-thresh is also 256.
> 
> There are several related options.
> TPA(HW-GRO, LRO), JUMBO, jumbo_thresh(firmware command), and Aggregation
> Ring.
> 
> The aggregation ring is fundamental to these all features.
> When gro/lro/jumbo packets are received, NIC receives the first packet
> from the normal ring.
> follow packets come from the aggregation ring.
> 
> These features are working regardless of HDS.
> If HDS is enabled, the first packet contains the header only, and the
> following packets contain only payload.
> So, HW-GRO/LRO is working regardless of HDS.
> 
> There is another threshold value, which is jumbo_thresh.
> This is very similar to hds_thresh, but jumbo thresh doesn't split
> header and data.
> It just split the first and following data based on length.
> When NIC receives 1500 sized packet, and jumbo_thresh is 256(default, but
> follows rx-copybreak),
> the first data is 256 and the following packet size is 1500-256.
> 
> Before this patch, at least if one of GRO, LRO, and JUMBO flags is
> enabled, the Aggregation ring will be enabled.
> If the Aggregation ring is enabled, both hds_threshold and
> jumbo_thresh are set to the default value of rx-copybreak.
> 
> So, GRO, LRO, JUMBO frames, they larger than 256 bytes, they will
> be split into header and data if the protocol is TCP or UDP.
> for the other protocol, jumbo_thresh works instead of hds_thresh.
> 
> This means that tcp-data-split relies on the GRO, LRO, and JUMBO flags.
> But by this patch, tcp-data-split no longer relies on these flags.
> If the tcp-data-split is enabled, the Aggregation ring will be
> enabled.
> Also, hds_threshold no longer follows rx-copybreak value, it will
> be set to the header-data-split-thresh value by user-space, but the
> default value is still 256.
> 
> If the protocol is TCP or UDP and the HDS is disabled and Aggregation
> ring is enabled, a packet will be split into several pieces due to
> jumbo_thresh.
> 
> When single buffer XDP is attached, tcp-data-split is automatically
> disabled.
> 
> LRO, GRO, and JUMBO are tested with BCM57414, BCM57504 and the firmware
> version is 230.0.157.0.
> I couldn't find any specification about minimum and maximum value
> of hds_threshold, but from my test result, it was about 0 ~ 1023.
> It means, over 1023 sized packets will be split into header and data if
> tcp-data-split is enabled regardless of hds_treshold value.
> When hds_threshold is 1500 and received packet size is 1400, HDS should
> not be activated, but it is activated.
> The maximum value of header-data-split-thresh value is 256 because it
> has been working. It was decided very conservatively.
> 
> I checked out the tcp-data-split(HDS) works independently of GRO, LRO,
> JUMBO.
> Also, I checked out tcp-data-split should be disabled automatically
> when XDP is attached and disallowed to enable it again while XDP is
> attached. I tested ranged values from min to max for
> header-data-split-thresh and rx-copybreak, and it works.
> header-data-split-thresh from 0 to 256, and rx-copybreak 0 to 256.
> When testing this patchset, I checked skb->data, skb->data_len, and
> nr_frags values.
> 
> By this patchset, bnxt_en driver supports a force enable tcp-data-split,
> but it doesn't support for disable tcp-data-split.
> When tcp-data-split is explicitly enabled, HDS works always.
> When tcp-data-split is unknown, it depends on the current
> configuration of LRO/GRO/JUMBO.
> 
> 1/7 patch implements .{set, get}_tunable() in the bnxt_en.
> The bnxt_en driver has been supporting the rx-copybreak feature but is
> not configurable, Only the default rx-copybreak value has been working.
> So, it changes the bnxt_en driver to be able to configure
> the rx-copybreak value.
> 
> 2/7 patch adds a new tcp_data_split_mod member in the
> kernel_ethtool_ringparam
> It indicates that user is explicitly set the tcp-data-split.
> So the driver can distinguish a passed tcp-data-split value is
> came from user or driver itself.
> 
> 3/7 patch adds an implementation of tcp-data-split ethtool
> command.
> The HDS relies on the Aggregation ring, which is automatically enabled
> when either LRO, GRO, or large mtu is configured.
> So, if the Aggregation ring is enabled, HDS is automatically enabled by
> it.
> 
> 4/7 patch adds header-data-split-thresh command in the ethtool.
> This threshold value indicates if a received packet size is larger
> than this threshold, the packet's header and payload will be split.
> Example:
>    # ethtool -G <interface name> header-data-split-thresh <value>
> This option can not be used when tcp-data-split is disabled or not
> supported.
>    # ethtool -G enp14s0f0np0 tcp-data-split on header-data-split-thresh 256
>    # ethtool -g enp14s0f0np0
>    Ring parameters for enp14s0f0np0:
>    Pre-set maximums:
>    ...
>    Current hardware settings:
>    ...
>    TCP data split:         on
>    Header data split thresh:  256
> 
> 5/7 patch adds the implementation of header-data-split-thresh logic
> in the bnxt_en driver.
> The default value is 256, which used to be the default rx-copybreak
> value.
> 
> 6/7, 7/7 add condition checks for devmem and ethtool.
> If tcp-data-split is disabled or threshold value is not zero, setup of
> devmem will be failed.
> Also, tcp-data-split and header-data-split-thresh will not be changed
> while devmem is running.
> 
> This series is tested with BCM57504.
> 
> All necessary configuration validations exist at the core API level.
> 
> v5:
>  - Drop implementation of device memory TCP for bnxt_en.
>  - Remove netdev_devmem_enabled() and use dev_get_min_mp_channel_count()
>    instead.
>  - change extack messages
>  - Add Review tags from Mina.
> 
> v4:
>  - Remove min rx-copybreak value.
>  - Do not support a disable of tcp-data-split by bnxt_en driver.
>  - Rename from tcp-data-split-thresh to header-data-split-thresh.
>  - Add ETHTOOL_RING_USE_HDS_THRS flag.
>  - Add dev_xdp_sb_prog_count() helper.
>  - Reduce hole in struct bnxt.
>  - Use ETHTOOL_RING_USE_HDS_THRS in bnxt_en driver.
>  - Improve condition check.
>  - Add netdev_devmem_enabled() helper.
>  - Add netmem_is_pfmemalloc() helper.
>  - Do not select NET_DEVMEM in Kconfig for bnxt_en driver.
>  - Pass PP_FLAG_ALLOW_UNREADABLE_NETMEM flag unconditionally.
>  - Use gfp flag in __bnxt_alloc_rx_netmem() in the last patch.
>  - Do not add *offset in the __bnxt_alloc_rx_netmem() in the last patch.
>  - Do not pass queue_idx to bnxt_alloc_rx_page_pool() in the last patch.
>  - Add Test tag from Stanislav.
>  - Add Review tag from Brett.
>  - Add page_pool_recycle_direct_netmem() helper
> 
> v3:
>  - Change headline
>  - Add condition checks for ethtool and devmem
>  - Fix documentation
>  - Move validation of tcp-data-split and thresh from dirver to core API
>  - Add implementation of device memory TCP for bnxt_en driver
> 
> v2:
>  - Add tcp-data-split-thresh ethtool command
>  - Implement tcp-data-split-threh in the bnxt_en driver
>  - Define min/max rx-copybreak value
>  - Update commit message
> 
> Taehee Yoo (7):
>   bnxt_en: add support for rx-copybreak ethtool command
>   net: ethtool: add tcp_data_split_mod member in
>     kernel_ethtool_ringparam
>   bnxt_en: add support for tcp-data-split ethtool command
>   net: ethtool: add support for configuring header-data-split-thresh
>   bnxt_en: add support for header-data-split-thresh ethtool command
>   net: devmem: add ring parameter filtering
>   net: ethtool: add ring parameter filtering
> 
>  Documentation/netlink/specs/ethtool.yaml      |  8 ++
>  Documentation/networking/ethtool-netlink.rst  | 79 ++++++++++++-------
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 31 +++++---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h     | 12 ++-
>  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 73 ++++++++++++++++-
>  include/linux/ethtool.h                       |  8 ++
>  include/linux/netdevice.h                     |  1 +
>  include/uapi/linux/ethtool_netlink.h          |  2 +
>  net/core/dev.c                                | 13 +++
>  net/core/devmem.c                             | 18 +++++
>  net/ethtool/netlink.h                         |  2 +-
>  net/ethtool/rings.c                           | 53 ++++++++++++-
>  12 files changed, 250 insertions(+), 50 deletions(-)
> 

Series looks good to me and testing also looks good.  Thanks for doing this!

Powered by blists - more mailing lists