| Message ID | 20241003160620.1521626-5-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bnxt_en: implement device memory TCP for bnxt | expand |
On 10/3/2024 9:06 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: > ... > TCP data split thresh: 256 > 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: > ... > TCP data split thresh: 256 > Current hardware settings: > ... > TCP data split: off > TCP data split thresh: n/a > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > --- > > v3: > - Drop validation logic tcp-data-split and tcp-data-split-thresh. > > 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 | 4 ++++ > 3 files changed, 8 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; Putting this here creates a 2 byte hole right after hds_threshold and also puts a 4 byte hole after cp_nr_rings. Since hds_threshold doesn't seem to be used in the hotpath maybe it would be best to fill a pre-existing hole in struct bnxt to put it? Thanks, Brett > > 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 e9ef65dd2e7b..af6ed492f688 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > @@ -839,6 +839,9 @@ 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; > + kernel_ering->tcp_data_split_thresh_max = BNXT_HDS_THRESHOLD_MAX; > + > ering->tx_max_pending = BNXT_MAX_TX_DESC_CNT; > > ering->rx_pending = bp->rx_ring_size; > @@ -871,6 +874,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 >
On Fri, Oct 4, 2024 at 3:13 AM Brett Creeley <bcreeley@amd.com> wrote: > > > > On 10/3/2024 9:06 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: > > ... > > TCP data split thresh: 256 > > 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: > > ... > > TCP data split thresh: 256 > > Current hardware settings: > > ... > > TCP data split: off > > TCP data split thresh: n/a > > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > --- > > > > v3: > > - Drop validation logic tcp-data-split and tcp-data-split-thresh. > > > > 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 | 4 ++++ > > 3 files changed, 8 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; > > Putting this here creates a 2 byte hole right after hds_threshold and > also puts a 4 byte hole after cp_nr_rings. > > Since hds_threshold doesn't seem to be used in the hotpath maybe it > would be best to fill a pre-existing hole in struct bnxt to put it? > Yes, hds_threshold makes an additional 2bytes hole. I checked pre-existing holes in struct bnxt, and almost all members are sorted by purpose. However, I think under num_tc would be a pretty good place for hds_threshold. Before: /* size: 7000, cachelines: 110, members: 185 */ /* sum members: 6931, holes: 21, sum holes: 64 */ /* sum bitfield members: 2 bits, bit holes: 1, sum bit holes: 6 bits */ /* padding: 4 */ /* paddings: 7, sum paddings: 25 */ /* last cacheline: 24 bytes */ After: /* size: 6992, cachelines: 110, members: 185 */ /* sum members: 6931, holes: 19, sum holes: 56 */ /* sum bitfield members: 2 bits, bit holes: 1, sum bit holes: 6 bits */ /* padding: 4 */ /* paddings: 7, sum paddings: 25 */ /* last cacheline: 16 bytes */ So, I would like to change it in a v4 patch if there are no objections. Thanks a lot! Taehee Yoo > Thanks, > > Brett > > > > > 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 e9ef65dd2e7b..af6ed492f688 100644 > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > @@ -839,6 +839,9 @@ 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; > > + kernel_ering->tcp_data_split_thresh_max = BNXT_HDS_THRESHOLD_MAX; > > + > > ering->tx_max_pending = BNXT_MAX_TX_DESC_CNT; > > > > ering->rx_pending = bp->rx_ring_size; > > @@ -871,6 +874,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 > >
On Thu, 3 Oct 2024 16:06:17 +0000 Taehee Yoo wrote: > +#define BNXT_HDS_THRESHOLD_MAX 256 > + u16 hds_threshold; From the cover letter it sounded like the max is 1023. Did I misread that ?
On Wed, Oct 9, 2024 at 3:35 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 3 Oct 2024 16:06:17 +0000 Taehee Yoo wrote: > > +#define BNXT_HDS_THRESHOLD_MAX 256 > > + u16 hds_threshold; > > From the cover letter it sounded like the max is 1023. > Did I misread that ? Based on my test, the maximum value seems to be 1023. But I'm not sure that all NICs, that use bnxt_en driver support 1023 value. (At least all NICs I have support 1023). I decided 256 as the maximum value, that was default value so all NICs can use it safely. If Broadcom Engineer confirms 1023 is right for all NICs, I would like to change it.
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 e9ef65dd2e7b..af6ed492f688 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -839,6 +839,9 @@ 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; + kernel_ering->tcp_data_split_thresh_max = BNXT_HDS_THRESHOLD_MAX; + ering->tx_max_pending = BNXT_MAX_TX_DESC_CNT; ering->rx_pending = bp->rx_ring_size; @@ -871,6 +874,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;
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: ... TCP data split thresh: 256 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: ... TCP data split thresh: 256 Current hardware settings: ... TCP data split: off TCP data split thresh: n/a Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- v3: - Drop validation logic tcp-data-split and tcp-data-split-thresh. 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 | 4 ++++ 3 files changed, 8 insertions(+), 1 deletion(-)