[PATCH net-next v2 0/4] bnxt_en: implement tcp-data-split ethtool command

[PATCH net-next v2 0/4] bnxt_en: implement tcp-data-split ethtool command

[Taehee Yoo]

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/disable 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
tcp-data-split-thresh(hds_threshold) configurable independently.

But the default configuration is the same.
The default value of rx-copybreak is 256 and default
tcp-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.
When TPA is enabled and HDS is disabled, the first packet contains
header and payload too.
and the following packets contain payload only.
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 tcp-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 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 hds_threshold(tcp-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. Tested GRO/LRO, JUMBO with enabled HDS and disabled HDS.
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
tcp-data-split-thresh and rx-copybreak, and it works.
tcp-data-split-thresh from 0 to 256, and rx-copybreak 65 to 256.
When testing this patchset, I checked skb->data, skb->data_len, and
nr_frags values.

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

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

The third patch adds tcp-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> tcp-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 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

   # 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 fourth patch adds the implementation of tcp-data-split-thresh logic
in the bnxt_en driver.
The default value is 256, which used to be the default rx-copybreak
value.

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 (4):
  bnxt_en: add support for rx-copybreak ethtool command
  bnxt_en: add support for tcp-data-split ethtool command
  ethtool: Add support for configuring tcp-data-split-thresh
  bnxt_en: add support for tcp-data-split-thresh ethtool command

 Documentation/networking/ethtool-netlink.rst  | 31 ++++---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 32 +++++---
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     | 13 ++-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 81 ++++++++++++++++++-
 include/linux/ethtool.h                       |  2 +
 include/uapi/linux/ethtool_netlink.h          |  1 +
 net/ethtool/netlink.h                         |  2 +-
 net/ethtool/rings.c                           | 32 +++++++-
 8 files changed, 158 insertions(+), 36 deletions(-)

-- 
2.34.1

[PATCH net-next v2 1/4] bnxt_en: add support for rx-copybreak ethtool command

[Taehee Yoo]

The bnxt_en driver supports rx-copybreak, but it couldn't be set by
userspace. Only the default value(256) has worked.
This patch makes the bnxt_en driver support following command.
`ethtool --set-tunable <devname> rx-copybreak <value> ` and
`ethtool --get-tunable <devname> rx-copybreak`.

Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

v2:
 - Define max/vim rx_copybreak value.

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 24 ++++++----
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  6 ++-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 47 ++++++++++++++++++-
 3 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 6e422e24750a..8da211e083a4 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -81,7 +81,6 @@ MODULE_DESCRIPTION("Broadcom NetXtreme network driver");
 
 #define BNXT_RX_OFFSET (NET_SKB_PAD + NET_IP_ALIGN)
 #define BNXT_RX_DMA_OFFSET NET_SKB_PAD
-#define BNXT_RX_COPY_THRESH 256
 
 #define BNXT_TX_PUSH_THRESH 164
 
@@ -1330,13 +1329,13 @@ static struct sk_buff *bnxt_copy_data(struct bnxt_napi *bnapi, u8 *data,
 	if (!skb)
 		return NULL;
 
-	dma_sync_single_for_cpu(&pdev->dev, mapping, bp->rx_copy_thresh,
+	dma_sync_single_for_cpu(&pdev->dev, mapping, bp->rx_copybreak,
 				bp->rx_dir);
 
 	memcpy(skb->data - NET_IP_ALIGN, data - NET_IP_ALIGN,
 	       len + NET_IP_ALIGN);
 
-	dma_sync_single_for_device(&pdev->dev, mapping, bp->rx_copy_thresh,
+	dma_sync_single_for_device(&pdev->dev, mapping, bp->rx_copybreak,
 				   bp->rx_dir);
 
 	skb_put(skb, len);
@@ -1829,7 +1828,7 @@ static inline struct sk_buff *bnxt_tpa_end(struct bnxt *bp,
 		return NULL;
 	}
 
-	if (len <= bp->rx_copy_thresh) {
+	if (len <= bp->rx_copybreak) {
 		skb = bnxt_copy_skb(bnapi, data_ptr, len, mapping);
 		if (!skb) {
 			bnxt_abort_tpa(cpr, idx, agg_bufs);
@@ -1931,6 +1930,7 @@ static void bnxt_deliver_skb(struct bnxt *bp, struct bnxt_napi *bnapi,
 		bnxt_vf_rep_rx(bp, skb);
 		return;
 	}
+
 	skb_record_rx_queue(skb, bnapi->index);
 	napi_gro_receive(&bnapi->napi, skb);
 }
@@ -2162,7 +2162,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 		}
 	}
 
-	if (len <= bp->rx_copy_thresh) {
+	if (len <= bp->rx_copybreak) {
 		if (!xdp_active)
 			skb = bnxt_copy_skb(bnapi, data_ptr, len, dma_addr);
 		else
@@ -4451,6 +4451,11 @@ void bnxt_set_tpa_flags(struct bnxt *bp)
 		bp->flags |= BNXT_FLAG_GRO;
 }
 
+static void bnxt_init_ring_params(struct bnxt *bp)
+{
+	bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
+}
+
 /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
  * be set on entry.
  */
@@ -4465,7 +4470,6 @@ void bnxt_set_ring_params(struct bnxt *bp)
 	rx_space = rx_size + ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) +
 		SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 
-	bp->rx_copy_thresh = BNXT_RX_COPY_THRESH;
 	ring_size = bp->rx_ring_size;
 	bp->rx_agg_ring_size = 0;
 	bp->rx_agg_nr_pages = 0;
@@ -4510,7 +4514,8 @@ void bnxt_set_ring_params(struct bnxt *bp)
 				  ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) -
 				  SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 		} else {
-			rx_size = SKB_DATA_ALIGN(BNXT_RX_COPY_THRESH + NET_IP_ALIGN);
+			rx_size = SKB_DATA_ALIGN(bp->rx_copybreak +
+						 NET_IP_ALIGN);
 			rx_space = rx_size + NET_SKB_PAD +
 				SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 		}
@@ -6424,8 +6429,8 @@ static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
 					  VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV6);
 		req->enables |=
 			cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_HDS_THRESHOLD_VALID);
-		req->jumbo_thresh = cpu_to_le16(bp->rx_copy_thresh);
-		req->hds_threshold = cpu_to_le16(bp->rx_copy_thresh);
+		req->jumbo_thresh = cpu_to_le16(bp->rx_copybreak);
+		req->hds_threshold = cpu_to_le16(bp->rx_copybreak);
 	}
 	req->vnic_id = cpu_to_le32(vnic->fw_vnic_id);
 	return hwrm_req_send(bp, req);
@@ -15864,6 +15869,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	bnxt_init_l2_fltr_tbl(bp);
 	bnxt_set_rx_skb_mode(bp, false);
 	bnxt_set_tpa_flags(bp);
+	bnxt_init_ring_params(bp);
 	bnxt_set_ring_params(bp);
 	bnxt_rdma_aux_device_init(bp);
 	rc = bnxt_set_dflt_rings(bp, true);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 69231e85140b..cff031993223 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -34,6 +34,10 @@
 #include <linux/firmware/broadcom/tee_bnxt_fw.h>
 #endif
 
+#define BNXT_DEFAULT_RX_COPYBREAK 256
+#define BNXT_MIN_RX_COPYBREAK 65
+#define BNXT_MAX_RX_COPYBREAK 1024
+
 extern struct list_head bnxt_block_cb_list;
 
 struct page_pool;
@@ -2299,7 +2303,7 @@ struct bnxt {
 	enum dma_data_direction	rx_dir;
 	u32			rx_ring_size;
 	u32			rx_agg_ring_size;
-	u32			rx_copy_thresh;
+	u32			rx_copybreak;
 	u32			rx_ring_mask;
 	u32			rx_agg_ring_mask;
 	int			rx_nr_pages;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index f71cc8188b4e..201c3fcba04e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -4319,6 +4319,49 @@ static int bnxt_get_eee(struct net_device *dev, struct ethtool_keee *edata)
 	return 0;
 }
 
+static int bnxt_set_tunable(struct net_device *dev,
+			    const struct ethtool_tunable *tuna,
+			    const void *data)
+{
+	struct bnxt *bp = netdev_priv(dev);
+	u32 rx_copybreak;
+
+	switch (tuna->id) {
+	case ETHTOOL_RX_COPYBREAK:
+		rx_copybreak = *(u32 *)data;
+		if (rx_copybreak < BNXT_MIN_RX_COPYBREAK ||
+		    rx_copybreak > BNXT_MAX_RX_COPYBREAK)
+			return -EINVAL;
+		if (rx_copybreak != bp->rx_copybreak) {
+			bp->rx_copybreak = rx_copybreak;
+			if (netif_running(dev)) {
+				bnxt_close_nic(bp, false, false);
+				bnxt_set_ring_params(bp);
+				bnxt_open_nic(bp, false, false);
+			}
+		}
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int bnxt_get_tunable(struct net_device *dev,
+			    const struct ethtool_tunable *tuna, void *data)
+{
+	struct bnxt *bp = netdev_priv(dev);
+
+	switch (tuna->id) {
+	case ETHTOOL_RX_COPYBREAK:
+		*(u32 *)data = bp->rx_copybreak;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int bnxt_read_sfp_module_eeprom_info(struct bnxt *bp, u16 i2c_addr,
 					    u16 page_number, u8 bank,
 					    u16 start_addr, u16 data_length,
@@ -4769,7 +4812,7 @@ static int bnxt_run_loopback(struct bnxt *bp)
 	cpr = &rxr->bnapi->cp_ring;
 	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS)
 		cpr = rxr->rx_cpr;
-	pkt_size = min(bp->dev->mtu + ETH_HLEN, bp->rx_copy_thresh);
+	pkt_size = min(bp->dev->mtu + ETH_HLEN, bp->rx_copybreak);
 	skb = netdev_alloc_skb(bp->dev, pkt_size);
 	if (!skb)
 		return -ENOMEM;
@@ -5342,6 +5385,8 @@ const struct ethtool_ops bnxt_ethtool_ops = {
 	.get_link_ext_stats	= bnxt_get_link_ext_stats,
 	.get_eee		= bnxt_get_eee,
 	.set_eee		= bnxt_set_eee,
+	.get_tunable		= bnxt_get_tunable,
+	.set_tunable		= bnxt_set_tunable,
 	.get_module_info	= bnxt_get_module_info,
 	.get_module_eeprom	= bnxt_get_module_eeprom,
 	.get_module_eeprom_by_page = bnxt_get_module_eeprom_by_page,
-- 
2.34.1

Re: [PATCH net-next v2 1/4] bnxt_en: add support for rx-copybreak ethtool command

[Brett Creeley]

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 bnxt_en driver supports rx-copybreak, but it couldn't be set by
> userspace. Only the default value(256) has worked.
> This patch makes the bnxt_en driver support following command.
> `ethtool --set-tunable <devname> rx-copybreak <value> ` and
> `ethtool --get-tunable <devname> rx-copybreak`.
> 
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---
> 
> v2:
>   - Define max/vim rx_copybreak value.
> 
>   drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 24 ++++++----
>   drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  6 ++-
>   .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 47 ++++++++++++++++++-
>   3 files changed, 66 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

<snip>

> +static void bnxt_init_ring_params(struct bnxt *bp)
> +{
> +       bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
> +}
> +
>   /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
>    * be set on entry.
>    */
> @@ -4465,7 +4470,6 @@ void bnxt_set_ring_params(struct bnxt *bp)
>          rx_space = rx_size + ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) +
>                  SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> 
> -       bp->rx_copy_thresh = BNXT_RX_COPY_THRESH;
>          ring_size = bp->rx_ring_size;
>          bp->rx_agg_ring_size = 0;
>          bp->rx_agg_nr_pages = 0;
> @@ -4510,7 +4514,8 @@ void bnxt_set_ring_params(struct bnxt *bp)
>                                    ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) -
>                                    SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>                  } else {
> -                       rx_size = SKB_DATA_ALIGN(BNXT_RX_COPY_THRESH + NET_IP_ALIGN);
> +                       rx_size = SKB_DATA_ALIGN(bp->rx_copybreak +
> +                                                NET_IP_ALIGN);

Tiny nit, but why did you wrap NET_IP_ALIGN to the next line?

>                          rx_space = rx_size + NET_SKB_PAD +
>                                  SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>                  }
> @@ -6424,8 +6429,8 @@ static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
>                                            VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV6);
>                  req->enables |=
>                          cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_HDS_THRESHOLD_VALID);
> -               req->jumbo_thresh = cpu_to_le16(bp->rx_copy_thresh);
> -               req->hds_threshold = cpu_to_le16(bp->rx_copy_thresh);
> +               req->jumbo_thresh = cpu_to_le16(bp->rx_copybreak);
> +               req->hds_threshold = cpu_to_le16(bp->rx_copybreak);
>          }
>          req->vnic_id = cpu_to_le32(vnic->fw_vnic_id);
>          return hwrm_req_send(bp, req);
> @@ -15864,6 +15869,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>          bnxt_init_l2_fltr_tbl(bp);
>          bnxt_set_rx_skb_mode(bp, false);
>          bnxt_set_tpa_flags(bp);
> +       bnxt_init_ring_params(bp);
>          bnxt_set_ring_params(bp);
>          bnxt_rdma_aux_device_init(bp);
>          rc = bnxt_set_dflt_rings(bp, true);

