Re: [RFC net-next v4 2/2] bnxt: Use generic HBH removal helper in tx path

[Michael Chan <michael.chan@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 3312 bytes --]

On Sun, Dec 4, 2022 at 9:14 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Dec 2, 2022 at 11:12 PM Coco Li <lixiaoyan@google.com> 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@broadcom.com>
> > Reviewed-by: Michael Chan <michael.chan@broadcom.com>
> > Signed-off-by: Coco Li <lixiaoyan@google.com>
> > ---
> >  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 #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]