On Fri, Oct 4, 2024 at 3:13 AM Brett Creeley <bcreeley@xxxxxxx> wrote:
>
>
>
> On 10/3/2024 9:06 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:
> > ...
> > TCP data split thresh: 256
> > 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:
> > ...
> > TCP data split thresh: 256
> > Current hardware settings:
> > ...
> > TCP data split: off
> > TCP data split thresh: n/a
> >
> > Signed-off-by: Taehee Yoo <ap420073@xxxxxxxxx>
> > ---
> >
> > v3:
> > - Drop validation logic tcp-data-split and tcp-data-split-thresh.
> >
> > 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 | 4 ++++
> > 3 files changed, 8 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;
>
> Putting this here creates a 2 byte hole right after hds_threshold and
> also puts a 4 byte hole after cp_nr_rings.
>
> Since hds_threshold doesn't seem to be used in the hotpath maybe it
> would be best to fill a pre-existing hole in struct bnxt to put it?
>
Yes, hds_threshold makes an additional 2bytes hole.
I checked pre-existing holes in struct bnxt, and almost all members
are sorted by purpose.
However, I think under num_tc would be a pretty good place for hds_threshold.
Before:
/* size: 7000, cachelines: 110, members: 185 */
/* sum members: 6931, holes: 21, sum holes: 64 */
/* sum bitfield members: 2 bits, bit holes: 1, sum bit holes: 6 bits */
/* padding: 4 */
/* paddings: 7, sum paddings: 25 */
/* last cacheline: 24 bytes */
After:
/* size: 6992, cachelines: 110, members: 185 */
/* sum members: 6931, holes: 19, sum holes: 56 */
/* sum bitfield members: 2 bits, bit holes: 1, sum bit holes: 6 bits */
/* padding: 4 */
/* paddings: 7, sum paddings: 25 */
/* last cacheline: 16 bytes */
So, I would like to change it in a v4 patch if there are no objections.
Thanks a lot!
Taehee Yoo
> Thanks,
>
> Brett
>
> >
> > 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 e9ef65dd2e7b..af6ed492f688 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > @@ -839,6 +839,9 @@ 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;
> > + kernel_ering->tcp_data_split_thresh_max = BNXT_HDS_THRESHOLD_MAX;
> > +
> > ering->tx_max_pending = BNXT_MAX_TX_DESC_CNT;
> >
> > ering->rx_pending = bp->rx_ring_size;
> > @@ -871,6 +874,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
> >