<snip>

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index f71cc8188b4e..201c3fcba04e 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -4319,6 +4319,49 @@ static int bnxt_get_eee(struct net_device *dev, struct ethtool_keee *edata)
>          return 0;
>   }
> 
> +static int bnxt_set_tunable(struct net_device *dev,
> +                           const struct ethtool_tunable *tuna,
> +                           const void *data)
> +{
> +       struct bnxt *bp = netdev_priv(dev);
> +       u32 rx_copybreak;
> +
> +       switch (tuna->id) {
> +       case ETHTOOL_RX_COPYBREAK:
> +               rx_copybreak = *(u32 *)data;
> +               if (rx_copybreak < BNXT_MIN_RX_COPYBREAK ||
> +                   rx_copybreak > BNXT_MAX_RX_COPYBREAK)
> +                       return -EINVAL;
> +               if (rx_copybreak != bp->rx_copybreak) {
> +                       bp->rx_copybreak = rx_copybreak;

Should bp->rx_copybreak get set before closing the interface in the 
netif_running case? Can changing this while traffic is running cause any 
unexpected issues?

I wonder if this would be better/safer?

if (netif_running(dev)) {
	bnxt_close_nic(bp, false, false);
	bp->rx_copybreak = rx_copybreak;
	bnxt_set_ring_params(bp);
	bnxt_open_nic(bp, false, false);
} else {
	bp->rx_copybreak = rx_copybreak;
}

Thanks,

Brett

> +                       if (netif_running(dev)) {
> +                               bnxt_close_nic(bp, false, false);
> +                               bnxt_set_ring_params(bp);
> +                               bnxt_open_nic(bp, false, false);
> +                       }
> +               }
> +               return 0;
> +       default:
> +               return -EOPNOTSUPP;
> +       }
> +}
> +

<snip>

Re: [PATCH net-next v2 1/4] bnxt_en: add support for rx-copybreak ethtool command

[Taehee Yoo]

On Thu, Sep 12, 2024 at 12:36 AM Brett Creeley <bcreeley@amd.com> wrote:

