netdev - Re: [PATCH net-next v4 1/8] bnxt_en: add support for rx-copybreak ethtool command

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMArcTV3U62Rz+FPCJWVOqqNJOZBLnBvb+yRcjJ+drspm5nxbw@mail.gmail.com>
Date: Fri, 25 Oct 2024 01:38:15 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Michael Chan <michael.chan@...adcom.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 3:41 PM Michael Chan

Hi Michael,
Thank you so much for the review!

<michael.chan@...adcom.com> wrote:
>
> On Tue, Oct 22, 2024 at 9:24 AM Taehee Yoo <ap420073@...il.com> wrote:
> >
> > The bnxt_en driver supports rx-copybreak, but it couldn't be set by
> > userspace. Only the default value(256) has worked.
> > This patch makes the bnxt_en driver support following command.
> > `ethtool --set-tunable <devname> rx-copybreak <value> ` and
> > `ethtool --get-tunable <devname> rx-copybreak`.
> >
> > Reviewed-by: Brett Creeley <brett.creeley@....com>
> > Tested-by: Stanislav Fomichev <sdf@...ichev.me>
> > Signed-off-by: Taehee Yoo <ap420073@...il.com>
> > ---
> >
> > v4:
> >  - Remove min rx-copybreak value.
> >  - Add Review tag from Brett.
> >  - Add Test tag from Stanislav.
> >
> > v3:
> >  - Update copybreak value after closing nic and before opening nic when
> >    the device is running.
> >
> > v2:
> >  - Define max/vim rx_copybreak value.
> >
> >  drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 23 +++++----
> >  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  5 +-
> >  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 48 ++++++++++++++++++-
> >  3 files changed, 65 insertions(+), 11 deletions(-)
> >
> > 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!

Thank you so much,
Tahee Yoo