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@xxxxxxx> 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@xxxxxxxxx>
---
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
