Re: [RFC PATCH net-next] net: gro: coalesce padded small IPv4 TCP segments

> Also do not change frag0 and frag0_len: In GRO, frag0 is strictly
> reserved for page-fragmented skbs (napi_gro_frags(), where
> !skb_headlen(skb)). For linear skbs, frag0 must remain NULL.

Thanks for all the feedback. I have incorporated the partial
checksum optimization into my prototype.

Before presenting a prototype that addresses all feedback and
provides quantitative evaluation, I think it's important to discuss
the frag0, frag0_len point that you raised above.

gro.h:20 agrees with that description:
"Virtual address of skb_shinfo(skb)->frags[0].page + offset"

As far as I can tell, however, this appears to have changed
in c7583e9f768e ("net: gro: enable fast path for more cases"):
  "We therefore can initialize frag0 to skb->data so that GRO fast path
can be used in the following additional cases:
  - Drivers using header split (populating skb->data with headers, and
    having payload in one or more page fragments).
  - Drivers not using any page frag (entire packet is in skb->data)"

skb_gro_reset_offset() was modified as follows:
         NAPI_GRO_CB(skb)->data_offset = 0;
-        NAPI_GRO_CB(skb)->frag0 = NULL;
-        NAPI_GRO_CB(skb)->frag0_len = 0;
+        headlen = skb_headlen(skb);
+        NAPI_GRO_CB(skb)->frag0 = skb->data;
+        NAPI_GRO_CB(skb)->frag0_len = headlen;
+        if (headlen)
+                return;

-        if (!skb_headlen(skb) && pinfo->nr_frags &&
+       if (pinfo->nr_frags && ...

Unless I'm misreading something here, not touching frag0 and
frag0_len appears to leave the trim patch stuck. Trimming
shortens the skb, so frag0_len goes stale, resulting in reads
past the trimmed length.

Updating frag0_len after trim solves one inconsistency, but
updating *only* frag0_len causes a new inconsistency.
skb_gro_header() computes frag0 + offset before testing
skb_gro_may_pull(), while pskb_trim_rcsum() calls
skb_might_realloc() first, so the head can move even for
a linear skb. Refreshing iph through skb_gro_header()
does not appear to be enough on its own.

I believe that both frag0_len and frag0 must be updated together.
This could be coded directly in af_inet.c, but gro.c/h owns updates to
these fields. If we want to cleanly update these after a trim, we need
a small helper in gro.h (other fields already have similar helpers):

static inline void skb_gro_reset_frag0(struct sk_buff *skb)
{
        NAPI_GRO_CB(skb)->frag0 = skb->data;
        NAPI_GRO_CB(skb)->frag0_len = skb_headlen(skb);
}

with skb_gro_reset_offset() using it for its linear path, and af_inet.c
calling it after a successful trim. The write then belongs to
GRO rather than IPv4.

Let me know what you think.

Thanks.