On Mon, Jan 28, 2019 at 02:49:32PM -0600, Willem de Bruijn wrote:

On Mon, Jan 28, 2019 at 2:51 AM Steffen Klassert
[off-list ref] wrote:

quoted

This patch extends UDP GRO to support fraglist GRO/GSO
by using the previously introduced infrastructure.
All UDP packets that are not targeted to a GRO capable
UDP sockets are going to fraglist GRO now (local input
and forward).
---
 net/ipv4/udp_offload.c | 45 ++++++++++++++++++++++++++++++++++++++----
 net/ipv6/udp_offload.c |  9 +++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 584635db9231..c0be33216750 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c

@@ -188,6 +188,22 @@ struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
 }
 EXPORT_SYMBOL(skb_udp_tunnel_segment);

+static struct sk_buff *__udp_gso_segment_list(struct sk_buff *skb,
+                                             netdev_features_t features)
+{
+       unsigned int mss = skb_shinfo(skb)->gso_size;
+
+       skb = skb_segment_list(skb, features, skb_mac_header_len(skb));
+       if (IS_ERR(skb))
+               return skb;
+
+       udp_hdr(skb)->len = htons(sizeof(struct udphdr) + mss);
+       skb->ip_summed = CHECKSUM_NONE;
+       skb->csum_valid = 1;

csum_valid is only used on ingress.

Hardcoding CHECKSUM_NONE is probably fine as long as this function is
only used for forwarding, assuming we don't care about verifiying
checksums in the forwarding case.

But this is fragile if we ever add local list segment output. Should
convert the checksum field in skb_forward_csum, instead of at the GSO
layer, just as for forwarding of non list skbs? Though that would
require traversing the list yet another time. Other option is to
already do this conversion when building the list in GRO.

The comment also applies to the same logic in skb_segment_list. As a
matter or fact, even if this belongs in GSO instead of core forwarding
or GRO, then probably both the list head and frag_list skbs should be
set in the same function, so skb_segment_list.

I had a deeper look into this now, it seems that the head skb has
already the correct conversion (either for local input or forwarding).
So we probaply just need to adjust the frag_list skbs to the
head skb checksum conversion.

quoted

        /* pull encapsulating udp header */
        skb_gro_pull(skb, sizeof(struct udphdr));
-       skb_gro_postpull_rcsum(skb, uh, sizeof(struct udphdr));

        list_for_each_entry(p, head, list) {
                if (!NAPI_GRO_CB(p)->same_flow)

@@ -379,8 +397,17 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
                 * Under small packet flood GRO count could elsewhere grow a lot
                 * leading to execessive truesize values
                 */
-               if (!skb_gro_receive(p, skb) &&
-                   NAPI_GRO_CB(p)->count >= UDP_GRO_CNT_MAX)
+               if (NAPI_GRO_CB(skb)->is_flist) {
+                       if (!pskb_may_pull(skb, skb_gro_offset(skb)))
+                               return NULL;
+                       ret = skb_gro_receive_list(p, skb);
+               } else {
+                       skb_gro_postpull_rcsum(skb, uh, sizeof(struct udphdr));
+
+                       ret = skb_gro_receive(p, skb);
+               }
+
+               if (!ret && NAPI_GRO_CB(p)->count > UDP_GRO_CNT_MAX)
                        pp = p;
                else if (uh->len != uh2->len)
                        pp = p;

@@ -402,6 +429,7 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
        int flush = 1;

        if (!sk || !udp_sk(sk)->gro_receive) {
+               NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;

This updates the choice of whether to use a list on each received skb.
Which is problematic as a socket can call the setsockopt in between
packets.

Actually, there no longer is a need for a route lookup for each skb at
all. We always apply GRO, which was the previous reason for the
lookup. And if a matching flow is found in the GRO table, we already
the choice to use a list is already stored.

Yes, that's true. I'll change that.