Re: [RFC net-next v4 2/2] bnxt: Use generic HBH removal helper in tx path
From: Michael Chan
Date: Mon Dec 05 2022 - 17:48:06 EST
On Sun, Dec 4, 2022 at 9:14 PM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
>
> On Fri, Dec 2, 2022 at 11:12 PM Coco Li <lixiaoyan@xxxxxxxxxx> wrote:
> >
> > Eric Dumazet implemented Big TCP that allowed bigger TSO/GRO packet sizes
> > for IPv6 traffic. See patch series:
> > 'commit 89527be8d8d6 ("net: add IFLA_TSO_{MAX_SIZE|SEGS} attributes")'
> >
> > This reduces the number of packets traversing the networking stack and
> > should usually improves performance. However, it also inserts a
> > temporary Hop-by-hop IPv6 extension header.
> >
> > Using the HBH header removal method in the previous path, the extra header
> > be removed in bnxt drivers to allow it to send big TCP packets (bigger
> > TSO packets) as well.
> >
> > Tested:
> > Compiled locally
> >
> > To further test functional correctness, update the GSO/GRO limit on the
> > physical NIC:
> >
> > ip link set eth0 gso_max_size 181000
> > ip link set eth0 gro_max_size 181000
> >
> > Note that if there are bonding or ipvan devices on top of the physical
> > NIC, their GSO sizes need to be updated as well.
> >
> > Then, IPv6/TCP packets with sizes larger than 64k can be observed.
> >
> > Big TCP functionality is tested by Michael, feature checks not yet.
> >
> > Tested by Michael:
> > I've confirmed with our hardware team that this is supported by our
> > chips, and I've tested it up to gso_max_size of 524280. Thanks.
> >
> > Tested-by: Michael Chan <michael.chan@xxxxxxxxxxxx>
> > Reviewed-by: Michael Chan <michael.chan@xxxxxxxxxxxx>
> > Signed-off-by: Coco Li <lixiaoyan@xxxxxxxxxx>
> > ---
> > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 26 ++++++++++++++++++++++-
> > 1 file changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index 0fe164b42c5d..c2713cb5debd 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -389,6 +389,9 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > return NETDEV_TX_BUSY;
> > }
> >
> > + if (unlikely(ipv6_hopopt_jumbo_remove(skb)))
> > + goto tx_free;
> > +
> > length = skb->len;
> > len = skb_headlen(skb);
> > last_frag = skb_shinfo(skb)->nr_frags;
> > @@ -11342,9 +11345,28 @@ static bool bnxt_exthdr_check(struct bnxt *bp, struct sk_buff *skb, int nw_off,
> >
> > if (hdrlen > 64)
> > return false;
> > +
> > + /* The ext header may be a hop-by-hop header inserted for
> > + * big TCP purposes. This will be removed before sending
> > + * from NIC, so do not count it.
> > + */
> > + if (*nexthdr == NEXTHDR_HOP) {
> > + if (likely(skb->len <= GRO_LEGACY_MAX_SIZE))
> > + goto increment_hdr;
> > +
> > + struct hop_jumbo_hdr *jhdr = (struct hop_jumbo_hdr *)(nexthdr + hdrlen);
>
> We discourage adding a variable declaration in the middle of code.
>
This doesn't work. Initially nexthdr points to the nexthdr in the
ipv6 header. It should be coded like this:
struct hop_jumbo_hdr *jhdr = (struct hop_jumbo_hdr *)hp;
Thanks.
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature