Received-SPF: pass (google.com: domain of linux-kernel+bounces-15578-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <127b8199-1cd4-42d7-9b2b-875abaad93fe@gmail.com>
 <90117449-1f4a-47d7-baf4-2ed6540bc436@gmail.com> <CANn89i+GJOgcDWK=C0+vmomt2ShotrOKyLiXzFkfT1W8vpJv1Q@mail.gmail.com>
 <9419df03-a203-4b73-91a6-f008076c29b4@gmail.com>
In-Reply-To: <9419df03-a203-4b73-91a6-f008076c29b4@gmail.com>
From: Eric Dumazet <edumazet@google.com>
Date: Wed, 3 Jan 2024 14:30:00 +0100
Message-ID: <CANn89iJdPRspWo2XzqdGdGe9_am7zNwbq9vm0AFLF-KRODzE7A@mail.gmail.com>
Subject: Re: [PATCH net-next v2 2/3] net: gro: parse ipv6 ext headers without
 frag0 invalidation
To: Richard Gobert <richardbgobert@gmail.com>
Cc: davem@davemloft.net, dsahern@kernel.org, kuba@kernel.org, 
	pabeni@redhat.com, shuah@kernel.org, netdev@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 3, 2024 at 2:08=E2=80=AFPM Richard Gobert <richardbgobert@gmail=
.com> wrote:
>
>
>
> Eric Dumazet wrote:
> > On Tue, Jan 2, 2024 at 2:25=E2=80=AFPM Richard Gobert <richardbgobert@g=
mail.com> wrote:
> >>
> >> The existing code always pulls the IPv6 header and sets the transport
> >> offset initially. Then optionally again pulls any extension headers in
> >> ipv6_gso_pull_exthdrs and sets the transport offset again on return fr=
om
> >> that call. skb->data is set at the start of the first extension header
> >> before calling ipv6_gso_pull_exthdrs, and must disable the frag0
> >> optimization because that function uses pskb_may_pull/pskb_pull instea=
d of
> >> skb_gro_ helpers. It sets the GRO offset to the TCP header with
> >> skb_gro_pull and sets the transport header. Then returns skb->data to =
its
> >> position before this block.
> >>
> >> This commit introduces a new helper function - ipv6_gro_pull_exthdrs -
> >> which is used in ipv6_gro_receive to pull ipv6 ext headers instead of
> >> ipv6_gso_pull_exthdrs. Thus, there is no modification of skb->data, al=
l
> >> operations use skb_gro_* helpers, and the frag0 fast path can be taken=
 for
> >> IPv6 packets with ext headers.
> >>
> >> Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
> >> Reviewed-by: Willem de Bruijn <willemb@google.com>
> >> ---
> >>  include/net/ipv6.h     |  1 +
> >>  net/ipv6/ip6_offload.c | 51 +++++++++++++++++++++++++++++++++--------=
-
> >>  2 files changed, 42 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> >> index 78d38dd88aba..217240efa182 100644
> >> --- a/include/net/ipv6.h
> >> +++ b/include/net/ipv6.h
> >> @@ -26,6 +26,7 @@ struct ip_tunnel_info;
> >>  #define SIN6_LEN_RFC2133       24
> >>
> >>  #define IPV6_MAXPLEN           65535
> >> +#define IPV6_MIN_EXTHDR_LEN    8
> >
> > // Hmm see my following comment.
> >
> >>
> >>  /*
> >>   *     NextHeader field of IPv6 header
> >> diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
> >> index 0e0b5fed0995..c07111d8f56a 100644
> >> --- a/net/ipv6/ip6_offload.c
> >> +++ b/net/ipv6/ip6_offload.c
> >> @@ -37,6 +37,40 @@
> >>                 INDIRECT_CALL_L4(cb, f2, f1, head, skb);        \
> >>  })
> >>
> >> +static int ipv6_gro_pull_exthdrs(struct sk_buff *skb, int off, int pr=
oto)
> >> +{
> >> +       const struct net_offload *ops =3D NULL;
> >> +       struct ipv6_opt_hdr *opth;
> >> +
> >> +       for (;;) {
> >> +               int len;
> >> +
> >> +               ops =3D rcu_dereference(inet6_offloads[proto]);
> >> +
> >> +               if (unlikely(!ops))
> >> +                       break;
> >> +
> >> +               if (!(ops->flags & INET6_PROTO_GSO_EXTHDR))
> >> +                       break;
> >> +
> >> +               opth =3D skb_gro_header(skb, off + IPV6_MIN_EXTHDR_LEN=
, off);
> >
> > I do not see a compelling reason for adding yet another constant here.
> >
> > I would stick to
> >
> >    opth =3D skb_gro_header(skb, off + sizeof(*opth), off);
> >
> > Consistency with similar helpers is desirable.
> >
>
> In terms of consistency - similar helper functions (ipv6_gso_pull_exthdrs=
,
> ipv6_parse_hopopts) also pull 8 bytes at the beginning of every IPv6
> extension header, because the minimum extension header length is 8 bytes.
>
> sizeof(*opth) =3D 2, so for an IPv6 packet with one extension header with=
 a
> common length of 8 bytes, pskb_may_pull will be called twice: first with
> length =3D 2 and again with length =3D 8, which might not be ideal when p=
arsing
> non-linear packets.
>
> Willem suggested adding a constant to make the code more self-documenting=
.


Hmm... I was looking at

skb_checksum_setup_ipv6() , it uses skb_maybe_pull_tail( ...
sizeof(struct ipv6_opt_hdr))
ipv6_skip_exthdr()  also uses sizeof(struct ipv6_opt_hdr)
ip6_tnl_parse_tlv_enc_lim also uses the same.
hbh_mt6(), ipv6header_mt6(),  .. same...
ip6_find_1stfragopt(), get_ipv6_ext_hdrs(), tcf_csum_ipv6(),
mip6_rthdr_offset() same

So it seems you found two helpers that went the other way.

If you think pulling 8 bytes first is a win, I would suggest a stand
alone patch, adding the magic constant
using it in all places, so that a casual reader can make sense of the
magical 8 value.