Hi Brett,
Thank you so much 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 bnxt_en driver supports rx-copybreak, but it couldn't be set by
> > userspace. Only the default value(256) has worked.
> > This patch makes the bnxt_en driver support following command.
> > `ethtool --set-tunable <devname> rx-copybreak <value> ` and
> > `ethtool --get-tunable <devname> rx-copybreak`.
> >
> > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > ---
> >
> > v2:
> > - Define max/vim rx_copybreak value.
> >
> > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 24 ++++++----
> > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 6 ++-
> > .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 47 ++++++++++++++++++-
> > 3 files changed, 66 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>
> <snip>
>
> > +static void bnxt_init_ring_params(struct bnxt *bp)
> > +{
> > + bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
> > +}
> > +
> > /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
> > * be set on entry.
> > */
> > @@ -4465,7 +4470,6 @@ void bnxt_set_ring_params(struct bnxt *bp)
> > rx_space = rx_size + ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) +
> > SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> >
> > - bp->rx_copy_thresh = BNXT_RX_COPY_THRESH;
> > ring_size = bp->rx_ring_size;
> > bp->rx_agg_ring_size = 0;
> > bp->rx_agg_nr_pages = 0;
> > @@ -4510,7 +4514,8 @@ void bnxt_set_ring_params(struct bnxt *bp)
> > ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) -
> > SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > } else {
> > - rx_size = SKB_DATA_ALIGN(BNXT_RX_COPY_THRESH + NET_IP_ALIGN);
> > + rx_size = SKB_DATA_ALIGN(bp->rx_copybreak +
> > + NET_IP_ALIGN);
>
> Tiny nit, but why did you wrap NET_IP_ALIGN to the next line?

Because It exceeds 80 characters line.

>
> > rx_space = rx_size + NET_SKB_PAD +
> > SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > }
> > @@ -6424,8 +6429,8 @@ static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
> > VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV6);
> > req->enables |=
> > cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_HDS_THRESHOLD_VALID);
> > - req->jumbo_thresh = cpu_to_le16(bp->rx_copy_thresh);
> > - req->hds_threshold = cpu_to_le16(bp->rx_copy_thresh);
> > + req->jumbo_thresh = cpu_to_le16(bp->rx_copybreak);
> > + req->hds_threshold = cpu_to_le16(bp->rx_copybreak);
> > }
> > req->vnic_id = cpu_to_le32(vnic->fw_vnic_id);
> > return hwrm_req_send(bp, req);
> > @@ -15864,6 +15869,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> > bnxt_init_l2_fltr_tbl(bp);
> > bnxt_set_rx_skb_mode(bp, false);
> > bnxt_set_tpa_flags(bp);
> > + bnxt_init_ring_params(bp);
> > bnxt_set_ring_params(bp);
> > bnxt_rdma_aux_device_init(bp);
> > rc = bnxt_set_dflt_rings(bp, true);
>
> <snip>
>
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > index f71cc8188b4e..201c3fcba04e 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > @@ -4319,6 +4319,49 @@ static int bnxt_get_eee(struct net_device *dev, struct ethtool_keee *edata)
> > return 0;
> > }
> >
> > +static int bnxt_set_tunable(struct net_device *dev,
> > + const struct ethtool_tunable *tuna,
> > + const void *data)
> > +{
> > + struct bnxt *bp = netdev_priv(dev);
> > + u32 rx_copybreak;
> > +
> > + switch (tuna->id) {
> > + case ETHTOOL_RX_COPYBREAK:
> > + rx_copybreak = *(u32 *)data;
> > + if (rx_copybreak < BNXT_MIN_RX_COPYBREAK ||
> > + rx_copybreak > BNXT_MAX_RX_COPYBREAK)
> > + return -EINVAL;
> > + if (rx_copybreak != bp->rx_copybreak) {
> > + bp->rx_copybreak = rx_copybreak;
>
> Should bp->rx_copybreak get set before closing the interface in the
> netif_running case? Can changing this while traffic is running cause any
> unexpected issues?
>
> I wonder if this would be better/safer?
>
> if (netif_running(dev)) {
> bnxt_close_nic(bp, false, false);
> bp->rx_copybreak = rx_copybreak;
> bnxt_set_ring_params(bp);
> bnxt_open_nic(bp, false, false);
> } else {
> bp->rx_copybreak = rx_copybreak;
> }

I think your suggestion is much safer!
I will use your suggestion in the v3 patch.

>
> Thanks,
>
> Brett
>
> > + if (netif_running(dev)) {
> > + bnxt_close_nic(bp, false, false);
> > + bnxt_set_ring_params(bp);
> > + bnxt_open_nic(bp, false, false);
> > + }
> > + }
> > + return 0;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
>
> <snip>

Thanks a lot!
Taehee Yoo

Re: [PATCH net-next v2 1/4] bnxt_en: add support for rx-copybreak ethtool command

[Jakub Kicinski]

On Thu, 12 Sep 2024 00:53:31 +0900 Taehee Yoo wrote:
> > if (netif_running(dev)) {
> > bnxt_close_nic(bp, false, false);
> > bp->rx_copybreak = rx_copybreak;
> > bnxt_set_ring_params(bp);
> > bnxt_open_nic(bp, false, false);
> > } else {
> > bp->rx_copybreak = rx_copybreak;
> > }  
> 
> I think your suggestion is much safer!
> I will use your suggestion in the v3 patch.

This is better but Andy mentioned on another thread that queue reset
should work, so instead of full close / open maybe we can just do:

	for (/* all Rx queues */) {
		bnxt_queue_stop();
		bnxt_queue_start();
	}

when the device is already running?

Re: [PATCH net-next v2 1/4] bnxt_en: add support for rx-copybreak ethtool command

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

On Wed, Sep 11, 2024 at 5:22 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 12 Sep 2024 00:53:31 +0900 Taehee Yoo wrote:
> > > if (netif_running(dev)) {
> > > bnxt_close_nic(bp, false, false);
> > > bp->rx_copybreak = rx_copybreak;
> > > bnxt_set_ring_params(bp);
> > > bnxt_open_nic(bp, false, false);
> > > } else {
> > > bp->rx_copybreak = rx_copybreak;
> > > }
> >
> > I think your suggestion is much safer!
> > I will use your suggestion in the v3 patch.
>
> This is better but Andy mentioned on another thread that queue reset
> should work, so instead of full close / open maybe we can just do:
>
>         for (/* all Rx queues */) {
>                 bnxt_queue_stop();
>                 bnxt_queue_start();
>         }
>
> when the device is already running?

If the copybreak value changes, I don't think queue restart will work.
We need to size and allocate the buffers differently than before, so I
think we need to do close/open.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH net-next v2 2/4] bnxt_en: add support for tcp-data-split ethtool command

[Taehee Yoo]

NICs that uses bnxt_en driver supports tcp-data-split feature by the
name of HDS(header-data-split).
But there is no implementation for the HDS to enable or disable 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 rx-copybreak value. and it was unchangeable.

This implements `ethtool -G <interface name> tcp-data-split <value>`
command option.
The value can be <on>, <off>, and <auto> but the <auto> will be
automatically changed to <on>.

HDS feature relies on the aggregation ring.
So, if HDS is enabled, the bnxt_en driver initializes the aggregation
ring.
This is the reason why BNXT_FLAG_AGG_RINGS contains HDS condition.

Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

v2:
 - Do not set hds_threshold to 0.

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  9 +++----
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  5 ++--
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 25 +++++++++++++++++--
 3 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 8da211e083a4..f046478dfd2a 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4454,6 +4454,7 @@ void bnxt_set_tpa_flags(struct bnxt *bp)
 static void bnxt_init_ring_params(struct bnxt *bp)
 {
 	bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
+	bp->flags |= BNXT_FLAG_HDS;
 }
 
 /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
@@ -4474,7 +4475,7 @@ void bnxt_set_ring_params(struct bnxt *bp)
 	bp->rx_agg_ring_size = 0;
 	bp->rx_agg_nr_pages = 0;
 
-	if (bp->flags & BNXT_FLAG_TPA)
+	if (bp->flags & BNXT_FLAG_TPA || bp->flags & BNXT_FLAG_HDS)
 		agg_factor = min_t(u32, 4, 65536 / BNXT_RX_PAGE_SIZE);
 
 	bp->flags &= ~BNXT_FLAG_JUMBO;
@@ -6421,15 +6422,13 @@ static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
 
 	req->flags = cpu_to_le32(VNIC_PLCMODES_CFG_REQ_FLAGS_JUMBO_PLACEMENT);
 	req->enables = cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_JUMBO_THRESH_VALID);
+	req->jumbo_thresh = cpu_to_le16(bp->rx_buf_use_size);
 
