Received-SPF: pass (google.com: domain of linux-kernel+bounces-144306-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240412152120.115067-1-richardbgobert@gmail.com>
 <20240412152120.115067-2-richardbgobert@gmail.com> <661ad1136bc10_3be9a7294c2@willemb.c.googlers.com.notmuch>
In-Reply-To: <661ad1136bc10_3be9a7294c2@willemb.c.googlers.com.notmuch>
From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Sun, 14 Apr 2024 10:22:56 -0700
Message-ID: <CAKgT0UfB+3DTjK7vq1uvG-2xtA53pw03ygJhwSG8j1bPtmYU8A@mail.gmail.com>
Subject: Re: [PATCH net v1 1/2] net: gro: add flush check in udp_gro_receive_segment
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Richard Gobert <richardbgobert@gmail.com>, davem@davemloft.net, edumazet@google.com, 
	kuba@kernel.org, pabeni@redhat.com, dsahern@kernel.org, 
	aleksander.lobakin@intel.com, netdev@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Apr 13, 2024 at 11:38=E2=80=AFAM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Richard Gobert wrote:
> > GRO-GSO path is supposed to be transparent and as such L3 flush checks =
are
> > relevant to all flows which call skb_gro_receive. This patch uses the s=
ame
> > logic and code from tcp_gro_receive but in the relevant flow path in
> > udp_gro_receive_segment.
> >
> > Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) U=
DP GRO packets")
> > Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
>
> > ---
> >  net/ipv4/udp_offload.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > index 3498dd1d0694..1f4e08f43c4b 100644
> > --- a/net/ipv4/udp_offload.c
> > +++ b/net/ipv4/udp_offload.c
> > @@ -471,6 +471,7 @@ static struct sk_buff *udp_gro_receive_segment(stru=
ct list_head *head,
> >       struct sk_buff *p;
> >       unsigned int ulen;
> >       int ret =3D 0;
> > +     int flush;
> >
> >       /* requires non zero csum, for symmetry with GSO */
> >       if (!uh->check) {
> > @@ -528,7 +529,17 @@ static struct sk_buff *udp_gro_receive_segment(str=
uct list_head *head,
> >                               skb_gro_postpull_rcsum(skb, uh,
> >                                                      sizeof(struct udph=
dr));
> >
> > -                             ret =3D skb_gro_receive(p, skb);
> > +                             flush =3D NAPI_GRO_CB(p)->flush;
> > +
> > +                             if (NAPI_GRO_CB(p)->flush_id !=3D 1 ||
> > +                                 NAPI_GRO_CB(p)->count !=3D 1 ||
> > +                                 !NAPI_GRO_CB(p)->is_atomic)
> > +                                     flush |=3D NAPI_GRO_CB(p)->flush_=
id;
> > +                             else
> > +                                     NAPI_GRO_CB(p)->is_atomic =3D fal=
se;
> > +
> > +                             if (flush || skb_gro_receive(p, skb))
> > +                                     ret =3D 1;
>
> UDP_L4 does not have the SKB_GSO_TCP_FIXEDID that uses is_atomic as
> input.
>
> And I still don't fully internalize the flush_id logic after staring
> at it for more than one coffee.

The flush_id field is there to indicate the difference between the
current IPv4 ID of the previous IP header. It is meant to be used in
conjunction with the is_atomic for the frame coalescing. Basically
after the second frame we can decide the pattern either incrementing
IPv4 ID or fixed, so on frames 3 or later we can decide to drop the
frame if it doesn't follow that pattern.

> But even ignoring those, the flush signal of NAPI_GRO_CB(p)->flush
> set the network layer must be followed, so ACK. Thanks for the fix.

I'm not sure about the placement of this code though. That is the one
thing that seems off to me. Specifically this seems like it should be
done before we start the postpull, not after. It should be something
that can terminate the flow before we attempt to aggregate the UDP
headers.