| Message ID | 20241003160620.1521626-3-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 Thu, 3 Oct 2024 16:06:15 +0000 Taehee Yoo wrote: > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > index fdecdf8894b3..e9ef65dd2e7b 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; This breaks previous behavior. The HDS reporting from get was introduced to signal to user space whether the page flip based TCP zero-copy (the one added some years ago not the recent one) will be usable with this NIC. When HW-GRO is enabled HDS will be working. I think that the driver should only track if the user has set the value to ENABLED (forced HDS), or to UKNOWN (driver default). Setting the HDS to disabled is not useful, don't support it. > 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; > + } Technically just if the XDP does not support multi-buffer. Any chance we could do this check in the core?
On Wed, Oct 9, 2024 at 3:19 AM Jakub Kicinski <kuba@kernel.org> wrote: > Hi Jakub, Thanks a lot for your reviews! > On Thu, 3 Oct 2024 16:06:15 +0000 Taehee Yoo wrote: > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > index fdecdf8894b3..e9ef65dd2e7b 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; > > This breaks previous behavior. The HDS reporting from get was > introduced to signal to user space whether the page flip based > TCP zero-copy (the one added some years ago not the recent one) > will be usable with this NIC. > > When HW-GRO is enabled HDS will be working. > > I think that the driver should only track if the user has set the value > to ENABLED (forced HDS), or to UKNOWN (driver default). Setting the HDS > to disabled is not useful, don't support it. Okay, I will remove the disable feature in a v4 patch. Before this patch, hds_threshold was rx-copybreak value. How do you think hds_threshold should still follow rx-copybreak value if it is UNKNOWN mode? I think hds_threshold need to follow new tcp-data-split-thresh value in ENABLE/UNKNOWN and make rx-copybreak pure software feature. But if so, it changes the default behavior. How do you think about it? > > > 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; > > + } > > Technically just if the XDP does not support multi-buffer. > Any chance we could do this check in the core? I think we can access xdp_rxq_info with netdev_rx_queue structure. However, xdp_rxq_info is not sufficient to distinguish mb is supported by the driver or not. I think prog->aux->xdp_has_frags is required to distinguish it correctly. So, I think we need something more. Do you have any idea?
On Wed, 9 Oct 2024 22:54:17 +0900 Taehee Yoo wrote: > > This breaks previous behavior. The HDS reporting from get was > > introduced to signal to user space whether the page flip based > > TCP zero-copy (the one added some years ago not the recent one) > > will be usable with this NIC. > > > > When HW-GRO is enabled HDS will be working. > > > > I think that the driver should only track if the user has set the value > > to ENABLED (forced HDS), or to UKNOWN (driver default). Setting the HDS > > to disabled is not useful, don't support it. > > Okay, I will remove the disable feature in a v4 patch. > Before this patch, hds_threshold was rx-copybreak value. > How do you think hds_threshold should still follow rx-copybreak value > if it is UNKNOWN mode? IIUC the rx_copybreak only applies to the header? Or does it apply to the entire frame? If rx_copybreak applies to the entire frame and not just the first buffer (headers or headers+payload if not split) - no preference. If rx_copybreak only applies to the headers / first buffer then I'd keep them separate as they operate on a different length. > I think hds_threshold need to follow new tcp-data-split-thresh value in > ENABLE/UNKNOWN and make rx-copybreak pure software feature. Sounds good to me, but just to be clear: If user sets the HDS enable to UNKNOWN (or doesn't set it): - GET returns (current behavior, AFAIU): - DISABLED (if HW-GRO is disabled and MTU is not Jumbo) - ENABLED (if HW-GRO is enabled of MTU is Jumbo) If user sets the HDS enable to ENABLED (force HDS on): - GET returns ENABLED hds_threshold returns: some value, but it's only actually used if GET returns ENABLED. > But if so, it changes the default behavior. How so? The configuration of neither of those two is exposed to the user. We can keep the same defaults, until user overrides them. > How do you think about it? > > > > > > 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; > > > + } > > > > Technically just if the XDP does not support multi-buffer. > > Any chance we could do this check in the core? > > I think we can access xdp_rxq_info with netdev_rx_queue structure. > However, xdp_rxq_info is not sufficient to distinguish mb is supported > by the driver or not. I think prog->aux->xdp_has_frags is required to > distinguish it correctly. > So, I think we need something more. > Do you have any idea? Take a look at dev_xdp_prog_count(), something like that but only counting non-mb progs?
On Thu, Oct 10, 2024 at 12:28 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 9 Oct 2024 22:54:17 +0900 Taehee Yoo wrote: > > > This breaks previous behavior. The HDS reporting from get was > > > introduced to signal to user space whether the page flip based > > > TCP zero-copy (the one added some years ago not the recent one) > > > will be usable with this NIC. > > > > > > When HW-GRO is enabled HDS will be working. > > > > > > I think that the driver should only track if the user has set the value > > > to ENABLED (forced HDS), or to UKNOWN (driver default). Setting the HDS > > > to disabled is not useful, don't support it. > > > > Okay, I will remove the disable feature in a v4 patch. > > Before this patch, hds_threshold was rx-copybreak value. > > How do you think hds_threshold should still follow rx-copybreak value > > if it is UNKNOWN mode? > > IIUC the rx_copybreak only applies to the header? Or does it apply > to the entire frame? > > If rx_copybreak applies to the entire frame and not just the first > buffer (headers or headers+payload if not split) - no preference. > If rx_copybreak only applies to the headers / first buffer then > I'd keep them separate as they operate on a different length. It applies only the first buffer. So, if HDS is enabled, it copies only header. Thanks, I will separate rx-copybreak and hds_threshold. > > > I think hds_threshold need to follow new tcp-data-split-thresh value in > > ENABLE/UNKNOWN and make rx-copybreak pure software feature. > > Sounds good to me, but just to be clear: > > If user sets the HDS enable to UNKNOWN (or doesn't set it): > - GET returns (current behavior, AFAIU): > - DISABLED (if HW-GRO is disabled and MTU is not Jumbo) > - ENABLED (if HW-GRO is enabled of MTU is Jumbo) > If user sets the HDS enable to ENABLED (force HDS on): > - GET returns ENABLED > > hds_threshold returns: some value, but it's only actually used if GET > returns ENABLED. > Thanks for the detailed explanation! > > But if so, it changes the default behavior. > > How so? The configuration of neither of those two is exposed to > the user. We can keep the same defaults, until user overrides them. > Ah, right. I understood. > > How do you think about it? > > > > > > > > > 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; > > > > + } > > > > > > Technically just if the XDP does not support multi-buffer. > > > Any chance we could do this check in the core? > > > > I think we can access xdp_rxq_info with netdev_rx_queue structure. > > However, xdp_rxq_info is not sufficient to distinguish mb is supported > > by the driver or not. I think prog->aux->xdp_has_frags is required to > > distinguish it correctly. > > So, I think we need something more. > > Do you have any idea? > > Take a look at dev_xdp_prog_count(), something like that but only > counting non-mb progs? Thanks for very nice example, I will try it!
On Thu, Oct 10, 2024 at 12:28 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 9 Oct 2024 22:54:17 +0900 Taehee Yoo wrote: > > > This breaks previous behavior. The HDS reporting from get was > > > introduced to signal to user space whether the page flip based > > > TCP zero-copy (the one added some years ago not the recent one) > > > will be usable with this NIC. > > > > > > When HW-GRO is enabled HDS will be working. > > > > > > I think that the driver should only track if the user has set the value > > > to ENABLED (forced HDS), or to UKNOWN (driver default). Setting the HDS > > > to disabled is not useful, don't support it. > > > > Okay, I will remove the disable feature in a v4 patch. > > Before this patch, hds_threshold was rx-copybreak value. > > How do you think hds_threshold should still follow rx-copybreak value > > if it is UNKNOWN mode? > > IIUC the rx_copybreak only applies to the header? Or does it apply > to the entire frame? > > If rx_copybreak applies to the entire frame and not just the first > buffer (headers or headers+payload if not split) - no preference. > If rx_copybreak only applies to the headers / first buffer then > I'd keep them separate as they operate on a different length. > > > I think hds_threshold need to follow new tcp-data-split-thresh value in > > ENABLE/UNKNOWN and make rx-copybreak pure software feature. > > Sounds good to me, but just to be clear: > > If user sets the HDS enable to UNKNOWN (or doesn't set it): > - GET returns (current behavior, AFAIU): > - DISABLED (if HW-GRO is disabled and MTU is not Jumbo) > - ENABLED (if HW-GRO is enabled of MTU is Jumbo) > If user sets the HDS enable to ENABLED (force HDS on): > - GET returns ENABLED While I'm writing a patch I face an ambiguous problem here. ethnl_set_ring() first calls .get_ringparam() to get current config. Then it calls .set_ringparam() after it sets the current config + new config to param structures. The bnxt_set_ringparam() may receive ETHTOOL_TCP_DATA_SPLIT_ENABLED because two cases. 1. from user 2. from bnxt_get_ringparam() because of UNKNWON. The problem is that the bnxt_set_ringparam() can't distinguish them. The problem scenario is here. 1. tcp-data-split is UNKNOWN mode. 2. HDS is automatically enabled because one of LRO or GRO is enabled. 3. user changes ring parameter with following command `ethtool -G eth0 rx 1024` 4. ethnl_set_rings() calls .get_ringparam() to get current config. 5. bnxt_get_ringparam() returns ENABLE of HDS because of UNKNWON mode. 6. ethnl_set_rings() calls .set_ringparam() after setting param with configs comes from .get_ringparam(). 7. bnxt_set_ringparam() is passed ETHTOOL_TCP_DATA_SPLIT_ENABLED but the user didn't set it explicitly. 8. bnxt_set_ringparam() eventually force enables tcp-data-split. I couldn't find a way to distinguish them so far. I'm not sure if this is acceptable or not. Maybe we need to modify a scenario? > > hds_threshold returns: some value, but it's only actually used if GET > returns ENABLED. > > > But if so, it changes the default behavior. > > How so? The configuration of neither of those two is exposed to > the user. We can keep the same defaults, until user overrides them. > > > How do you think about it? > > > > > > > > > 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; > > > > + } > > > > > > Technically just if the XDP does not support multi-buffer. > > > Any chance we could do this check in the core? > > > > I think we can access xdp_rxq_info with netdev_rx_queue structure. > > However, xdp_rxq_info is not sufficient to distinguish mb is supported > > by the driver or not. I think prog->aux->xdp_has_frags is required to > > distinguish it correctly. > > So, I think we need something more. > > Do you have any idea? > > Take a look at dev_xdp_prog_count(), something like that but only > counting non-mb progs?
On Fri, 1 Nov 2024 02:34:59 +0900 Taehee Yoo wrote: > While I'm writing a patch I face an ambiguous problem here. > ethnl_set_ring() first calls .get_ringparam() to get current config. > Then it calls .set_ringparam() after it sets the current config + new > config to param structures. > The bnxt_set_ringparam() may receive ETHTOOL_TCP_DATA_SPLIT_ENABLED > because two cases. > 1. from user > 2. from bnxt_get_ringparam() because of UNKNWON. > The problem is that the bnxt_set_ringparam() can't distinguish them. > The problem scenario is here. > 1. tcp-data-split is UNKNOWN mode. > 2. HDS is automatically enabled because one of LRO or GRO is enabled. > 3. user changes ring parameter with following command > `ethtool -G eth0 rx 1024` > 4. ethnl_set_rings() calls .get_ringparam() to get current config. > 5. bnxt_get_ringparam() returns ENABLE of HDS because of UNKNWON mode. > 6. ethnl_set_rings() calls .set_ringparam() after setting param with > configs comes from .get_ringparam(). > 7. bnxt_set_ringparam() is passed ETHTOOL_TCP_DATA_SPLIT_ENABLED but > the user didn't set it explicitly. > 8. bnxt_set_ringparam() eventually force enables tcp-data-split. > > I couldn't find a way to distinguish them so far. > I'm not sure if this is acceptable or not. > Maybe we need to modify a scenario? I thought we discussed this, but I may be misremembering. You may need to record in the core whether the setting came from the user or not (similarly to IFF_RXFH_CONFIGURED). User setting UNKNWON would mean "reset". Maybe I'm misunderstanding..
On Fri, Nov 1, 2024 at 8:56 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 1 Nov 2024 02:34:59 +0900 Taehee Yoo wrote: > > While I'm writing a patch I face an ambiguous problem here. > > ethnl_set_ring() first calls .get_ringparam() to get current config. > > Then it calls .set_ringparam() after it sets the current config + new > > config to param structures. > > The bnxt_set_ringparam() may receive ETHTOOL_TCP_DATA_SPLIT_ENABLED > > because two cases. > > 1. from user > > 2. from bnxt_get_ringparam() because of UNKNWON. > > The problem is that the bnxt_set_ringparam() can't distinguish them. > > The problem scenario is here. > > 1. tcp-data-split is UNKNOWN mode. > > 2. HDS is automatically enabled because one of LRO or GRO is enabled. > > 3. user changes ring parameter with following command > > `ethtool -G eth0 rx 1024` > > 4. ethnl_set_rings() calls .get_ringparam() to get current config. > > 5. bnxt_get_ringparam() returns ENABLE of HDS because of UNKNWON mode. > > 6. ethnl_set_rings() calls .set_ringparam() after setting param with > > configs comes from .get_ringparam(). > > 7. bnxt_set_ringparam() is passed ETHTOOL_TCP_DATA_SPLIT_ENABLED but > > the user didn't set it explicitly. > > 8. bnxt_set_ringparam() eventually force enables tcp-data-split. > > > > I couldn't find a way to distinguish them so far. > > I'm not sure if this is acceptable or not. > > Maybe we need to modify a scenario? > > I thought we discussed this, but I may be misremembering. > You may need to record in the core whether the setting came > from the user or not (similarly to IFF_RXFH_CONFIGURED). > User setting UNKNWON would mean "reset". > Maybe I'm misunderstanding.. Thanks a lot for that! I will try to add a new variable, that indicates tcp-data-split is set by user. It would be the tcp_data_split_mod in the kernel_ethtool_ringparam structure. Thanks a lot! Taehee Yoo
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 fdecdf8894b3..e9ef65dd2e7b 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); @@ -5346,6 +5366,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,
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> --- v3: - No changes. 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(-)