| Message ID | 20241022162359.2713094-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 Tue, Oct 22, 2024 at 9:24 AM Taehee Yoo <ap420073@gmail.com> wrote: > > 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. > > Tested-by: Stanislav Fomichev <sdf@fomichev.me> > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > --- > > v4: > - Do not support disable tcp-data-split. > - Add Test tag from Stanislav. > > v3: > - No changes. > > v2: > - Do not set hds_threshold to 0. > > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 +++----- > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 5 +++-- > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 13 +++++++++++++ > 3 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > index 0f5fe9ba691d..91ea42ff9b17 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > @@ -6420,15 +6420,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); Please explain why this "if" condition is removed. BNXT_RX_PAGE_MODE() means that we are in XDP mode and we currently don't support HDS in XDP mode. Added Andy Gospo to CC so he can also comment. > - } else { > + if (bp->flags & BNXT_FLAG_AGG_RINGS) { > 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);
On Fri, Oct 25, 2024 at 2:02 PM Michael Chan Hi Michael, Thank you so much for the review! <michael.chan@broadcom.com> wrote: > > On Tue, Oct 22, 2024 at 9:24 AM Taehee Yoo <ap420073@gmail.com> wrote: > > > > 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. > > > > Tested-by: Stanislav Fomichev <sdf@fomichev.me> > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > --- > > > > v4: > > - Do not support disable tcp-data-split. > > - Add Test tag from Stanislav. > > > > v3: > > - No changes. > > > > v2: > > - Do not set hds_threshold to 0. > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 +++----- > > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 5 +++-- > > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 13 +++++++++++++ > > 3 files changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > index 0f5fe9ba691d..91ea42ff9b17 100644 > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > @@ -6420,15 +6420,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); > > Please explain why this "if" condition is removed. > BNXT_RX_PAGE_MODE() means that we are in XDP mode and we currently > don't support HDS in XDP mode. Added Andy Gospo to CC so he can also > comment. Yes, The reason why the "if" condition is removed is to make rx-copybreak a pure software feature. The current jumbo_thresh follows the rx-copybreak value, however, I thought the rx-copybreak value should not affect any hardware function. So, I thought following rx_buf_use_size instead of rx_copybreak is okay. By this change, jumbo_thresh always follows rx_buf_use_size, so I removed the "if" condition. Oh, on second thought, it changes a default behavior, it's not my intention. What value would be good for jumbo_thresh following? What do you think? > > > - } else { > > + if (bp->flags & BNXT_FLAG_AGG_RINGS) { > > 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); Thank you so much for the review! Taehee Yoo
On Thu, Oct 24, 2024 at 10:02:30PM -0700, Michael Chan wrote: > On Tue, Oct 22, 2024 at 9:24 AM Taehee Yoo <ap420073@gmail.com> wrote: > > > > 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. > > > > Tested-by: Stanislav Fomichev <sdf@fomichev.me> > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > --- > > > > v4: > > - Do not support disable tcp-data-split. > > - Add Test tag from Stanislav. > > > > v3: > > - No changes. > > > > v2: > > - Do not set hds_threshold to 0. > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 +++----- > > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 5 +++-- > > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 13 +++++++++++++ > > 3 files changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > index 0f5fe9ba691d..91ea42ff9b17 100644 > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > @@ -6420,15 +6420,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); > > Please explain why this "if" condition is removed. > BNXT_RX_PAGE_MODE() means that we are in XDP mode and we currently > don't support HDS in XDP mode. Added Andy Gospo to CC so he can also > comment. > In bnxt_set_rx_skb_mode we set BNXT_FLAG_RX_PAGE_MODE and clear BNXT_FLAG_AGG_RINGS, so this should work. The only issue is that we have spots in the driver where we check BNXT_RX_PAGE_MODE(bp) to indicate that XDP single-buffer mode is enabled on the device. If you need to respin this series I would prefer that the change is like below to key off the page mode being disabled and BNXT_FLAG_AGG_RINGS being enabled to setup HDS. This will serve as a reminder that this is for XDP. @@ -6418,15 +6418,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 (!BNXT_RX_PAGE_MODE(bp) && (bp->flags & BNXT_FLAG_AGG_RINGS)) { 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_copy_thresh); req->hds_threshold = cpu_to_le16(bp->rx_copy_thresh); } req->vnic_id = cpu_to_le32(vnic->fw_vnic_id); > > - } else { > > + if (bp->flags & BNXT_FLAG_AGG_RINGS) { > > 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);
On Fri, Oct 25, 2024 at 12:24 PM Andy Gospodarek <andrew.gospodarek@broadcom.com> wrote: > > On Thu, Oct 24, 2024 at 10:02:30PM -0700, Michael Chan wrote: > > On Tue, Oct 22, 2024 at 9:24 AM Taehee Yoo <ap420073@gmail.com> wrote: > > > > > > 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. > > > > > > Tested-by: Stanislav Fomichev <sdf@fomichev.me> > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > > --- > > > > > > v4: > > > - Do not support disable tcp-data-split. > > > - Add Test tag from Stanislav. > > > > > > v3: > > > - No changes. > > > > > > v2: > > > - Do not set hds_threshold to 0. > > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 +++----- > > > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 5 +++-- > > > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 13 +++++++++++++ > > > 3 files changed, 19 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > index 0f5fe9ba691d..91ea42ff9b17 100644 > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > > > @@ -6420,15 +6420,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); > > > > Please explain why this "if" condition is removed. > > BNXT_RX_PAGE_MODE() means that we are in XDP mode and we currently > > don't support HDS in XDP mode. Added Andy Gospo to CC so he can also > > comment. > > > > In bnxt_set_rx_skb_mode we set BNXT_FLAG_RX_PAGE_MODE and clear > BNXT_FLAG_AGG_RINGS The BNXT_FLAG_AGG_RINGS flag is true if the JUMBO, GRO, or LRO flag is set. So even though it is initially cleared in bnxt_set_rx_skb_mode(), we'll set the JUMBO flag if we are in multi-buffer XDP mode. Again, we don't enable HDS in any XDP mode so I think we need to keep the original logic here to skip setting the HDS threshold if BNXT_FLAG_RX_PAGE_MODE is set. > , so this should work. The only issue is that we > have spots in the driver where we check BNXT_RX_PAGE_MODE(bp) to > indicate that XDP single-buffer mode is enabled on the device. > > If you need to respin this series I would prefer that the change is like > below to key off the page mode being disabled and BNXT_FLAG_AGG_RINGS > being enabled to setup HDS. This will serve as a reminder that this is > for XDP. > > @@ -6418,15 +6418,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 (!BNXT_RX_PAGE_MODE(bp) && (bp->flags & BNXT_FLAG_AGG_RINGS)) { > 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_copy_thresh); > req->hds_threshold = cpu_to_le16(bp->rx_copy_thresh); > } > req->vnic_id = cpu_to_le32(vnic->fw_vnic_id); >
On Sat, Oct 26, 2024 at 7:00 AM Michael Chan <michael.chan@broadcom.com> wrote: > > On Fri, Oct 25, 2024 at 12:24 PM Andy Gospodarek > <andrew.gospodarek@broadcom.com> wrote: Hi Andy, Thank you so much for your review! > > > > On Thu, Oct 24, 2024 at 10:02:30PM -0700, Michael Chan wrote: > > > On Tue, Oct 22, 2024 at 9:24 AM Taehee Yoo <ap420073@gmail.com> wrote: > > > > > > > > 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. > > > > > > > > Tested-by: Stanislav Fomichev <sdf@fomichev.me> > > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > > > --- > > > > > > > > v4: > > > > - Do not support disable tcp-data-split. > > > > - Add Test tag from Stanislav. > > > > > > > > v3: > > > > - No changes. > > > > > > > > v2: > > > > - Do not set hds_threshold to 0. > > > > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 +++----- > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 5 +++-- > > > > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 13 +++++++++++++ > > > > 3 files changed, 19 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > > index 0f5fe9ba691d..91ea42ff9b17 100644 > > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > > > > > @@ -6420,15 +6420,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); > > > > > > Please explain why this "if" condition is removed. > > > BNXT_RX_PAGE_MODE() means that we are in XDP mode and we currently > > > don't support HDS in XDP mode. Added Andy Gospo to CC so he can also > > > comment. > > > > > > > In bnxt_set_rx_skb_mode we set BNXT_FLAG_RX_PAGE_MODE and clear > > BNXT_FLAG_AGG_RINGS > > The BNXT_FLAG_AGG_RINGS flag is true if the JUMBO, GRO, or LRO flag is > set. So even though it is initially cleared in > bnxt_set_rx_skb_mode(), we'll set the JUMBO flag if we are in > multi-buffer XDP mode. Again, we don't enable HDS in any XDP mode so > I think we need to keep the original logic here to skip setting the > HDS threshold if BNXT_FLAG_RX_PAGE_MODE is set. I thought the HDS is disallowed only when single-buffer XDP is set. By this series, Core API disallows tcp-data-split only when single-buffer XDP is set, but it allows tcp-data-split to set when multi-buffer XDP is set. + if (kernel_ringparam.tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED && + dev_xdp_sb_prog_count(dev)) { + NL_SET_ERR_MSG(info->extack, + "tcp-data-split can not be enabled with single buffer XDP"); + return -EINVAL; + } I think other drivers would allow tcp-data-split on multi buffer XDP, so I wouldn't like to remove this condition check code. I will not set HDS if XDP is set in the bnxt_hwrm_vnic_set_hds() In addition, I think we need to add a condition to check XDP is set in bnxt_set_ringparam(). > > > , so this should work. The only issue is that we > > have spots in the driver where we check BNXT_RX_PAGE_MODE(bp) to > > indicate that XDP single-buffer mode is enabled on the device. > > > > If you need to respin this series I would prefer that the change is like > > below to key off the page mode being disabled and BNXT_FLAG_AGG_RINGS > > being enabled to setup HDS. This will serve as a reminder that this is > > for XDP. > > > > @@ -6418,15 +6418,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 (!BNXT_RX_PAGE_MODE(bp) && (bp->flags & BNXT_FLAG_AGG_RINGS)) { > > 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_copy_thresh); > > req->hds_threshold = cpu_to_le16(bp->rx_copy_thresh); > > } > > req->vnic_id = cpu_to_le32(vnic->fw_vnic_id); > > Thanks a lot! Taehee Yoo
On Sat, Oct 26, 2024 at 02:11:15PM +0900, Taehee Yoo wrote: > On Sat, Oct 26, 2024 at 7:00 AM Michael Chan <michael.chan@broadcom.com> wrote: > > > > On Fri, Oct 25, 2024 at 12:24 PM Andy Gospodarek > > <andrew.gospodarek@broadcom.com> wrote: > > Hi Andy, > Thank you so much for your review! > > > > > > > On Thu, Oct 24, 2024 at 10:02:30PM -0700, Michael Chan wrote: > > > > On Tue, Oct 22, 2024 at 9:24 AM Taehee Yoo <ap420073@gmail.com> wrote: > > > > > > > > > > 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. > > > > > > > > > > Tested-by: Stanislav Fomichev <sdf@fomichev.me> > > > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > > > > --- > > > > > > > > > > v4: > > > > > - Do not support disable tcp-data-split. > > > > > - Add Test tag from Stanislav. > > > > > > > > > > v3: > > > > > - No changes. > > > > > > > > > > v2: > > > > > - Do not set hds_threshold to 0. > > > > > > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 +++----- > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 5 +++-- > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 13 +++++++++++++ > > > > > 3 files changed, 19 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > > > index 0f5fe9ba691d..91ea42ff9b17 100644 > > > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > > > > > > > @@ -6420,15 +6420,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); > > > > > > > > Please explain why this "if" condition is removed. > > > > BNXT_RX_PAGE_MODE() means that we are in XDP mode and we currently > > > > don't support HDS in XDP mode. Added Andy Gospo to CC so he can also > > > > comment. > > > > > > > > > > In bnxt_set_rx_skb_mode we set BNXT_FLAG_RX_PAGE_MODE and clear > > > BNXT_FLAG_AGG_RINGS > > > > The BNXT_FLAG_AGG_RINGS flag is true if the JUMBO, GRO, or LRO flag is > > set. So even though it is initially cleared in > > bnxt_set_rx_skb_mode(), we'll set the JUMBO flag if we are in > > multi-buffer XDP mode. Again, we don't enable HDS in any XDP mode so > > I think we need to keep the original logic here to skip setting the > > HDS threshold if BNXT_FLAG_RX_PAGE_MODE is set. > > I thought the HDS is disallowed only when single-buffer XDP is set. > By this series, Core API disallows tcp-data-split only when > single-buffer XDP is set, but it allows tcp-data-split to set when > multi-buffer XDP is set. So you are saying that a user could set copybreak with ethtool (included in patch 1) and when a multibuffer XDP program is attached to an interface with an MTU of 9k, only the header will be in the first page and all the TCP data will be in the pages that follow? > + if (kernel_ringparam.tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED && > + dev_xdp_sb_prog_count(dev)) { > + NL_SET_ERR_MSG(info->extack, > + "tcp-data-split can not be enabled with > single buffer XDP"); > + return -EINVAL; > + } > > I think other drivers would allow tcp-data-split on multi buffer XDP, > so I wouldn't like to remove this condition check code. > I have no problem keeping that logic in the core kernel. I'm just asking you to please preserve the existing logic since it is functionally equivalent and easier to read/compare to other spots where XDP single-buffer mode is used. > I will not set HDS if XDP is set in the bnxt_hwrm_vnic_set_hds() > In addition, I think we need to add a condition to check XDP is set in > bnxt_set_ringparam(). > > > > > > , so this should work. The only issue is that we > > > have spots in the driver where we check BNXT_RX_PAGE_MODE(bp) to > > > indicate that XDP single-buffer mode is enabled on the device. > > > > > > If you need to respin this series I would prefer that the change is like > > > below to key off the page mode being disabled and BNXT_FLAG_AGG_RINGS > > > being enabled to setup HDS. This will serve as a reminder that this is > > > for XDP. > > > > > > @@ -6418,15 +6418,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 (!BNXT_RX_PAGE_MODE(bp) && (bp->flags & BNXT_FLAG_AGG_RINGS)) { > > > 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_copy_thresh); > > > req->hds_threshold = cpu_to_le16(bp->rx_copy_thresh); > > > } > > > req->vnic_id = cpu_to_le32(vnic->fw_vnic_id); > > > > > Thanks a lot! > Taehee Yoo
On Thu, Oct 31, 2024 at 5:39 AM Andy Gospodarek <andrew.gospodarek@broadcom.com> wrote: > > On Sat, Oct 26, 2024 at 02:11:15PM +0900, Taehee Yoo wrote: > > On Sat, Oct 26, 2024 at 7:00 AM Michael Chan <michael.chan@broadcom.com> wrote: > > > > > > On Fri, Oct 25, 2024 at 12:24 PM Andy Gospodarek > > > <andrew.gospodarek@broadcom.com> wrote: > > > > Hi Andy, > > Thank you so much for your review! > > > > > > > > > > On Thu, Oct 24, 2024 at 10:02:30PM -0700, Michael Chan wrote: > > > > > On Tue, Oct 22, 2024 at 9:24 AM Taehee Yoo <ap420073@gmail.com> wrote: > > > > > > > > > > > > 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. > > > > > > > > > > > > Tested-by: Stanislav Fomichev <sdf@fomichev.me> > > > > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > > > > > --- > > > > > > > > > > > > v4: > > > > > > - Do not support disable tcp-data-split. > > > > > > - Add Test tag from Stanislav. > > > > > > > > > > > > v3: > > > > > > - No changes. > > > > > > > > > > > > v2: > > > > > > - Do not set hds_threshold to 0. > > > > > > > > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 +++----- > > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 5 +++-- > > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 13 +++++++++++++ > > > > > > 3 files changed, 19 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > > > > index 0f5fe9ba691d..91ea42ff9b17 100644 > > > > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > > > > > > > > > @@ -6420,15 +6420,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); > > > > > > > > > > Please explain why this "if" condition is removed. > > > > > BNXT_RX_PAGE_MODE() means that we are in XDP mode and we currently > > > > > don't support HDS in XDP mode. Added Andy Gospo to CC so he can also > > > > > comment. > > > > > > > > > > > > > In bnxt_set_rx_skb_mode we set BNXT_FLAG_RX_PAGE_MODE and clear > > > > BNXT_FLAG_AGG_RINGS > > > > > > The BNXT_FLAG_AGG_RINGS flag is true if the JUMBO, GRO, or LRO flag is > > > set. So even though it is initially cleared in > > > bnxt_set_rx_skb_mode(), we'll set the JUMBO flag if we are in > > > multi-buffer XDP mode. Again, we don't enable HDS in any XDP mode so > > > I think we need to keep the original logic here to skip setting the > > > HDS threshold if BNXT_FLAG_RX_PAGE_MODE is set. > > > > I thought the HDS is disallowed only when single-buffer XDP is set. > > By this series, Core API disallows tcp-data-split only when > > single-buffer XDP is set, but it allows tcp-data-split to set when > > multi-buffer XDP is set. > > So you are saying that a user could set copybreak with ethtool (included > in patch 1) and when a multibuffer XDP program is attached to an > interface with an MTU of 9k, only the header will be in the first page > and all the TCP data will be in the pages that follow? > I think you asked about `hds_threshold = bp->rx_copybreak` right? By this patchset, rx-copybreak will be a pure software feature. hds_threshold value will be set by `ethtool -G eth0 header-data-split-thresh N`. This is implemented in 4/8 patch. Sorry, I missed commenting in this commit message that hds_threshold value will no longer follow bp->rx_copybreak. If HDS is allowed when multi buffer XDP is attached, xdp program will see only the header on the first page. But As Michael and you suggested, HDS is not going to be allowed if XDP is attached. If I misunderstood your question, please let me know! > > + if (kernel_ringparam.tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED && > > + dev_xdp_sb_prog_count(dev)) { > > + NL_SET_ERR_MSG(info->extack, > > + "tcp-data-split can not be enabled with > > single buffer XDP"); > > + return -EINVAL; > > + } > > > > I think other drivers would allow tcp-data-split on multi buffer XDP, > > so I wouldn't like to remove this condition check code. > > > > I have no problem keeping that logic in the core kernel. I'm just > asking you to please preserve the existing logic since it is > functionally equivalent and easier to read/compare to other spots where > XDP single-buffer mode is used. Thanks a lot for the explanation and confirmation! I will preserve the existing logic. > > > I will not set HDS if XDP is set in the bnxt_hwrm_vnic_set_hds() > > In addition, I think we need to add a condition to check XDP is set in > > bnxt_set_ringparam(). > > > > > > > > > , so this should work. The only issue is that we > > > > have spots in the driver where we check BNXT_RX_PAGE_MODE(bp) to > > > > indicate that XDP single-buffer mode is enabled on the device. > > > > > > > > If you need to respin this series I would prefer that the change is like > > > > below to key off the page mode being disabled and BNXT_FLAG_AGG_RINGS > > > > being enabled to setup HDS. This will serve as a reminder that this is > > > > for XDP. > > > > > > > > @@ -6418,15 +6418,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 (!BNXT_RX_PAGE_MODE(bp) && (bp->flags & BNXT_FLAG_AGG_RINGS)) { > > > > 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_copy_thresh); > > > > req->hds_threshold = cpu_to_le16(bp->rx_copy_thresh); > > > > } > > > > req->vnic_id = cpu_to_le32(vnic->fw_vnic_id); > > > > > > > > Thanks a lot! > > Taehee Yoo
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index 0f5fe9ba691d..91ea42ff9b17 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -4473,7 +4473,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,15 +6420,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_AGG_RINGS) { 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 1b83a2c8027b..432bc19b35ea 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -2201,8 +2201,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 @@ -2223,6 +2221,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 9af0a3f34750..5172d0547e0c 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -854,9 +854,21 @@ 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) + return -EOPNOTSUPP; + if (netif_running(dev)) bnxt_close_nic(bp, false, false); + switch (kernel_ering->tcp_data_split) { + case ETHTOOL_TCP_DATA_SPLIT_ENABLED: + bp->flags |= BNXT_FLAG_HDS; + break; + case ETHTOOL_TCP_DATA_SPLIT_UNKNOWN: + 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); @@ -5345,6 +5357,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,