* [PATCH net-next 0/2] bnxt_en: implement tcp-data-split ethtool command
@ 2024-09-06 8:07 Taehee Yoo
2024-09-06 8:07 ` [PATCH net-next 1/2] bnxt_en: add support for rx-copybreak " Taehee Yoo
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Taehee Yoo @ 2024-09-06 8:07 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, michael.chan, netdev; +Cc: ap420073
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.
To implement, it requires decoupling rx-copybreak and hds_threshold
value.
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 approach of this patch is to support the bnxt_en driver setting up
enable/disable HDS explicitly, not rely on LRO/GRO, JUMBO.
In addition, hds_threshold no longer follows rx-copybreak.
By this patch, hds_threshold always be 0.
Taehee Yoo (2):
bnxt_en: add support for rx-copybreak ethtool command
bnxt_en: add support for tcp-data-split ethtool command
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 30 ++++----
drivers/net/ethernet/broadcom/bnxt/bnxt.h | 10 ++-
.../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 70 ++++++++++++++++++-
3 files changed, 91 insertions(+), 19 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net-next 1/2] bnxt_en: add support for rx-copybreak ethtool command
2024-09-06 8:07 [PATCH net-next 0/2] bnxt_en: implement tcp-data-split ethtool command Taehee Yoo
@ 2024-09-06 8:07 ` Taehee Yoo
2024-09-06 8:07 ` [PATCH net-next 2/2] bnxt_en: add support for tcp-data-split " Taehee Yoo
2024-09-07 1:38 ` [PATCH net-next 0/2] bnxt_en: implement " Jakub Kicinski
2 siblings, 0 replies; 5+ messages in thread
From: Taehee Yoo @ 2024-09-06 8:07 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, michael.chan, netdev; +Cc: ap420073
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>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 23 ++++++----
drivers/net/ethernet/broadcom/bnxt/bnxt.h | 5 ++-
.../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 45 ++++++++++++++++++-
3 files changed, 62 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index c9248ed9330c..e1a4beece582 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);
@@ -2162,7 +2161,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 +4450,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 +4469,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 +4513,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 +6428,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);
@@ -15847,6 +15851,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 3b805ed433ed..ad95d0ede5d8 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -34,6 +34,9 @@
#include <linux/firmware/broadcom/tee_bnxt_fw.h>
#endif
+#define BNXT_DEFAULT_RX_COPYBREAK 256
+#define BNXT_MIN_RX_COPYBREAK 64
+
extern struct list_head bnxt_block_cb_list;
struct page_pool;
@@ -2296,7 +2299,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 7392a716f28d..052b53937757 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -4318,6 +4318,47 @@ 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)
+ return -EINVAL;
+ if (rx_copybreak != bp->rx_copybreak) {
+ bp->rx_copybreak = rx_copybreak;
+ if (netif_running(dev)) {
+ bnxt_close_nic(bp, false, false);
+ 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,
@@ -4768,7 +4809,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;
@@ -5344,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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH net-next 2/2] bnxt_en: add support for tcp-data-split ethtool command
2024-09-06 8:07 [PATCH net-next 0/2] bnxt_en: implement tcp-data-split ethtool command Taehee Yoo
2024-09-06 8:07 ` [PATCH net-next 1/2] bnxt_en: add support for rx-copybreak " Taehee Yoo
@ 2024-09-06 8:07 ` Taehee Yoo
2024-09-07 1:38 ` [PATCH net-next 0/2] bnxt_en: implement " Jakub Kicinski
2 siblings, 0 replies; 5+ messages in thread
From: Taehee Yoo @ 2024-09-06 8:07 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, michael.chan, netdev; +Cc: ap420073
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>.
By this change, hds_threshold will always be 0 if tcp-data-split is
enabled until we provide some option to set/get tcp-data-split-threshold
option someday And hds_threshold doesn't follow rx-copybreak value
anymore.
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>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 11 ++++----
drivers/net/ethernet/broadcom/bnxt/bnxt.h | 5 ++--
.../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 25 +++++++++++++++++--
3 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index e1a4beece582..d3641616e313 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4453,6 +4453,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
@@ -4473,7 +4474,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;
@@ -6420,16 +6421,14 @@ 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->hds_threshold = 0;
}
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 ad95d0ede5d8..382a87a80bb4 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2198,8 +2198,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
@@ -2220,6 +2218,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 052b53937757..f619a0bc3a6a 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 0/2] bnxt_en: implement tcp-data-split ethtool command
2024-09-06 8:07 [PATCH net-next 0/2] bnxt_en: implement tcp-data-split ethtool command Taehee Yoo
2024-09-06 8:07 ` [PATCH net-next 1/2] bnxt_en: add support for rx-copybreak " Taehee Yoo
2024-09-06 8:07 ` [PATCH net-next 2/2] bnxt_en: add support for tcp-data-split " Taehee Yoo
@ 2024-09-07 1:38 ` Jakub Kicinski
2024-09-07 17:06 ` Taehee Yoo
2 siblings, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2024-09-07 1:38 UTC (permalink / raw)
To: Taehee Yoo; +Cc: davem, pabeni, edumazet, michael.chan, netdev
On Fri, 6 Sep 2024 08:07:48 +0000 Taehee Yoo wrote:
> The approach of this patch is to support the bnxt_en driver setting up
> enable/disable HDS explicitly, not rely on LRO/GRO, JUMBO.
> In addition, hds_threshold no longer follows rx-copybreak.
> By this patch, hds_threshold always be 0.
That may make sense for zero-copy use cases, where you want to make
sure that all of the data lands in target page pool. But in general
using the data buffers may waste quite a bit of memory, and PCIe bus
bandwidth (two small transfers instead of one medium size).
I think we should add a user-controlled setting in ethtool -g for
hds-threshold.
Also please make sure you describe the level of testing you have done
in the commit message. I remember discussing this a few years back
and at that time HDS was tied to GRO for bnxt at the FW level.
A lot has changed since but please describe what you tested..
--
pw-bot: cr
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 0/2] bnxt_en: implement tcp-data-split ethtool command
2024-09-07 1:38 ` [PATCH net-next 0/2] bnxt_en: implement " Jakub Kicinski
@ 2024-09-07 17:06 ` Taehee Yoo
0 siblings, 0 replies; 5+ messages in thread
From: Taehee Yoo @ 2024-09-07 17:06 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, pabeni, edumazet, michael.chan, netdev
On Sat, Sep 7, 2024 at 10:38 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
Hi Jakub,
Thanks a lot for your review!
> On Fri, 6 Sep 2024 08:07:48 +0000 Taehee Yoo wrote:
> > The approach of this patch is to support the bnxt_en driver setting up
> > enable/disable HDS explicitly, not rely on LRO/GRO, JUMBO.
> > In addition, hds_threshold no longer follows rx-copybreak.
> > By this patch, hds_threshold always be 0.
>
> That may make sense for zero-copy use cases, where you want to make
> sure that all of the data lands in target page pool. But in general
> using the data buffers may waste quite a bit of memory, and PCIe bus
> bandwidth (two small transfers instead of one medium size).
>
> I think we should add a user-controlled setting in ethtool -g for
> hds-threshold.
Thanks, I understand your concern.
So, I will implement the tcp-data-split-threshold option in the v2 patch.
>
> Also please make sure you describe the level of testing you have done
> in the commit message. I remember discussing this a few years back
> and at that time HDS was tied to GRO for bnxt at the FW level.
> A lot has changed since but please describe what you tested..
Sorry about the lack of describing how I tested this patch.
I'm using BCM57412 and the latest firmware(230.0.157.0/pkg 230.1.116.0).
I tested if the HDS had any dependencies such as TPA (HW-GRO, LRO) or jumbo.
When I tested it I checked out skb->data size and skb->data_len size.
And HDS and TPA were worked independently.
HDS disabled + TPA disabled:
It receives normal packets.
`ethtool -S <interface name> | grep tpa` doesn't show an increment of
tpa statistics.
HDS disabled + TPA enabled:
It receives gro packets.
`ethtool -S <interface name> | grep tpa` shows an increment of tpa statistics.
HDS enabled + TPA disabled:
It receives header-data split packets.
`ethtool -S <interface name> | grep tpa` doesn't show an increment of
tpa statistics.
HDS enabled + TPA enabled:
It receives header-data split gro packets.
`ethtool -S <interface name> | grep tpa` shows an increment of tpa statistics.
I tested the above cases and they worked expectedly.
But I tested again after your review, I found a bug that sometimes
couldn't reset jumbo_thresh properly.
I will fix that bug too in the v2 patch.
So, the v2 patch will contain the following.
1. fix jumbo_thresh reset logic.
2. add a description of how I tested this patch.
3. implement `ethtool -G <interface name> tcp-data-split-threshold
<value> option.
If you think the above description is still not enough, please let me know!
Thanks a lot!
Taehee Yoo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-09-07 17:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-06 8:07 [PATCH net-next 0/2] bnxt_en: implement tcp-data-split ethtool command Taehee Yoo
2024-09-06 8:07 ` [PATCH net-next 1/2] bnxt_en: add support for rx-copybreak " Taehee Yoo
2024-09-06 8:07 ` [PATCH net-next 2/2] bnxt_en: add support for tcp-data-split " Taehee Yoo
2024-09-07 1:38 ` [PATCH net-next 0/2] bnxt_en: implement " Jakub Kicinski
2024-09-07 17:06 ` Taehee Yoo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).