Only merge encapsulated packets if their outer IDs are either
incrementing or fixed, just like for inner IDs and IDs of non-encapsulated
packets.
Add another ip_fixedid bit for a total of two bits: one for outer IDs (and
for unencapsulated packets) and one for inner IDs.
This commit preserves the current behavior of GSO where only the IDs of the
inner-most headers are restored correctly.
Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
---
include/net/gro.h | 37 ++++++++++++++++++-------------------
net/ipv4/tcp_offload.c | 5 ++++-
2 files changed, 22 insertions(+), 20 deletions(-)
diff --git a/include/net/gro.h b/include/net/gro.h
index 87c68007f949..4307b68afda7 100644
--- a/include/net/gro.h
+++ b/include/net/gro.h
@@ -75,7 +75,7 @@ struct napi_gro_cb {
u8 is_fou:1;
/* Used to determine if ipid_offset can be ignored */
- u8 ip_fixedid:1;
+ u8 ip_fixedid:2;
/* Number of gro_receive callbacks this packet already went through */
u8 recursion_counter:4;
@@ -442,29 +442,26 @@ static inline __wsum ip6_gro_compute_pseudo(const struct sk_buff *skb,
}
static inline int inet_gro_flush(const struct iphdr *iph, const struct iphdr *iph2,
- struct sk_buff *p, bool outer)
+ struct sk_buff *p, bool inner)
{
const u32 id = ntohl(*(__be32 *)&iph->id);
const u32 id2 = ntohl(*(__be32 *)&iph2->id);
const u16 ipid_offset = (id >> 16) - (id2 >> 16);
const u16 count = NAPI_GRO_CB(p)->count;
const u32 df = id & IP_DF;
- int flush;
/* All fields must match except length and checksum. */
- flush = (iph->ttl ^ iph2->ttl) | (iph->tos ^ iph2->tos) | (df ^ (id2 & IP_DF));
-
- if (flush | (outer && df))
- return flush;
+ if ((iph->ttl ^ iph2->ttl) | (iph->tos ^ iph2->tos) | (df ^ (id2 & IP_DF)))
+ return true;
/* When we receive our second frame we can make a decision on if we
* continue this flow as an atomic flow with a fixed ID or if we use
* an incrementing ID.
*/
if (count == 1 && df && !ipid_offset)
- NAPI_GRO_CB(p)->ip_fixedid = true;
+ NAPI_GRO_CB(p)->ip_fixedid |= 1 << inner;
- return ipid_offset ^ (count * !NAPI_GRO_CB(p)->ip_fixedid);
+ return ipid_offset ^ (count * !(NAPI_GRO_CB(p)->ip_fixedid & (1 << inner)));
}
static inline int ipv6_gro_flush(const struct ipv6hdr *iph, const struct ipv6hdr *iph2)
@@ -478,28 +475,30 @@ static inline int ipv6_gro_flush(const struct ipv6hdr *iph, const struct ipv6hdr
}
static inline int __gro_receive_network_flush(const void *th, const void *th2,
- struct sk_buff *p, const u16 diff,
- bool outer)
+ struct sk_buff *p, bool inner)
{
- const void *nh = th - diff;
- const void *nh2 = th2 - diff;
+ const void *nh, *nh2;
+ int off, diff;
+
+ off = skb_transport_offset(p);
+ diff = off - NAPI_GRO_CB(p)->network_offsets[inner];
+ nh = th - diff;
+ nh2 = th2 - diff;
if (((struct iphdr *)nh)->version == 6)
return ipv6_gro_flush(nh, nh2);
else
- return inet_gro_flush(nh, nh2, p, outer);
+ return inet_gro_flush(nh, nh2, p, inner);
}
static inline int gro_receive_network_flush(const void *th, const void *th2,
struct sk_buff *p)
{
- const bool encap_mark = NAPI_GRO_CB(p)->encap_mark;
- int off = skb_transport_offset(p);
int flush;
- flush = __gro_receive_network_flush(th, th2, p, off - NAPI_GRO_CB(p)->network_offset, encap_mark);
- if (encap_mark)
- flush |= __gro_receive_network_flush(th, th2, p, off - NAPI_GRO_CB(p)->inner_network_offset, false);
+ flush = __gro_receive_network_flush(th, th2, p, false);
+ if (NAPI_GRO_CB(p)->encap_mark)
+ flush |= __gro_receive_network_flush(th, th2, p, true);
return flush;
}
diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index be5c2294610e..56817ef12ad2 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -472,6 +472,7 @@ INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
const u16 offset = NAPI_GRO_CB(skb)->network_offsets[skb->encapsulation];
const struct iphdr *iph = (struct iphdr *)(skb->data + offset);
struct tcphdr *th = tcp_hdr(skb);
+ bool is_fixedid;
if (unlikely(NAPI_GRO_CB(skb)->is_flist)) {
skb_shinfo(skb)->gso_type |= SKB_GSO_FRAGLIST | SKB_GSO_TCPV4;
@@ -485,8 +486,10 @@ INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
th->check = ~tcp_v4_check(skb->len - thoff, iph->saddr,
iph->daddr, 0);
+ is_fixedid = (NAPI_GRO_CB(skb)->ip_fixedid >> skb->encapsulation) & 1;
+
skb_shinfo(skb)->gso_type |= SKB_GSO_TCPV4 |
- (NAPI_GRO_CB(skb)->ip_fixedid * SKB_GSO_TCP_FIXEDID);
+ (is_fixedid * SKB_GSO_TCP_FIXEDID);
tcp_gro_complete(skb);
return 0;
--
2.36.1On 8/21/25 9:30 AM, Richard Gobert wrote:
> @@ -442,29 +442,26 @@ static inline __wsum ip6_gro_compute_pseudo(const struct sk_buff *skb,
> }
>
> static inline int inet_gro_flush(const struct iphdr *iph, const struct iphdr *iph2,
> - struct sk_buff *p, bool outer)
> + struct sk_buff *p, bool inner)
> {
> const u32 id = ntohl(*(__be32 *)&iph->id);
> const u32 id2 = ntohl(*(__be32 *)&iph2->id);
> const u16 ipid_offset = (id >> 16) - (id2 >> 16);
> const u16 count = NAPI_GRO_CB(p)->count;
> const u32 df = id & IP_DF;
> - int flush;
>
> /* All fields must match except length and checksum. */
> - flush = (iph->ttl ^ iph2->ttl) | (iph->tos ^ iph2->tos) | (df ^ (id2 & IP_DF));
> -
> - if (flush | (outer && df))
> - return flush;
> + if ((iph->ttl ^ iph2->ttl) | (iph->tos ^ iph2->tos) | (df ^ (id2 & IP_DF)))
> + return true;
>
> /* When we receive our second frame we can make a decision on if we
> * continue this flow as an atomic flow with a fixed ID or if we use
> * an incrementing ID.
> */
> if (count == 1 && df && !ipid_offset)
> - NAPI_GRO_CB(p)->ip_fixedid = true;
> + NAPI_GRO_CB(p)->ip_fixedid |= 1 << inner;
>
> - return ipid_offset ^ (count * !NAPI_GRO_CB(p)->ip_fixedid);
> + return ipid_offset ^ (count * !(NAPI_GRO_CB(p)->ip_fixedid & (1 << inner)));
> }
>
> static inline int ipv6_gro_flush(const struct ipv6hdr *iph, const struct ipv6hdr *iph2)
> @@ -478,28 +475,30 @@ static inline int ipv6_gro_flush(const struct ipv6hdr *iph, const struct ipv6hdr
> }
>
> static inline int __gro_receive_network_flush(const void *th, const void *th2,
> - struct sk_buff *p, const u16 diff,
> - bool outer)
> + struct sk_buff *p, bool inner)
> {
> - const void *nh = th - diff;
> - const void *nh2 = th2 - diff;
> + const void *nh, *nh2;
> + int off, diff;
> +
> + off = skb_transport_offset(p);
> + diff = off - NAPI_GRO_CB(p)->network_offsets[inner];
> + nh = th - diff;
> + nh2 = th2 - diff;
>
> if (((struct iphdr *)nh)->version == 6)
> return ipv6_gro_flush(nh, nh2);
> else
> - return inet_gro_flush(nh, nh2, p, outer);
> + return inet_gro_flush(nh, nh2, p, inner);
> }
>
> static inline int gro_receive_network_flush(const void *th, const void *th2,
> struct sk_buff *p)
> {
> - const bool encap_mark = NAPI_GRO_CB(p)->encap_mark;
> - int off = skb_transport_offset(p);
> int flush;
>
> - flush = __gro_receive_network_flush(th, th2, p, off - NAPI_GRO_CB(p)->network_offset, encap_mark);
> - if (encap_mark)
> - flush |= __gro_receive_network_flush(th, th2, p, off - NAPI_GRO_CB(p)->inner_network_offset, false);
> + flush = __gro_receive_network_flush(th, th2, p, false);
> + if (NAPI_GRO_CB(p)->encap_mark)
> + flush |= __gro_receive_network_flush(th, th2, p, true);
Minor nit: I'm under the (unverified) impression that the old syntax
could help the compiler generating better code. What about storing the
diff in a local variable:
int diff;
diff = skb_transport_offset(p) - NAPI_GRO_CB(p)->network_offset;
flush = __gro_receive_network_flush(th, th2, diff, false);
if (NAPI_GRO_CB(p)->encap_mark)
flush |= __gro_receive_network_flush(th, th2, diff, true);
?
/P
On 8/22/25 10:26 AM, Paolo Abeni wrote:
> On 8/21/25 9:30 AM, Richard Gobert wrote:
>> @@ -442,29 +442,26 @@ static inline __wsum ip6_gro_compute_pseudo(const struct sk_buff *skb,
>> }
>>
>> static inline int inet_gro_flush(const struct iphdr *iph, const struct iphdr *iph2,
>> - struct sk_buff *p, bool outer)
>> + struct sk_buff *p, bool inner)
>> {
>> const u32 id = ntohl(*(__be32 *)&iph->id);
>> const u32 id2 = ntohl(*(__be32 *)&iph2->id);
>> const u16 ipid_offset = (id >> 16) - (id2 >> 16);
>> const u16 count = NAPI_GRO_CB(p)->count;
>> const u32 df = id & IP_DF;
>> - int flush;
>>
>> /* All fields must match except length and checksum. */
>> - flush = (iph->ttl ^ iph2->ttl) | (iph->tos ^ iph2->tos) | (df ^ (id2 & IP_DF));
>> -
>> - if (flush | (outer && df))
>> - return flush;
>> + if ((iph->ttl ^ iph2->ttl) | (iph->tos ^ iph2->tos) | (df ^ (id2 & IP_DF)))
>> + return true;
>>
>> /* When we receive our second frame we can make a decision on if we
>> * continue this flow as an atomic flow with a fixed ID or if we use
>> * an incrementing ID.
>> */
>> if (count == 1 && df && !ipid_offset)
>> - NAPI_GRO_CB(p)->ip_fixedid = true;
>> + NAPI_GRO_CB(p)->ip_fixedid |= 1 << inner;
>>
>> - return ipid_offset ^ (count * !NAPI_GRO_CB(p)->ip_fixedid);
>> + return ipid_offset ^ (count * !(NAPI_GRO_CB(p)->ip_fixedid & (1 << inner)));
>> }
>>
>> static inline int ipv6_gro_flush(const struct ipv6hdr *iph, const struct ipv6hdr *iph2)
>> @@ -478,28 +475,30 @@ static inline int ipv6_gro_flush(const struct ipv6hdr *iph, const struct ipv6hdr
>> }
>>
>> static inline int __gro_receive_network_flush(const void *th, const void *th2,
>> - struct sk_buff *p, const u16 diff,
>> - bool outer)
>> + struct sk_buff *p, bool inner)
>> {
>> - const void *nh = th - diff;
>> - const void *nh2 = th2 - diff;
>> + const void *nh, *nh2;
>> + int off, diff;
>> +
>> + off = skb_transport_offset(p);
>> + diff = off - NAPI_GRO_CB(p)->network_offsets[inner];
>> + nh = th - diff;
>> + nh2 = th2 - diff;
>>
>> if (((struct iphdr *)nh)->version == 6)
>> return ipv6_gro_flush(nh, nh2);
>> else
>> - return inet_gro_flush(nh, nh2, p, outer);
>> + return inet_gro_flush(nh, nh2, p, inner);
>> }
>>
>> static inline int gro_receive_network_flush(const void *th, const void *th2,
>> struct sk_buff *p)
>> {
>> - const bool encap_mark = NAPI_GRO_CB(p)->encap_mark;
>> - int off = skb_transport_offset(p);
>> int flush;
>>
>> - flush = __gro_receive_network_flush(th, th2, p, off - NAPI_GRO_CB(p)->network_offset, encap_mark);
>> - if (encap_mark)
>> - flush |= __gro_receive_network_flush(th, th2, p, off - NAPI_GRO_CB(p)->inner_network_offset, false);
>> + flush = __gro_receive_network_flush(th, th2, p, false);
>> + if (NAPI_GRO_CB(p)->encap_mark)
>> + flush |= __gro_receive_network_flush(th, th2, p, true);
>
> Minor nit: I'm under the (unverified) impression that the old syntax
> could help the compiler generating better code. What about storing the
> diff in a local variable:
>
> int diff;
>
> diff = skb_transport_offset(p) - NAPI_GRO_CB(p)->network_offset;
> flush = __gro_receive_network_flush(th, th2, diff, false);
> if (NAPI_GRO_CB(p)->encap_mark)
> flush |= __gro_receive_network_flush(th, th2, diff, true);
whoops, I rushed the above. I mean:
diff = off - NAPI_GRO_CB(p)->network_offset;
flush = __gro_receive_network_flush(th, th2, p, diff, false);
if (NAPI_GRO_CB(p)->encap_mark) {
diff = off - NAPI_GRO_CB(p)->inner network_offset;
flush |= __gro_receive_network_flush(th, th2, diff, true);
}
/P
© 2016 - 2025 Red Hat, Inc.