On Thu, Oct 31, 2024 at 5:39 AM Andy Gospodarek
<andrew.gospodarek@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> wrote:
> > >
> > > On Fri, Oct 25, 2024 at 12:24 PM Andy Gospodarek
> > > <andrew.gospodarek@xxxxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxx>
> > > > > > Signed-off-by: Taehee Yoo <ap420073@xxxxxxxxx>
> > > > > > ---
> > > > > >
> > > > > > 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
