[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACKFLikK0p1e41sbkBt1MCL=gxoWbKNHvqzEfcaV=rBfvQjB4w@mail.gmail.com>
Date: Thu, 24 Oct 2024 21:54:50 -0700
From: Michael Chan <michael.chan@...adcom.com>
To: Taehee Yoo <ap420073@...il.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
edumazet@...gle.com, almasrymina@...gle.com, donald.hunter@...il.com,
corbet@....net, andrew+netdev@...n.ch, hawk@...nel.org,
ilias.apalodimas@...aro.org, ast@...nel.org, daniel@...earbox.net,
john.fastabend@...il.com, dw@...idwei.uk, sdf@...ichev.me,
asml.silence@...il.com, brett.creeley@....com, linux-doc@...r.kernel.org,
netdev@...r.kernel.org, kory.maincent@...tlin.com,
maxime.chevallier@...tlin.com, danieller@...dia.com, hengqi@...ux.alibaba.com,
ecree.xilinx@...il.com, przemyslaw.kitszel@...el.com, hkallweit1@...il.com,
ahmed.zaki@...el.com, rrameshbabu@...dia.com, idosch@...dia.com,
jiri@...nulli.us, bigeasy@...utronix.de, lorenzo@...nel.org,
jdamato@...tly.com, aleksander.lobakin@...el.com, kaiyuanz@...gle.com,
willemb@...gle.com, daniel.zahka@...il.com
Subject: Re: [PATCH net-next v4 1/8] bnxt_en: add support for rx-copybreak
ethtool command
On Thu, Oct 24, 2024 at 9:38 AM Taehee Yoo <ap420073@...il.com> wrote:
>
> On Thu, Oct 24, 2024 at 3:41 PM Michael Chan
> > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > > index bda3742d4e32..0f5fe9ba691d 100644
> > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> >
> > > @@ -4510,7 +4513,8 @@ void bnxt_set_ring_params(struct bnxt *bp)
> > > ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) -
> > > SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > > } else {
> > > - rx_size = SKB_DATA_ALIGN(BNXT_RX_COPY_THRESH + NET_IP_ALIGN);
> > > + rx_size = SKB_DATA_ALIGN(bp->rx_copybreak +
> > > + NET_IP_ALIGN);
> >
> > When rx_copybreak is 0 or very small, rx_size will be very small and
> > will be a problem. We need rx_size to be big enough to contain the
> > packet header, so rx_size cannot be below some minimum (256?).
> >
> > > rx_space = rx_size + NET_SKB_PAD +
> > > SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > > }
> > > @@ -6424,8 +6428,8 @@ 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->jumbo_thresh = cpu_to_le16(bp->rx_copy_thresh);
> > > - req->hds_threshold = cpu_to_le16(bp->rx_copy_thresh);
> > > + req->jumbo_thresh = cpu_to_le16(bp->rx_copybreak);
> > > + req->hds_threshold = cpu_to_le16(bp->rx_copybreak);
> >
> > Similarly, these thresholds should not go to 0 when rx_copybreak becomes small.
> >
> > > }
> > > req->vnic_id = cpu_to_le32(vnic->fw_vnic_id);
> > > return hwrm_req_send(bp, req);
> >
> > > @@ -4769,7 +4813,7 @@ static int bnxt_run_loopback(struct bnxt *bp)
> > > cpr = &rxr->bnapi->cp_ring;
> > > if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS)
> > > cpr = rxr->rx_cpr;
> > > - pkt_size = min(bp->dev->mtu + ETH_HLEN, bp->rx_copy_thresh);
> > > + pkt_size = min(bp->dev->mtu + ETH_HLEN, bp->rx_copybreak);
> >
> > The loopback test will also not work if rx_copybreak is very small. I
> > think we should always use 256 bytes for the loopback test packet
> > size. Thanks.
> >
> > > skb = netdev_alloc_skb(bp->dev, pkt_size);
> > > if (!skb)
> > > return -ENOMEM;
>
> I tested `ethtool -t eth0` and I checked it fails if rx-copybreak is too
> small. Sorry for missing that.
> I think we can use max(BNXT_DEFAULT_RX_COPYBREAK,
> bp->rx_copybreak) for both cases.
> I tested it, it works well.
> So I will use that if you are okay!
>
Yes, please go ahead. I think all 3 places I commented above should
use max(BNXT_DEFAULT_RX_COPYBREAK, bp->rx_copybreak). Thanks.
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)
Powered by blists - more mailing lists