-	if (BNXT_RX_PAGE_MODE(bp)) {
-		req->jumbo_thresh = cpu_to_le16(bp->rx_buf_use_size);
-	} else {
+	if (bp->flags & BNXT_FLAG_HDS) {
 		req->flags |= cpu_to_le32(VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV4 |
 					  VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV6);
 		req->enables |=
 			cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_HDS_THRESHOLD_VALID);
-		req->jumbo_thresh = cpu_to_le16(bp->rx_copybreak);
 		req->hds_threshold = cpu_to_le16(bp->rx_copybreak);
 	}
 	req->vnic_id = cpu_to_le32(vnic->fw_vnic_id);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index cff031993223..35601c71dfe9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2202,8 +2202,6 @@ struct bnxt {
 	#define BNXT_FLAG_TPA		(BNXT_FLAG_LRO | BNXT_FLAG_GRO)
 	#define BNXT_FLAG_JUMBO		0x10
 	#define BNXT_FLAG_STRIP_VLAN	0x20
-	#define BNXT_FLAG_AGG_RINGS	(BNXT_FLAG_JUMBO | BNXT_FLAG_GRO | \
-					 BNXT_FLAG_LRO)
 	#define BNXT_FLAG_RFS		0x100
 	#define BNXT_FLAG_SHARED_RINGS	0x200
 	#define BNXT_FLAG_PORT_STATS	0x400
@@ -2224,6 +2222,9 @@ struct bnxt {
 	#define BNXT_FLAG_ROCE_MIRROR_CAP	0x4000000
 	#define BNXT_FLAG_TX_COAL_CMPL	0x8000000
 	#define BNXT_FLAG_PORT_STATS_EXT	0x10000000
+	#define BNXT_FLAG_HDS		0x20000000
+	#define BNXT_FLAG_AGG_RINGS	(BNXT_FLAG_JUMBO | BNXT_FLAG_GRO | \
+					 BNXT_FLAG_LRO | BNXT_FLAG_HDS)
 
 	#define BNXT_FLAG_ALL_CONFIG_FEATS (BNXT_FLAG_TPA |		\
 					    BNXT_FLAG_RFS |		\
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 201c3fcba04e..ab64d7f94796 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -829,12 +829,16 @@ static void bnxt_get_ringparam(struct net_device *dev,
 	if (bp->flags & BNXT_FLAG_AGG_RINGS) {
 		ering->rx_max_pending = BNXT_MAX_RX_DESC_CNT_JUM_ENA;
 		ering->rx_jumbo_max_pending = BNXT_MAX_RX_JUM_DESC_CNT;
-		kernel_ering->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_ENABLED;
 	} else {
 		ering->rx_max_pending = BNXT_MAX_RX_DESC_CNT;
 		ering->rx_jumbo_max_pending = 0;
-		kernel_ering->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_DISABLED;
 	}
+
+	if (bp->flags & BNXT_FLAG_HDS)
+		kernel_ering->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_ENABLED;
+	else
+		kernel_ering->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_DISABLED;
+
 	ering->tx_max_pending = BNXT_MAX_TX_DESC_CNT;
 
 	ering->rx_pending = bp->rx_ring_size;
@@ -854,9 +858,25 @@ static int bnxt_set_ringparam(struct net_device *dev,
 	    (ering->tx_pending < BNXT_MIN_TX_DESC_CNT))
 		return -EINVAL;
 
+	if (kernel_ering->tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_DISABLED &&
+	    BNXT_RX_PAGE_MODE(bp)) {
+		NL_SET_ERR_MSG_MOD(extack, "tcp-data-split can not be enabled with XDP");
+		return -EINVAL;
+	}
+
 	if (netif_running(dev))
 		bnxt_close_nic(bp, false, false);
 
+	switch (kernel_ering->tcp_data_split) {
+	case ETHTOOL_TCP_DATA_SPLIT_UNKNOWN:
+	case ETHTOOL_TCP_DATA_SPLIT_ENABLED:
+		bp->flags |= BNXT_FLAG_HDS;
+		break;
+	case ETHTOOL_TCP_DATA_SPLIT_DISABLED:
+		bp->flags &= ~BNXT_FLAG_HDS;
+		break;
+	}
+
 	bp->rx_ring_size = ering->rx_pending;
 	bp->tx_ring_size = ering->tx_pending;
 	bnxt_set_ring_params(bp);
@@ -5344,6 +5364,7 @@ const struct ethtool_ops bnxt_ethtool_ops = {
 				     ETHTOOL_COALESCE_STATS_BLOCK_USECS |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX |
 				     ETHTOOL_COALESCE_USE_CQE,
+	.supported_ring_params	= ETHTOOL_RING_USE_TCP_DATA_SPLIT,
 	.get_link_ksettings	= bnxt_get_link_ksettings,
 	.set_link_ksettings	= bnxt_set_link_ksettings,
 	.get_fec_stats		= bnxt_get_fec_stats,
-- 
2.34.1

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

[Taehee Yoo]

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

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];
-- 
2.34.1

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

[Kory Maincent]

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.

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

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

[Taehee Yoo]

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

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

[Brett Creeley]

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

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

[Taehee Yoo]

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

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

[Brett Creeley]

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

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

[Samudrala, Sridhar]

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.

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

[Jakub Kicinski]

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

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

[Samudrala, Sridhar]

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.

[PATCH net-next v2 4/4] bnxt_en: add support for tcp-data-split-thresh ethtool command

[Taehee Yoo]

The bnxt_en driver has configured the hds_threshold value automatically
when TPA is enabled based on the rx-copybreak default value.
Now the tcp-data-split-thresh ethtool command is added, so it adds an
implementation of tcp-data-split-thresh option.

Configuration of the tcp-data-split-thresh is allowed only when
the tcp-data-split is enabled. The default value of
tcp-data-split-thresh is 256, which is the default value of rx-copybreak,
which used to be the hds_thresh value.

   # Example:
   # 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

It enables tcp-data-split and sets tcp-data-split-thresh value to 256.

   # 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

Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

v2:
 - Patch added.

 drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 3 ++-
 drivers/net/ethernet/broadcom/bnxt/bnxt.h         | 2 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 9 +++++++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index f046478dfd2a..872b15842b11 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4455,6 +4455,7 @@ static void bnxt_init_ring_params(struct bnxt *bp)
 {
 	bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
 	bp->flags |= BNXT_FLAG_HDS;
+	bp->hds_threshold = BNXT_DEFAULT_RX_COPYBREAK;
 }
 
 /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
@@ -6429,7 +6430,7 @@ static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
 					  VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV6);
 		req->enables |=
 			cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_HDS_THRESHOLD_VALID);
