Re: [PATCH net-next v4 2/8] bnxt_en: add support for tcp-data-split ethtool command

[Andy Gospodarek <andrew.gospodarek@broadcom.com>]

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