RE: [PATCH net-next] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets

On 2021-01-13 12:10, Alexander Duyck wrote:

On 1/12/21 1:16 PM, Alexander Lobakin wrote:

quoted

Commit 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.") actually
not only added a support for fraglisted UDP GRO, but also tweaked
some logics the way that non-fraglisted UDP GRO started to work for
forwarding too.
Tests showed that currently forwarding and NATing of plain UDP GRO
packets are performed fully correctly, regardless if the target
netdevice has a support for hardware/driver GSO UDP L4 or not.
Add the last element and allow to form plain UDP GRO packets if
there is no socket -> we are on forwarding path.

Your patch is very similar with the RFC what I submitted but has
different approach. My concern was NAT forwarding.
https://lore.kernel.org/patchwork/patch/1362257/

Nevertheless, I agreed with your idea that allow fraglisted UDP GRO
if there is socket.

quoted

Plain UDP GRO forwarding even shows better performance than fraglisted
UDP GRO in some cases due to not wasting one skbuff_head per every
segment.

Signed-off-by: Alexander Lobakin <redacted>
---
  net/ipv4/udp_offload.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index ff39e94781bf..9d71df3d52ce 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c

@@ -460,12 +460,13 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
  	if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
  		NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;

is_flist can be true even if !sk.

quoted

-	if ((sk && udp_sk(sk)->gro_enabled) || NAPI_GRO_CB(skb)->is_flist) {
+	if (!sk || (sk && udp_sk(sk)->gro_enabled) ||

Actually sk would be NULL by udp_encap_needed_key in udp4_gro_receive
or udp6_gro_receive.

quoted

+	    NAPI_GRO_CB(skb)->is_flist) {
  		pp = call_gro_receive(udp_gro_receive_segment, head, skb);

udp_gro_receive_segment will check is_flist first and try to do
fraglisted UDP GRO. Can you check what I'm missing?

quoted

  		return pp;
  	}

The second check for sk in "(sk && udp_sk(sk)->gro_enabled)" is
redundant and can be dropped. You already verified it is present when
you checked for !sk before the logical OR.

Sorry, Alexander Duyck. I believe Alexander Lobakin will answer this.

quoted

-	if (!sk || NAPI_GRO_CB(skb)->encap_mark ||
+	if (NAPI_GRO_CB(skb)->encap_mark ||
  	    (skb->ip_summed != CHECKSUM_PARTIAL &&
  	     NAPI_GRO_CB(skb)->csum_cnt == 0 &&
  	     !NAPI_GRO_CB(skb)->csum_valid) ||