-		req->hds_threshold = cpu_to_le16(bp->rx_copybreak);
+		req->hds_threshold = cpu_to_le16(bp->hds_threshold);
 	}
 	req->vnic_id = cpu_to_le32(vnic->fw_vnic_id);
 	return hwrm_req_send(bp, req);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 35601c71dfe9..48f390519c35 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2311,6 +2311,8 @@ struct bnxt {
 	int			rx_agg_nr_pages;
 	int			rx_nr_rings;
 	int			rsscos_nr_ctxs;
+#define BNXT_HDS_THRESHOLD_MAX	256
+	u16			hds_threshold;
 
 	u32			tx_ring_size;
 	u32			tx_ring_mask;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index ab64d7f94796..5b1f3047bf84 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -839,6 +839,8 @@ static void bnxt_get_ringparam(struct net_device *dev,
 	else
 		kernel_ering->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_DISABLED;
 
+	kernel_ering->tcp_data_split_thresh = bp->hds_threshold;
+
 	ering->tx_max_pending = BNXT_MAX_TX_DESC_CNT;
 
 	ering->rx_pending = bp->rx_ring_size;
@@ -864,6 +866,12 @@ static int bnxt_set_ringparam(struct net_device *dev,
 		return -EINVAL;
 	}
 
+	if (kernel_ering->tcp_data_split_thresh > BNXT_HDS_THRESHOLD_MAX) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "tcp-data-split-thresh size too big");
+		return -EINVAL;
+	}
+
 	if (netif_running(dev))
 		bnxt_close_nic(bp, false, false);
 
