Re: [PATCH net-next v2 4/4] bnxt_en: add support for tcp-data-split-thresh ethtool command

[Brett Creeley <bcreeley@xxxxxxx>]

On 9/11/2024 7:55 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:
>     ...
>     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:
>     ...
>     Current hardware settings:
>     ...
>     TCP data split:         off
>     TCP data split thresh:  n/a
>
> Signed-off-by: Taehee Yoo <ap420073@xxxxxxxxx>
> ---
>
> 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 | 9 +++++++++
>   3 files changed, 13 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;
>
>          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 ab64d7f94796..5b1f3047bf84 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -839,6 +839,8 @@ 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;
> +
>          ering->tx_max_pending = BNXT_MAX_TX_DESC_CNT;
>
>          ering->rx_pending = bp->rx_ring_size;
> @@ -864,6 +866,12 @@ static int bnxt_set_ringparam(struct net_device *dev,
>                  return -EINVAL;
>          }
>
> +       if (kernel_ering->tcp_data_split_thresh > BNXT_HDS_THRESHOLD_MAX) {
> +               NL_SET_ERR_MSG_MOD(extack,
> +                                  "tcp-data-split-thresh size too big");

Should you print the BNXT_HDS_THRESHOLD_MAX value here so the user knows 
the max size?

Actually, does it make more sense for ethtool get_ringparam to query the 
max threshold size from the driver and reject this in the core so all 
drivers don't have to have this same kind of check?

Thanks,

Brett

> +               return -EINVAL;
> +       }
> +
>          if (netif_running(dev))
>                  bnxt_close_nic(bp, false, false);
>
> @@ -871,6 +879,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