@@ -871,6 +879,7 @@ static int bnxt_set_ringparam(struct net_device *dev,
 	case ETHTOOL_TCP_DATA_SPLIT_UNKNOWN:
 	case ETHTOOL_TCP_DATA_SPLIT_ENABLED:
 		bp->flags |= BNXT_FLAG_HDS;
+		bp->hds_threshold = (u16)kernel_ering->tcp_data_split_thresh;
 		break;
 	case ETHTOOL_TCP_DATA_SPLIT_DISABLED:
 		bp->flags &= ~BNXT_FLAG_HDS;
-- 
2.34.1

Re: [PATCH net-next v2 4/4] bnxt_en: add support for tcp-data-split-thresh ethtool command

[Brett Creeley]

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 bnxt_en driver has configured the hds_threshold value automatically
> when TPA is enabled based on the rx-copybreak default value.
> Now the tcp-data-split-thresh ethtool command is added, so it adds an
> implementation of tcp-data-split-thresh option.
> 
> Configuration of the tcp-data-split-thresh is allowed only when
> the tcp-data-split is enabled. The default value of
> tcp-data-split-thresh is 256, which is the default value of rx-copybreak,
> which used to be the hds_thresh value.
> 
>     # Example:
>     # 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
> 
> It enables tcp-data-split and sets tcp-data-split-thresh value to 256.
> 
>     # 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
> 
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---
> 
> v2:
>   - Patch added.
> 
>   drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 3 ++-
>   drivers/net/ethernet/broadcom/bnxt/bnxt.h         | 2 ++
>   drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 9 +++++++++
>   3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index f046478dfd2a..872b15842b11 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -4455,6 +4455,7 @@ static void bnxt_init_ring_params(struct bnxt *bp)
>   {
>          bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
>          bp->flags |= BNXT_FLAG_HDS;
> +       bp->hds_threshold = BNXT_DEFAULT_RX_COPYBREAK;
>   }
> 
>   /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
> @@ -6429,7 +6430,7 @@ static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
>                                            VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV6);
>                  req->enables |=
>                          cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_HDS_THRESHOLD_VALID);
> -               req->hds_threshold = cpu_to_le16(bp->rx_copybreak);
> +               req->hds_threshold = cpu_to_le16(bp->hds_threshold);
>          }
>          req->vnic_id = cpu_to_le32(vnic->fw_vnic_id);
>          return hwrm_req_send(bp, req);
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> index 35601c71dfe9..48f390519c35 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> @@ -2311,6 +2311,8 @@ struct bnxt {
>          int                     rx_agg_nr_pages;
>          int                     rx_nr_rings;
>          int                     rsscos_nr_ctxs;
> +#define BNXT_HDS_THRESHOLD_MAX 256
> +       u16                     hds_threshold;
> 
>          u32                     tx_ring_size;
>          u32                     tx_ring_mask;
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index ab64d7f94796..5b1f3047bf84 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -839,6 +839,8 @@ static void bnxt_get_ringparam(struct net_device *dev,
>          else
>                  kernel_ering->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_DISABLED;
> 
> +       kernel_ering->tcp_data_split_thresh = bp->hds_threshold;
> +
>          ering->tx_max_pending = BNXT_MAX_TX_DESC_CNT;
> 
>          ering->rx_pending = bp->rx_ring_size;
> @@ -864,6 +866,12 @@ static int bnxt_set_ringparam(struct net_device *dev,
>                  return -EINVAL;
>          }
> 
> +       if (kernel_ering->tcp_data_split_thresh > BNXT_HDS_THRESHOLD_MAX) {
> +               NL_SET_ERR_MSG_MOD(extack,
> +                                  "tcp-data-split-thresh size too big");

Should you print the BNXT_HDS_THRESHOLD_MAX value here so the user knows 
the max size?

Actually, does it make more sense for ethtool get_ringparam to query the 
max threshold size from the driver and reject this in the core so all 
drivers don't have to have this same kind of check?

Thanks,

Brett

> +               return -EINVAL;
> +       }
> +
>          if (netif_running(dev))
>                  bnxt_close_nic(bp, false, false);
> 
> @@ -871,6 +879,7 @@ static int bnxt_set_ringparam(struct net_device *dev,
>          case ETHTOOL_TCP_DATA_SPLIT_UNKNOWN:
>          case ETHTOOL_TCP_DATA_SPLIT_ENABLED:
>                  bp->flags |= BNXT_FLAG_HDS;
> +               bp->hds_threshold = (u16)kernel_ering->tcp_data_split_thresh;
>                  break;
>          case ETHTOOL_TCP_DATA_SPLIT_DISABLED:
>                  bp->flags &= ~BNXT_FLAG_HDS;
> --
> 2.34.1
> 
> 

Re: [PATCH net-next v2 4/4] bnxt_en: add support for tcp-data-split-thresh ethtool command

[Taehee Yoo]

On Thu, Sep 12, 2024 at 12:52 AM Brett Creeley <bcreeley@amd.com> wrote:

Hi Brett,
Thank you so much 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 bnxt_en driver has configured the hds_threshold value automatically
> > when TPA is enabled based on the rx-copybreak default value.
> > Now the tcp-data-split-thresh ethtool command is added, so it adds an
> > implementation of tcp-data-split-thresh option.
> >
> > Configuration of the tcp-data-split-thresh is allowed only when
> > the tcp-data-split is enabled. The default value of
> > tcp-data-split-thresh is 256, which is the default value of rx-copybreak,
> > which used to be the hds_thresh value.
> >
> >     # Example:
> >     # 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
> >
> > It enables tcp-data-split and sets tcp-data-split-thresh value to 256.
> >
> >     # 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
> >
> > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > ---
> >
> > v2:
> >   - Patch added.
> >
> >   drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 3 ++-
> >   drivers/net/ethernet/broadcom/bnxt/bnxt.h         | 2 ++
> >   drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 9 +++++++++
> >   3 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index f046478dfd2a..872b15842b11 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -4455,6 +4455,7 @@ static void bnxt_init_ring_params(struct bnxt *bp)
> >   {
> >          bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
> >          bp->flags |= BNXT_FLAG_HDS;
> > +       bp->hds_threshold = BNXT_DEFAULT_RX_COPYBREAK;
> >   }
> >
> >   /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
> > @@ -6429,7 +6430,7 @@ static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
> >                                            VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV6);
> >                  req->enables |=
> >                          cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_HDS_THRESHOLD_VALID);
> > -               req->hds_threshold = cpu_to_le16(bp->rx_copybreak);
> > +               req->hds_threshold = cpu_to_le16(bp->hds_threshold);
> >          }
> >          req->vnic_id = cpu_to_le32(vnic->fw_vnic_id);
> >          return hwrm_req_send(bp, req);
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> > index 35601c71dfe9..48f390519c35 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> > @@ -2311,6 +2311,8 @@ struct bnxt {
> >          int                     rx_agg_nr_pages;
> >          int                     rx_nr_rings;
> >          int                     rsscos_nr_ctxs;
> > +#define BNXT_HDS_THRESHOLD_MAX 256
> > +       u16                     hds_threshold;
> >
> >          u32                     tx_ring_size;
> >          u32                     tx_ring_mask;
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > index ab64d7f94796..5b1f3047bf84 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > @@ -839,6 +839,8 @@ static void bnxt_get_ringparam(struct net_device *dev,
> >          else
> >                  kernel_ering->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_DISABLED;
> >
> > +       kernel_ering->tcp_data_split_thresh = bp->hds_threshold;
> > +
> >          ering->tx_max_pending = BNXT_MAX_TX_DESC_CNT;
> >
> >          ering->rx_pending = bp->rx_ring_size;
> > @@ -864,6 +866,12 @@ static int bnxt_set_ringparam(struct net_device *dev,
> >                  return -EINVAL;
> >          }
> >
> > +       if (kernel_ering->tcp_data_split_thresh > BNXT_HDS_THRESHOLD_MAX) {
> > +               NL_SET_ERR_MSG_MOD(extack,
> > +                                  "tcp-data-split-thresh size too big");
>
> Should you print the BNXT_HDS_THRESHOLD_MAX value here so the user knows
> the max size?

Okay, I will print BNXT_HDS_THRESHOLD_MAX value with extack message.

>
> Actually, does it make more sense for ethtool get_ringparam to query the
> max threshold size from the driver and reject this in the core so all
> drivers don't have to have this same kind of check?

Ah, I didn't consider this.
You mean that like ETHTOOL_A_RINGS_RX_MAX, right?
So, we can check precise information in userspace without error.

We can check tcp-data-split-threshold-max information like below.
ethtool -g enp13s0f0np0
Ring parameters for enp13s0f0np0:
Pre-set maximums:
RX: 2047
RX Mini: n/a
RX Jumbo: 8191
TX: 2047
TX push buff len: n/a
TCP data split thresh: 256 <-- here
Current hardware settings:
RX: 511
RX Mini: n/a
RX Jumbo: 2044
TX: 511
RX Buf Len: n/a
CQE Size: n/a
TX Push: off
RX Push: off
TX push buff len: n/a
TCP data split: on
TCP data split thresh: 0

I agree with this suggestion.
So, I will try to add ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH_MAX
option in the ethtool core unless there is no objection.

>
> Thanks,
>
> Brett
>
> > +               return -EINVAL;
> > +       }
> > +
> >          if (netif_running(dev))
> >                  bnxt_close_nic(bp, false, false);
> >
> > @@ -871,6 +879,7 @@ static int bnxt_set_ringparam(struct net_device *dev,
> >          case ETHTOOL_TCP_DATA_SPLIT_UNKNOWN:
> >          case ETHTOOL_TCP_DATA_SPLIT_ENABLED:
> >                  bp->flags |= BNXT_FLAG_HDS;
> > +               bp->hds_threshold = (u16)kernel_ering->tcp_data_split_thresh;
> >                  break;
> >          case ETHTOOL_TCP_DATA_SPLIT_DISABLED:
> >                  bp->flags &= ~BNXT_FLAG_HDS;
> > --
> > 2.34.1
> >
> >

Thanks a lot!
Taehee Yoo

Re: [PATCH net-next v2 4/4] bnxt_en: add support for tcp-data-split-thresh ethtool command

[Brett Creeley]

On 9/11/2024 9:32 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:52 AM Brett Creeley <bcreeley@amd.com> wrote:
> 
> Hi Brett,
> Thank you so much 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 bnxt_en driver has configured the hds_threshold value automatically
>>> when TPA is enabled based on the rx-copybreak default value.
>>> Now the tcp-data-split-thresh ethtool command is added, so it adds an
>>> implementation of tcp-data-split-thresh option.
>>>
>>> Configuration of the tcp-data-split-thresh is allowed only when
>>> the tcp-data-split is enabled. The default value of
>>> tcp-data-split-thresh is 256, which is the default value of rx-copybreak,
>>> which used to be the hds_thresh value.
>>>
>>>      # Example:
>>>      # 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
>>>
>>> It enables tcp-data-split and sets tcp-data-split-thresh value to 256.
>>>
>>>      # 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
>>>
>>> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
>>> ---
>>>
>>> v2:
>>>    - Patch added.
>>>
>>>    drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 3 ++-
>>>    drivers/net/ethernet/broadcom/bnxt/bnxt.h         | 2 ++
>>>    drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 9 +++++++++
>>>    3 files changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>> index f046478dfd2a..872b15842b11 100644
>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>> @@ -4455,6 +4455,7 @@ static void bnxt_init_ring_params(struct bnxt *bp)
>>>    {
>>>           bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
>>>           bp->flags |= BNXT_FLAG_HDS;
>>> +       bp->hds_threshold = BNXT_DEFAULT_RX_COPYBREAK;
>>>    }
>>>
>>>    /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
>>> @@ -6429,7 +6430,7 @@ static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
>>>                                             VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV6);
>>>                   req->enables |=
>>>                           cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_HDS_THRESHOLD_VALID);
>>> -               req->hds_threshold = cpu_to_le16(bp->rx_copybreak);
>>> +               req->hds_threshold = cpu_to_le16(bp->hds_threshold);
>>>           }
>>>           req->vnic_id = cpu_to_le32(vnic->fw_vnic_id);
>>>           return hwrm_req_send(bp, req);
>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>>> index 35601c71dfe9..48f390519c35 100644
>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>>> @@ -2311,6 +2311,8 @@ struct bnxt {
>>>           int                     rx_agg_nr_pages;
>>>           int                     rx_nr_rings;
>>>           int                     rsscos_nr_ctxs;
>>> +#define BNXT_HDS_THRESHOLD_MAX 256
>>> +       u16                     hds_threshold;
>>>
>>>           u32                     tx_ring_size;
>>>           u32                     tx_ring_mask;
>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
>>> index ab64d7f94796..5b1f3047bf84 100644
>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
>>> @@ -839,6 +839,8 @@ static void bnxt_get_ringparam(struct net_device *dev,
>>>           else
>>>                   kernel_ering->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_DISABLED;
>>>
>>> +       kernel_ering->tcp_data_split_thresh = bp->hds_threshold;
>>> +
>>>           ering->tx_max_pending = BNXT_MAX_TX_DESC_CNT;
>>>
>>>           ering->rx_pending = bp->rx_ring_size;
>>> @@ -864,6 +866,12 @@ static int bnxt_set_ringparam(struct net_device *dev,
>>>                   return -EINVAL;
>>>           }
>>>
>>> +       if (kernel_ering->tcp_data_split_thresh > BNXT_HDS_THRESHOLD_MAX) {
>>> +               NL_SET_ERR_MSG_MOD(extack,
>>> +                                  "tcp-data-split-thresh size too big");
>>
>> Should you print the BNXT_HDS_THRESHOLD_MAX value here so the user knows
>> the max size?
> 
> Okay, I will print BNXT_HDS_THRESHOLD_MAX value with extack message.
> 
>>
>> Actually, does it make more sense for ethtool get_ringparam to query the
>> max threshold size from the driver and reject this in the core so all
>> drivers don't have to have this same kind of check?
> 
> Ah, I didn't consider this.
> You mean that like ETHTOOL_A_RINGS_RX_MAX, right?
> So, we can check precise information in userspace without error.
> 
> We can check tcp-data-split-threshold-max information like below.
> ethtool -g enp13s0f0np0
> Ring parameters for enp13s0f0np0:
> Pre-set maximums:
> RX: 2047
> RX Mini: n/a
> RX Jumbo: 8191
> TX: 2047
> TX push buff len: n/a
> TCP data split thresh: 256 <-- here
> Current hardware settings:
> RX: 511
> RX Mini: n/a
> RX Jumbo: 2044
> TX: 511
> RX Buf Len: n/a
> CQE Size: n/a
> TX Push: off
> RX Push: off
> TX push buff len: n/a
> TCP data split: on
> TCP data split thresh: 0
> 
> I agree with this suggestion.
> So, I will try to add ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH_MAX
> option in the ethtool core unless there is no objection.

Yeah, I think this makes the most sense so all/any vendor drivers 
implementing tcp-data-split-thresh don't have to worry about checking 
against their max in their set_ringparam callback, they just have to set 
their max in the get_ringparam callback.

You mentioned ETHTOOL_A_RINGS_RX_MAX, which there are already examples 
of this in the kernel and other similar ring parameter max values.

Thanks,

Brett

> 
>>
>> Thanks,
>>
>> Brett
>>
>>> +               return -EINVAL;
>>> +       }
>>> +
>>>           if (netif_running(dev))
>>>                   bnxt_close_nic(bp, false, false);
>>>
>>> @@ -871,6 +879,7 @@ static int bnxt_set_ringparam(struct net_device *dev,
>>>           case ETHTOOL_TCP_DATA_SPLIT_UNKNOWN:
>>>           case ETHTOOL_TCP_DATA_SPLIT_ENABLED:
>>>                   bp->flags |= BNXT_FLAG_HDS;
>>> +               bp->hds_threshold = (u16)kernel_ering->tcp_data_split_thresh;
>>>                   break;
>>>           case ETHTOOL_TCP_DATA_SPLIT_DISABLED:
>>>                   bp->flags &= ~BNXT_FLAG_HDS;
>>> --
>>> 2.34.1
>>>
>>>
> 
> Thanks a lot!
> Taehee Yoo