* [PATCH net-next 0/3] net: gro: encapsulation bug fix and flush checks improvements
@ 2024-02-22 21:00 Richard Gobert
2024-02-22 21:05 ` [PATCH net-next 1/3] net: gro: set {inner_,}network_header in receive phase Richard Gobert
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Richard Gobert @ 2024-02-22 21:00 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, dsahern, shuah, liujian56, horms,
aleksander.lobakin, richardbgobert, linyunsheng, therbert, netdev,
linux-kernel, linux-kselftest
This series fixes a bug in the complete phase of UDP in GRO, in which
socket lookup fails due to using network_header when parsing encapsulated
packets. The fix is to keep track of both outer and inner offsets.
The last commit leverages the first commit to remove some state from
napi_gro_cb, and stateful code in {ipv6,inet}_gro_receive which may be
unnecessarily complicated due to encapsulation support in GRO.
In addition, udpgro_fwd selftest is adjusted to include the socket lookup
case for vxlan. This selftest will test its supposed functionality once
local bind support is merged (https://lore.kernel.org/netdev/df300a49-7811-4126-a56a-a77100c8841b@gmail.com/).
Richard Gobert (3):
net: gro: set {inner_,}network_header in receive phase
selftests/net: add local address bind in vxlan selftest
net: gro: move L3 flush checks to tcp_gro_receive
include/net/gro.h | 23 ++++---
net/8021q/vlan_core.c | 3 +
net/core/gro.c | 3 -
net/ipv4/af_inet.c | 44 ++------------
net/ipv4/tcp_offload.c | 73 ++++++++++++++++++-----
net/ipv4/udp_offload.c | 2 +-
net/ipv6/ip6_offload.c | 22 ++-----
net/ipv6/tcpv6_offload.c | 2 +-
net/ipv6/udp_offload.c | 2 +-
tools/testing/selftests/net/udpgro_fwd.sh | 10 +++-
10 files changed, 97 insertions(+), 87 deletions(-)
--
2.36.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH net-next 1/3] net: gro: set {inner_,}network_header in receive phase 2024-02-22 21:00 [PATCH net-next 0/3] net: gro: encapsulation bug fix and flush checks improvements Richard Gobert @ 2024-02-22 21:05 ` Richard Gobert 2024-02-22 21:25 ` Eric Dumazet 2024-02-22 21:07 ` [PATCH net-next 2/3] selftests/net: add local address bind in vxlan selftest Richard Gobert 2024-02-22 21:12 ` [PATCH net-next 3/3] net: gro: move L3 flush checks to tcp_gro_receive Richard Gobert 2 siblings, 1 reply; 8+ messages in thread From: Richard Gobert @ 2024-02-22 21:05 UTC (permalink / raw) To: davem, edumazet, kuba, pabeni, dsahern, shuah, liujian56, horms, aleksander.lobakin, linyunsheng, therbert, netdev, linux-kernel, linux-kselftest Commits a602456 ("udp: Add GRO functions to UDP socket") and 57c67ff ("udp: additional GRO support") introduce incorrect usage of {ip,ipv6}_hdr in the complete phase of gro. The functions always return skb->network_header, which in the case of encapsulated packets at the gro complete phase, is always set to the innermost L3 of the packet. That means that calling {ip,ipv6}_hdr for skbs which completed the GRO receive phase (both in gro_list and *_gro_complete) when parsing an encapsulated packet's _outer_ L3/L4 may return an unexpected value. This incorrect usage leads to a bug in GRO's UDP socket lookup. udp{4,6}_lib_lookup_skb functions use ip_hdr/ipv6_hdr respectively. These *_hdr functions return network_header which will point to the innermost L3, resulting in the wrong offset being used in __udp{4,6}_lib_lookup with encapsulated packets. Reproduction example: Endpoint configuration example (fou + local address bind) # ip fou add port 6666 ipproto 4 # ip link add name tun1 type ipip remote 2.2.2.1 local 2.2.2.2 encap fou encap-dport 5555 encap-sport 6666 mode ipip # ip link set tun1 up # ip a add 1.1.1.2/24 dev tun1 Netperf TCP_STREAM result on net-next before patch is applied: net-next main, GRO enabled: $ netperf -H 1.1.1.2 -t TCP_STREAM -l 5 Recv Send Send Socket Socket Message Elapsed Size Size Size Time Throughput bytes bytes bytes secs. 10^6bits/sec 131072 16384 16384 5.28 2.37 net-next main, GRO disabled: $ netperf -H 1.1.1.2 -t TCP_STREAM -l 5 Recv Send Send Socket Socket Message Elapsed Size Size Size Time Throughput bytes bytes bytes secs. 10^6bits/sec 131072 16384 16384 5.01 2745.06 patch applied, GRO enabled: $ netperf -H 1.1.1.2 -t TCP_STREAM -l 5 Recv Send Send Socket Socket Message Elapsed Size Size Size Time Throughput bytes bytes bytes secs. 10^6bits/sec 131072 16384 16384 5.01 2877.38 This patch fixes this bug and prevents similar future misuse of network_header by setting network_header and inner_network_header to their respective values during the receive phase of GRO. This results in more coherent {inner_,}network_header values for every skb in gro_list, which also means there's no need to set/fix these values before passing the packet forward. network_header is already set in dev_gro_receive and under encapsulation we set inner_network_header. *_gro_complete functions use a new helper function - skb_gro_complete_network_header, which returns the network_header/inner_network_header offset during the GRO complete phase, depending on skb->encapsulation. Fixes: 57c67ff4bd92 ("udp: additional GRO support") Signed-off-by: Richard Gobert <richardbgobert@gmail.com> --- include/net/gro.h | 14 +++++++++++++- net/8021q/vlan_core.c | 3 +++ net/ipv4/af_inet.c | 8 ++++---- net/ipv4/tcp_offload.c | 2 +- net/ipv4/udp_offload.c | 2 +- net/ipv6/ip6_offload.c | 11 +++++------ net/ipv6/tcpv6_offload.c | 2 +- net/ipv6/udp_offload.c | 2 +- 8 files changed, 29 insertions(+), 15 deletions(-) diff --git a/include/net/gro.h b/include/net/gro.h index b435f0ddbf64..89502a7e35ed 100644 --- a/include/net/gro.h +++ b/include/net/gro.h @@ -177,10 +177,22 @@ static inline void *skb_gro_header(struct sk_buff *skb, return ptr; } +static inline int skb_gro_network_offset(struct sk_buff *skb) +{ + return NAPI_GRO_CB(skb)->encap_mark ? skb_inner_network_offset(skb) : + skb_network_offset(skb); +} + static inline void *skb_gro_network_header(struct sk_buff *skb) { return (NAPI_GRO_CB(skb)->frag0 ?: skb->data) + - skb_network_offset(skb); + skb_gro_network_offset(skb); +} + +static inline void *skb_gro_complete_network_header(struct sk_buff *skb) +{ + return skb->encapsulation ? skb_inner_network_header(skb) : + skb_network_header(skb); } static inline __wsum inet_gro_compute_pseudo(struct sk_buff *skb, int proto) diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c index f00158234505..8bc871397e47 100644 --- a/net/8021q/vlan_core.c +++ b/net/8021q/vlan_core.c @@ -478,6 +478,9 @@ static struct sk_buff *vlan_gro_receive(struct list_head *head, if (unlikely(!vhdr)) goto out; + if (!NAPI_GRO_CB(skb)->encap_mark) + skb_set_network_header(skb, hlen); + type = vhdr->h_vlan_encapsulated_proto; ptype = gro_find_receive_by_type(type); diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index 835f4f9d98d2..c0f3c162bf73 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -1564,7 +1564,9 @@ struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb) NAPI_GRO_CB(skb)->is_atomic = !!(iph->frag_off & htons(IP_DF)); NAPI_GRO_CB(skb)->flush |= flush; - skb_set_network_header(skb, off); + if (NAPI_GRO_CB(skb)->encap_mark) + skb_set_inner_network_header(skb, off); + /* The above will be needed by the transport layer if there is one * immediately following this IP hdr. */ @@ -1643,10 +1645,8 @@ int inet_gro_complete(struct sk_buff *skb, int nhoff) int proto = iph->protocol; int err = -ENOSYS; - if (skb->encapsulation) { + if (skb->encapsulation) skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IP)); - skb_set_inner_network_header(skb, nhoff); - } iph_set_totlen(iph, skb->len - nhoff); csum_replace2(&iph->check, totlen, iph->tot_len); diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c index 8311c38267b5..8bbcd3f502ac 100644 --- a/net/ipv4/tcp_offload.c +++ b/net/ipv4/tcp_offload.c @@ -330,7 +330,7 @@ struct sk_buff *tcp4_gro_receive(struct list_head *head, struct sk_buff *skb) INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff) { - const struct iphdr *iph = ip_hdr(skb); + const struct iphdr *iph = skb_gro_complete_network_header(skb); struct tcphdr *th = tcp_hdr(skb); th->check = ~tcp_v4_check(skb->len - thoff, iph->saddr, diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index 6c95d28d0c4a..7f59cede67f5 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -709,7 +709,7 @@ EXPORT_SYMBOL(udp_gro_complete); INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff) { - const struct iphdr *iph = ip_hdr(skb); + const struct iphdr *iph = skb_gro_complete_network_header(skb); struct udphdr *uh = (struct udphdr *)(skb->data + nhoff); /* do fraglist only if there is no outer UDP encap (or we already processed it) */ diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c index cca64c7809be..db7e3db587b9 100644 --- a/net/ipv6/ip6_offload.c +++ b/net/ipv6/ip6_offload.c @@ -67,7 +67,7 @@ static int ipv6_gro_pull_exthdrs(struct sk_buff *skb, int off, int proto) off += len; } - skb_gro_pull(skb, off - skb_network_offset(skb)); + skb_gro_pull(skb, off - skb_gro_network_offset(skb)); return proto; } @@ -236,7 +236,8 @@ INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head, if (unlikely(!iph)) goto out; - skb_set_network_header(skb, off); + if (NAPI_GRO_CB(skb)->encap_mark) + skb_set_inner_network_header(skb, off); flush += ntohs(iph->payload_len) != skb->len - hlen; @@ -259,7 +260,7 @@ INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head, NAPI_GRO_CB(skb)->proto = proto; flush--; - nlen = skb_network_header_len(skb); + nlen = skb_gro_offset(skb) - off; list_for_each_entry(p, head, list) { const struct ipv6hdr *iph2; @@ -353,10 +354,8 @@ INDIRECT_CALLABLE_SCOPE int ipv6_gro_complete(struct sk_buff *skb, int nhoff) int err = -ENOSYS; u32 payload_len; - if (skb->encapsulation) { + if (skb->encapsulation) skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IPV6)); - skb_set_inner_network_header(skb, nhoff); - } payload_len = skb->len - nhoff - sizeof(*iph); if (unlikely(payload_len > IPV6_MAXPLEN)) { diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c index bf0c957e4b5e..79eeaced2834 100644 --- a/net/ipv6/tcpv6_offload.c +++ b/net/ipv6/tcpv6_offload.c @@ -29,7 +29,7 @@ struct sk_buff *tcp6_gro_receive(struct list_head *head, struct sk_buff *skb) INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int thoff) { - const struct ipv6hdr *iph = ipv6_hdr(skb); + const struct ipv6hdr *iph = skb_gro_complete_network_header(skb); struct tcphdr *th = tcp_hdr(skb); th->check = ~tcp_v6_check(skb->len - thoff, &iph->saddr, diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c index 6b95ba241ebe..897caa2e39fb 100644 --- a/net/ipv6/udp_offload.c +++ b/net/ipv6/udp_offload.c @@ -164,7 +164,7 @@ struct sk_buff *udp6_gro_receive(struct list_head *head, struct sk_buff *skb) INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff) { - const struct ipv6hdr *ipv6h = ipv6_hdr(skb); + const struct ipv6hdr *ipv6h = skb_gro_complete_network_header(skb); struct udphdr *uh = (struct udphdr *)(skb->data + nhoff); /* do fraglist only if there is no outer UDP encap (or we already processed it) */ -- 2.36.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 1/3] net: gro: set {inner_,}network_header in receive phase 2024-02-22 21:05 ` [PATCH net-next 1/3] net: gro: set {inner_,}network_header in receive phase Richard Gobert @ 2024-02-22 21:25 ` Eric Dumazet 2024-02-29 13:22 ` Richard Gobert 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2024-02-22 21:25 UTC (permalink / raw) To: Richard Gobert Cc: davem, kuba, pabeni, dsahern, shuah, liujian56, horms, aleksander.lobakin, linyunsheng, therbert, netdev, linux-kernel, linux-kselftest On Thu, Feb 22, 2024 at 10:05 PM Richard Gobert <richardbgobert@gmail.com> wrote: > > Commits a602456 ("udp: Add GRO functions to UDP socket") and 57c67ff ("udp: > additional GRO support") introduce incorrect usage of {ip,ipv6}_hdr in the > complete phase of gro. The functions always return skb->network_header, > which in the case of encapsulated packets at the gro complete phase, is > always set to the innermost L3 of the packet. That means that calling > {ip,ipv6}_hdr for skbs which completed the GRO receive phase (both in > gro_list and *_gro_complete) when parsing an encapsulated packet's _outer_ > L3/L4 may return an unexpected value. > > This incorrect usage leads to a bug in GRO's UDP socket lookup. > udp{4,6}_lib_lookup_skb functions use ip_hdr/ipv6_hdr respectively. These > *_hdr functions return network_header which will point to the innermost L3, > resulting in the wrong offset being used in __udp{4,6}_lib_lookup with > encapsulated packets. > > Reproduction example: > > Endpoint configuration example (fou + local address bind) > > # ip fou add port 6666 ipproto 4 > # ip link add name tun1 type ipip remote 2.2.2.1 local 2.2.2.2 encap fou encap-dport 5555 encap-sport 6666 mode ipip > # ip link set tun1 up > # ip a add 1.1.1.2/24 dev tun1 > > Netperf TCP_STREAM result on net-next before patch is applied: > > net-next main, GRO enabled: > $ netperf -H 1.1.1.2 -t TCP_STREAM -l 5 > Recv Send Send > Socket Socket Message Elapsed > Size Size Size Time Throughput > bytes bytes bytes secs. 10^6bits/sec > > 131072 16384 16384 5.28 2.37 > > net-next main, GRO disabled: > $ netperf -H 1.1.1.2 -t TCP_STREAM -l 5 > Recv Send Send > Socket Socket Message Elapsed > Size Size Size Time Throughput > bytes bytes bytes secs. 10^6bits/sec > > 131072 16384 16384 5.01 2745.06 > > patch applied, GRO enabled: > $ netperf -H 1.1.1.2 -t TCP_STREAM -l 5 > Recv Send Send > Socket Socket Message Elapsed > Size Size Size Time Throughput > bytes bytes bytes secs. 10^6bits/sec > > 131072 16384 16384 5.01 2877.38 > > This patch fixes this bug and prevents similar future misuse of > network_header by setting network_header and inner_network_header to their > respective values during the receive phase of GRO. This results in > more coherent {inner_,}network_header values for every skb in gro_list, > which also means there's no need to set/fix these values before passing > the packet forward. > > network_header is already set in dev_gro_receive and under encapsulation we > set inner_network_header. *_gro_complete functions use a new helper > function - skb_gro_complete_network_header, which returns the > network_header/inner_network_header offset during the GRO complete phase, > depending on skb->encapsulation. > > Fixes: 57c67ff4bd92 ("udp: additional GRO support") > Signed-off-by: Richard Gobert <richardbgobert@gmail.com> > --- > include/net/gro.h | 14 +++++++++++++- > net/8021q/vlan_core.c | 3 +++ > net/ipv4/af_inet.c | 8 ++++---- > net/ipv4/tcp_offload.c | 2 +- > net/ipv4/udp_offload.c | 2 +- > net/ipv6/ip6_offload.c | 11 +++++------ > net/ipv6/tcpv6_offload.c | 2 +- > net/ipv6/udp_offload.c | 2 +- > 8 files changed, 29 insertions(+), 15 deletions(-) > > diff --git a/include/net/gro.h b/include/net/gro.h > index b435f0ddbf64..89502a7e35ed 100644 > --- a/include/net/gro.h > +++ b/include/net/gro.h > @@ -177,10 +177,22 @@ static inline void *skb_gro_header(struct sk_buff *skb, > return ptr; > } > > +static inline int skb_gro_network_offset(struct sk_buff *skb) > +{ > + return NAPI_GRO_CB(skb)->encap_mark ? skb_inner_network_offset(skb) : > + skb_network_offset(skb); > +} > + > static inline void *skb_gro_network_header(struct sk_buff *skb) > { > return (NAPI_GRO_CB(skb)->frag0 ?: skb->data) + > - skb_network_offset(skb); > + skb_gro_network_offset(skb); > +} > + > +static inline void *skb_gro_complete_network_header(struct sk_buff *skb) > +{ > + return skb->encapsulation ? skb_inner_network_header(skb) : > + skb_network_header(skb); > } > > static inline __wsum inet_gro_compute_pseudo(struct sk_buff *skb, int proto) > diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c > index f00158234505..8bc871397e47 100644 > --- a/net/8021q/vlan_core.c > +++ b/net/8021q/vlan_core.c > @@ -478,6 +478,9 @@ static struct sk_buff *vlan_gro_receive(struct list_head *head, > if (unlikely(!vhdr)) > goto out; > > + if (!NAPI_GRO_CB(skb)->encap_mark) > + skb_set_network_header(skb, hlen); > + > type = vhdr->h_vlan_encapsulated_proto; > > ptype = gro_find_receive_by_type(type); > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c > index 835f4f9d98d2..c0f3c162bf73 100644 > --- a/net/ipv4/af_inet.c > +++ b/net/ipv4/af_inet.c > @@ -1564,7 +1564,9 @@ struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb) > > NAPI_GRO_CB(skb)->is_atomic = !!(iph->frag_off & htons(IP_DF)); > NAPI_GRO_CB(skb)->flush |= flush; > - skb_set_network_header(skb, off); > + if (NAPI_GRO_CB(skb)->encap_mark) > + skb_set_inner_network_header(skb, off); > + > /* The above will be needed by the transport layer if there is one > * immediately following this IP hdr. > */ > @@ -1643,10 +1645,8 @@ int inet_gro_complete(struct sk_buff *skb, int nhoff) > int proto = iph->protocol; > int err = -ENOSYS; > > - if (skb->encapsulation) { > + if (skb->encapsulation) > skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IP)); > - skb_set_inner_network_header(skb, nhoff); > - } > > iph_set_totlen(iph, skb->len - nhoff); > csum_replace2(&iph->check, totlen, iph->tot_len); > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c > index 8311c38267b5..8bbcd3f502ac 100644 > --- a/net/ipv4/tcp_offload.c > +++ b/net/ipv4/tcp_offload.c > @@ -330,7 +330,7 @@ struct sk_buff *tcp4_gro_receive(struct list_head *head, struct sk_buff *skb) > > INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff) > { > - const struct iphdr *iph = ip_hdr(skb); > + const struct iphdr *iph = skb_gro_complete_network_header(skb); > struct tcphdr *th = tcp_hdr(skb); > > th->check = ~tcp_v4_check(skb->len - thoff, iph->saddr, > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > index 6c95d28d0c4a..7f59cede67f5 100644 > --- a/net/ipv4/udp_offload.c > +++ b/net/ipv4/udp_offload.c > @@ -709,7 +709,7 @@ EXPORT_SYMBOL(udp_gro_complete); > > INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff) > { > - const struct iphdr *iph = ip_hdr(skb); > + const struct iphdr *iph = skb_gro_complete_network_header(skb); > struct udphdr *uh = (struct udphdr *)(skb->data + nhoff); > > /* do fraglist only if there is no outer UDP encap (or we already processed it) */ > diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c > index cca64c7809be..db7e3db587b9 100644 > --- a/net/ipv6/ip6_offload.c > +++ b/net/ipv6/ip6_offload.c > @@ -67,7 +67,7 @@ static int ipv6_gro_pull_exthdrs(struct sk_buff *skb, int off, int proto) > off += len; > } > > - skb_gro_pull(skb, off - skb_network_offset(skb)); > + skb_gro_pull(skb, off - skb_gro_network_offset(skb)); > return proto; > } > > @@ -236,7 +236,8 @@ INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head, > if (unlikely(!iph)) > goto out; > > - skb_set_network_header(skb, off); > + if (NAPI_GRO_CB(skb)->encap_mark) > + skb_set_inner_network_header(skb, off); > > flush += ntohs(iph->payload_len) != skb->len - hlen; > > @@ -259,7 +260,7 @@ INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head, > NAPI_GRO_CB(skb)->proto = proto; > > flush--; > - nlen = skb_network_header_len(skb); > + nlen = skb_gro_offset(skb) - off; > > list_for_each_entry(p, head, list) { > const struct ipv6hdr *iph2; > @@ -353,10 +354,8 @@ INDIRECT_CALLABLE_SCOPE int ipv6_gro_complete(struct sk_buff *skb, int nhoff) > int err = -ENOSYS; > u32 payload_len; > > - if (skb->encapsulation) { > + if (skb->encapsulation) > skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IPV6)); > - skb_set_inner_network_header(skb, nhoff); > - } > > payload_len = skb->len - nhoff - sizeof(*iph); > if (unlikely(payload_len > IPV6_MAXPLEN)) { > diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c > index bf0c957e4b5e..79eeaced2834 100644 > --- a/net/ipv6/tcpv6_offload.c > +++ b/net/ipv6/tcpv6_offload.c > @@ -29,7 +29,7 @@ struct sk_buff *tcp6_gro_receive(struct list_head *head, struct sk_buff *skb) > > INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int thoff) > { > - const struct ipv6hdr *iph = ipv6_hdr(skb); > + const struct ipv6hdr *iph = skb_gro_complete_network_header(skb); > struct tcphdr *th = tcp_hdr(skb); > > th->check = ~tcp_v6_check(skb->len - thoff, &iph->saddr, > diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c > index 6b95ba241ebe..897caa2e39fb 100644 > --- a/net/ipv6/udp_offload.c > +++ b/net/ipv6/udp_offload.c > @@ -164,7 +164,7 @@ struct sk_buff *udp6_gro_receive(struct list_head *head, struct sk_buff *skb) > > INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff) > { > - const struct ipv6hdr *ipv6h = ipv6_hdr(skb); > + const struct ipv6hdr *ipv6h = skb_gro_complete_network_header(skb); > struct udphdr *uh = (struct udphdr *)(skb->data + nhoff); My intuition is that this patch has a high cost for normal GRO processing. SW-GRO is already a bottleneck on ARM cores in smart NICS. I would suggest instead using parameters to give both the nhoff and thoff values this would avoid many conditionals in the fast path. -> INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff, int thoff) { const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)(skb->data + nhoff); struct udphdr *uh = (struct udphdr *)(skb->data + thoff); ... } INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int nhoff, int thoff) { const struct ipv6hdr *iph = (const struct ipv6hdr *)(skb->data + nhoff); struct tcphdr *th = (struct tcphdr *)(skb->data + thoff); Why storing in skb fields things that really could be propagated more efficiently as function parameters ? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 1/3] net: gro: set {inner_,}network_header in receive phase 2024-02-22 21:25 ` Eric Dumazet @ 2024-02-29 13:22 ` Richard Gobert 2024-02-29 20:15 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: Richard Gobert @ 2024-02-29 13:22 UTC (permalink / raw) To: Eric Dumazet Cc: davem, kuba, pabeni, dsahern, shuah, liujian56, horms, aleksander.lobakin, linyunsheng, therbert, netdev, linux-kernel, linux-kselftest Eric Dumazet wrote: > > My intuition is that this patch has a high cost for normal GRO processing. > SW-GRO is already a bottleneck on ARM cores in smart NICS. > > I would suggest instead using parameters to give both the nhoff and thoff values > this would avoid many conditionals in the fast path. > > -> > > INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int > nhoff, int thoff) > { > const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)(skb->data + nhoff); > struct udphdr *uh = (struct udphdr *)(skb->data + thoff); > ... > } > > INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int > nhoff, int thoff) > { > const struct ipv6hdr *iph = (const struct ipv6hdr *)(skb->data + nhoff); > struct tcphdr *th = (struct tcphdr *)(skb->data + thoff); > > Why storing in skb fields things that really could be propagated more > efficiently as function parameters ? Hi Eric, Thanks for the review! I agree, the conditionals could be a problem and are actually not needed. The third commit in this patch series introduces an optimisation for ipv6/ipv4 using the correct {inner_}network_header. We can remove the conditionals; I thought about multiple ways to do so. First, remove the conditional in skb_gro_network_offset: static inline int skb_gro_network_offset(const struct sk_buff *skb) { const u32 mask = NAPI_GRO_CB(skb)->encap_mark - 1; return (skb_network_offset(skb) & mask) | (skb_inner_network_offset(skb) & ~mask); } And for the conditionals in {inet,ipv6}_gro_receive I thought about two ideas. The first is to move set_inner_network_header to encapsulation gro functions like ipip_gro_receive, this way there's one less write (in comparison to main) in these functions: static struct sk_buff *ipip_gro_receive(struct list_head *head, struct sk_buff *skb) { ... NAPI_GRO_CB(skb)->encap_mark = 1; skb_set_inner_network_header(skb, skb_gro_offset(skb)); The second way is to always write to inner_network_header: INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head, struct sk_buff *skb) { ... skb_set_inner_network_header(skb, off); ... What do you think is better? I think the 1st is more beneficial for the fast path. We could then use the {inner_}network_header separation to optimise the receive path, such as in the 3rd commit in this patch series. Regards, Richard ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 1/3] net: gro: set {inner_,}network_header in receive phase 2024-02-29 13:22 ` Richard Gobert @ 2024-02-29 20:15 ` Eric Dumazet 2024-03-01 15:02 ` Richard Gobert 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2024-02-29 20:15 UTC (permalink / raw) To: Richard Gobert Cc: davem, kuba, pabeni, dsahern, shuah, liujian56, horms, aleksander.lobakin, linyunsheng, therbert, netdev, linux-kernel, linux-kselftest On Thu, Feb 29, 2024 at 2:22 PM Richard Gobert <richardbgobert@gmail.com> wrote: > > > > Eric Dumazet wrote: > > > > My intuition is that this patch has a high cost for normal GRO processing. > > SW-GRO is already a bottleneck on ARM cores in smart NICS. > > > > I would suggest instead using parameters to give both the nhoff and thoff values > > this would avoid many conditionals in the fast path. > > > > -> > > > > INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int > > nhoff, int thoff) > > { > > const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)(skb->data + nhoff); > > struct udphdr *uh = (struct udphdr *)(skb->data + thoff); > > ... > > } > > > > INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int > > nhoff, int thoff) > > { > > const struct ipv6hdr *iph = (const struct ipv6hdr *)(skb->data + nhoff); > > struct tcphdr *th = (struct tcphdr *)(skb->data + thoff); > > > > Why storing in skb fields things that really could be propagated more > > efficiently as function parameters ? > > Hi Eric, > Thanks for the review! > > I agree, the conditionals could be a problem and are actually not needed. > The third commit in this patch series introduces an optimisation for > ipv6/ipv4 using the correct {inner_}network_header. We can remove the > conditionals; I thought about multiple ways to do so. First, remove the > conditional in skb_gro_network_offset: > > static inline int skb_gro_network_offset(const struct sk_buff *skb) > { > const u32 mask = NAPI_GRO_CB(skb)->encap_mark - 1; > return (skb_network_offset(skb) & mask) | (skb_inner_network_offset(skb) & ~mask); > } I was trying to say that we do not need all these helpers, storing state in NAPI_GRO_CB(skb), dirtying cache lines... Ideally, the skb network/transport/... headers could be set at the last stage, in gro_complete(big_gro_skb), instead of doing this for each segment. All the gro_receive() could be much faster by using additional parameters (nhoff, thoff) skb_gro_offset() could be replaced by the current offset (nhoff or other name), passed as a parameter. Here is a WIP for gro_complete() step, this looks large but this is only adding a 2nd 'offset' parameter Prior offset (typically network offset), called p_off Old argument nhoff, (renamed thoff if that makes sense), pointing to the current offset. drivers/net/geneve.c | 6 +++--- drivers/net/vxlan/vxlan_core.c | 11 +++++++---- include/linux/etherdevice.h | 2 +- include/linux/netdevice.h | 2 +- include/linux/udp.h | 2 +- include/net/gro.h | 10 +++++----- include/net/inet_common.h | 2 +- include/net/tcp.h | 4 ++-- include/net/udp.h | 8 ++++---- include/net/udp_tunnel.h | 2 +- net/8021q/vlan_core.c | 4 ++-- net/core/gro.c | 2 +- net/ethernet/eth.c | 4 ++-- net/ipv4/af_inet.c | 8 ++++---- net/ipv4/fou_core.c | 9 +++++---- net/ipv4/gre_offload.c | 4 ++-- net/ipv4/tcp_offload.c | 6 +++--- net/ipv4/udp.c | 3 ++- net/ipv4/udp_offload.c | 24 ++++++++++++------------ net/ipv6/ip6_offload.c | 22 ++++++++++++---------- net/ipv6/tcpv6_offload.c | 7 ++++--- net/ipv6/udp.c | 3 ++- net/ipv6/udp_offload.c | 12 ++++++------ 23 files changed, 83 insertions(+), 74 deletions(-) diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c index 6f3f9b446b1d202f6c71a20ce48088691e9120bf..af8dfdd47ffdeb7bdea302c5957e81faf83b19db 100644 --- a/drivers/net/geneve.c +++ b/drivers/net/geneve.c @@ -546,7 +546,7 @@ static struct sk_buff *geneve_gro_receive(struct sock *sk, } static int geneve_gro_complete(struct sock *sk, struct sk_buff *skb, - int nhoff) + int p_off, int nhoff) { struct genevehdr *gh; struct packet_offload *ptype; @@ -560,11 +560,11 @@ static int geneve_gro_complete(struct sock *sk, struct sk_buff *skb, /* since skb->encapsulation is set, eth_gro_complete() sets the inner mac header */ if (likely(type == htons(ETH_P_TEB))) - return eth_gro_complete(skb, nhoff + gh_len); + return eth_gro_complete(skb, p_off, nhoff + gh_len); ptype = gro_find_complete_by_type(type); if (ptype) - err = ptype->callbacks.gro_complete(skb, nhoff + gh_len); + err = ptype->callbacks.gro_complete(skb, p_off, nhoff + gh_len); skb_set_inner_mac_header(skb, nhoff + gh_len); diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c index 386cbe4d33272791e80470bd1378731d0c3b4d3b..84c123405b70f986a40b9f531e826807bcfc880b 100644 --- a/drivers/net/vxlan/vxlan_core.c +++ b/drivers/net/vxlan/vxlan_core.c @@ -767,15 +767,17 @@ static struct sk_buff *vxlan_gpe_gro_receive(struct sock *sk, return pp; } -static int vxlan_gro_complete(struct sock *sk, struct sk_buff *skb, int nhoff) +static int vxlan_gro_complete(struct sock *sk, struct sk_buff *skb, + int p_off, int nhoff) { /* Sets 'skb->inner_mac_header' since we are always called with * 'skb->encapsulation' set. */ - return eth_gro_complete(skb, nhoff + sizeof(struct vxlanhdr)); + return eth_gro_complete(skb, p_off, nhoff + sizeof(struct vxlanhdr)); } -static int vxlan_gpe_gro_complete(struct sock *sk, struct sk_buff *skb, int nhoff) +static int vxlan_gpe_gro_complete(struct sock *sk, struct sk_buff *skb, + int p_off, int nhoff) { struct vxlanhdr *vh = (struct vxlanhdr *)(skb->data + nhoff); const struct packet_offload *ptype; @@ -786,7 +788,8 @@ static int vxlan_gpe_gro_complete(struct sock *sk, struct sk_buff *skb, int nhof return err; ptype = gro_find_complete_by_type(protocol); if (ptype) - err = ptype->callbacks.gro_complete(skb, nhoff + sizeof(struct vxlanhdr)); + err = ptype->callbacks.gro_complete(skb, p_off, nhoff + + sizeof(struct vxlanhdr)); return err; } diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h index 224645f17c333b2311573197a28b41701eb35f92..b081b43d9686a1f3b0ddc9d84e66566e297a2d67 100644 --- a/include/linux/etherdevice.h +++ b/include/linux/etherdevice.h @@ -64,7 +64,7 @@ struct net_device *devm_alloc_etherdev_mqs(struct device *dev, int sizeof_priv, #define devm_alloc_etherdev(dev, sizeof_priv) devm_alloc_etherdev_mqs(dev, sizeof_priv, 1, 1) struct sk_buff *eth_gro_receive(struct list_head *head, struct sk_buff *skb); -int eth_gro_complete(struct sk_buff *skb, int nhoff); +int eth_gro_complete(struct sk_buff *skb, int p_off, int nhoff); /* Reserved Ethernet Addresses per IEEE 802.1Q */ static const u8 eth_reserved_addr_base[ETH_ALEN] __aligned(2) = diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 09023e44db4e2c3a2133afc52ba5a335d6030646..b21745f6233a2b56a64adda9f303d4db22019a37 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2788,7 +2788,7 @@ struct offload_callbacks { netdev_features_t features); struct sk_buff *(*gro_receive)(struct list_head *head, struct sk_buff *skb); - int (*gro_complete)(struct sk_buff *skb, int nhoff); + int (*gro_complete)(struct sk_buff *skb, int nhoff, int thoff); }; struct packet_offload { diff --git a/include/linux/udp.h b/include/linux/udp.h index 3748e82b627b7044508db66adbf77c54a8e3d612..a04d94a3b42f8f5fb3d85622d2f3e56a94c4ea86 100644 --- a/include/linux/udp.h +++ b/include/linux/udp.h @@ -82,7 +82,7 @@ struct udp_sock { struct sk_buff *skb); int (*gro_complete)(struct sock *sk, struct sk_buff *skb, - int nhoff); + int nhoff, int thoff); /* udp_recvmsg try to use this before splicing sk_receive_queue */ struct sk_buff_head reader_queue ____cacheline_aligned_in_smp; diff --git a/include/net/gro.h b/include/net/gro.h index b435f0ddbf64f7bf740b7e479a1b28bcdef122c6..2856b00b84dfbf122871ef9bedab2097a7c823eb 100644 --- a/include/net/gro.h +++ b/include/net/gro.h @@ -385,18 +385,18 @@ static inline void skb_gro_flush_final_remcsum(struct sk_buff *skb, INDIRECT_CALLABLE_DECLARE(struct sk_buff *ipv6_gro_receive(struct list_head *, struct sk_buff *)); -INDIRECT_CALLABLE_DECLARE(int ipv6_gro_complete(struct sk_buff *, int)); +INDIRECT_CALLABLE_DECLARE(int ipv6_gro_complete(struct sk_buff *, int, int)); INDIRECT_CALLABLE_DECLARE(struct sk_buff *inet_gro_receive(struct list_head *, struct sk_buff *)); -INDIRECT_CALLABLE_DECLARE(int inet_gro_complete(struct sk_buff *, int)); +INDIRECT_CALLABLE_DECLARE(int inet_gro_complete(struct sk_buff *, int, int)); INDIRECT_CALLABLE_DECLARE(struct sk_buff *udp4_gro_receive(struct list_head *, struct sk_buff *)); -INDIRECT_CALLABLE_DECLARE(int udp4_gro_complete(struct sk_buff *, int)); +INDIRECT_CALLABLE_DECLARE(int udp4_gro_complete(struct sk_buff *, int, int)); INDIRECT_CALLABLE_DECLARE(struct sk_buff *udp6_gro_receive(struct list_head *, struct sk_buff *)); -INDIRECT_CALLABLE_DECLARE(int udp6_gro_complete(struct sk_buff *, int)); +INDIRECT_CALLABLE_DECLARE(int udp6_gro_complete(struct sk_buff *, int, int)); #define indirect_call_gro_receive_inet(cb, f2, f1, head, skb) \ ({ \ @@ -407,7 +407,7 @@ INDIRECT_CALLABLE_DECLARE(int udp6_gro_complete(struct sk_buff *, int)); struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb, struct udphdr *uh, struct sock *sk); -int udp_gro_complete(struct sk_buff *skb, int nhoff, udp_lookup_t lookup); +int udp_gro_complete(struct sk_buff *skb, int nhoff, int thoff, udp_lookup_t lookup); static inline struct udphdr *udp_gro_udphdr(struct sk_buff *skb) { diff --git a/include/net/inet_common.h b/include/net/inet_common.h index f50a644d87a9871fbed2dfd49d4bde9d3df0fd92..605f917c830c9b1794dfc3bda1bdc9ac24a0def3 100644 --- a/include/net/inet_common.h +++ b/include/net/inet_common.h @@ -64,7 +64,7 @@ int inet_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len); struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb); -int inet_gro_complete(struct sk_buff *skb, int nhoff); +int inet_gro_complete(struct sk_buff *skb, int nhoff, int thoff); struct sk_buff *inet_gso_segment(struct sk_buff *skb, netdev_features_t features); diff --git a/include/net/tcp.h b/include/net/tcp.h index 6ae35199d3b3c159ba029ff74b109c56a7c7d2fc..ad8e6efc8bd867ada990820b5badce96ee49c3da 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -2196,9 +2196,9 @@ void tcp_v4_destroy_sock(struct sock *sk); struct sk_buff *tcp_gso_segment(struct sk_buff *skb, netdev_features_t features); struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb); -INDIRECT_CALLABLE_DECLARE(int tcp4_gro_complete(struct sk_buff *skb, int thoff)); +INDIRECT_CALLABLE_DECLARE(int tcp4_gro_complete(struct sk_buff *skb, int nhoff, int thoff)); INDIRECT_CALLABLE_DECLARE(struct sk_buff *tcp4_gro_receive(struct list_head *head, struct sk_buff *skb)); -INDIRECT_CALLABLE_DECLARE(int tcp6_gro_complete(struct sk_buff *skb, int thoff)); +INDIRECT_CALLABLE_DECLARE(int tcp6_gro_complete(struct sk_buff *skb, int nhoff, int thoff)); INDIRECT_CALLABLE_DECLARE(struct sk_buff *tcp6_gro_receive(struct list_head *head, struct sk_buff *skb)); #ifdef CONFIG_INET void tcp_gro_complete(struct sk_buff *skb); diff --git a/include/net/udp.h b/include/net/udp.h index 488a6d2babccf26edfbaecc525f25e03d86b7d62..601d1c3b677a9acd6b6615e64de3f034f940287d 100644 --- a/include/net/udp.h +++ b/include/net/udp.h @@ -166,8 +166,8 @@ static inline void udp_csum_pull_header(struct sk_buff *skb) UDP_SKB_CB(skb)->cscov -= sizeof(struct udphdr); } -typedef struct sock *(*udp_lookup_t)(const struct sk_buff *skb, __be16 sport, - __be16 dport); +typedef struct sock *(*udp_lookup_t)(const struct sk_buff *skb, int nhoff, + __be16 sport, __be16 dport); void udp_v6_early_demux(struct sk_buff *skb); INDIRECT_CALLABLE_DECLARE(int udpv6_rcv(struct sk_buff *)); @@ -301,7 +301,7 @@ struct sock *udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport, struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport, __be32 daddr, __be16 dport, int dif, int sdif, struct udp_table *tbl, struct sk_buff *skb); -struct sock *udp4_lib_lookup_skb(const struct sk_buff *skb, +struct sock *udp4_lib_lookup_skb(const struct sk_buff *skb, int nhoff, __be16 sport, __be16 dport); struct sock *udp6_lib_lookup(struct net *net, const struct in6_addr *saddr, __be16 sport, @@ -312,7 +312,7 @@ struct sock *__udp6_lib_lookup(struct net *net, const struct in6_addr *daddr, __be16 dport, int dif, int sdif, struct udp_table *tbl, struct sk_buff *skb); -struct sock *udp6_lib_lookup_skb(const struct sk_buff *skb, +struct sock *udp6_lib_lookup_skb(const struct sk_buff *skb, int nhoff, __be16 sport, __be16 dport); int udp_read_skb(struct sock *sk, skb_read_actor_t recv_actor); diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h index d716214fe03df0a56266c22c7e8b42ba650e728b..a641392e70b0aa3d01826430640f3db6557d604b 100644 --- a/include/net/udp_tunnel.h +++ b/include/net/udp_tunnel.h @@ -75,7 +75,7 @@ typedef struct sk_buff *(*udp_tunnel_gro_receive_t)(struct sock *sk, struct list_head *head, struct sk_buff *skb); typedef int (*udp_tunnel_gro_complete_t)(struct sock *sk, struct sk_buff *skb, - int nhoff); + int nhoff, int thoff); struct udp_tunnel_sock_cfg { void *sk_user_data; /* user data used by encap_rcv call back */ diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c index f001582345052f8c26e008058ae5f721f8bc224d..247704cf70aff1279c62507a940581e5049175b9 100644 --- a/net/8021q/vlan_core.c +++ b/net/8021q/vlan_core.c @@ -510,7 +510,7 @@ static struct sk_buff *vlan_gro_receive(struct list_head *head, return pp; } -static int vlan_gro_complete(struct sk_buff *skb, int nhoff) +static int vlan_gro_complete(struct sk_buff *skb, int p_off, int nhoff) { struct vlan_hdr *vhdr = (struct vlan_hdr *)(skb->data + nhoff); __be16 type = vhdr->h_vlan_encapsulated_proto; @@ -521,7 +521,7 @@ static int vlan_gro_complete(struct sk_buff *skb, int nhoff) if (ptype) err = INDIRECT_CALL_INET(ptype->callbacks.gro_complete, ipv6_gro_complete, inet_gro_complete, - skb, nhoff + sizeof(*vhdr)); + skb, p_off, nhoff + sizeof(*vhdr)); return err; } diff --git a/net/core/gro.c b/net/core/gro.c index 0759277dc14ee65d0a5376d48694cc1cccaee959..07768055ecf25d426e7cd01551f32978a70e8379 100644 --- a/net/core/gro.c +++ b/net/core/gro.c @@ -254,7 +254,7 @@ static void napi_gro_complete(struct napi_struct *napi, struct sk_buff *skb) err = INDIRECT_CALL_INET(ptype->callbacks.gro_complete, ipv6_gro_complete, inet_gro_complete, - skb, 0); + skb, 0, 0); break; } rcu_read_unlock(); diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c index 2edc8b796a4e7326aa44128a0618e15b9aa817de..7515e6bcbb7d1e62fa0af2fd477bdea0284bfe40 100644 --- a/net/ethernet/eth.c +++ b/net/ethernet/eth.c @@ -453,7 +453,7 @@ struct sk_buff *eth_gro_receive(struct list_head *head, struct sk_buff *skb) } EXPORT_SYMBOL(eth_gro_receive); -int eth_gro_complete(struct sk_buff *skb, int nhoff) +int eth_gro_complete(struct sk_buff *skb, int p_off, int nhoff) { struct ethhdr *eh = (struct ethhdr *)(skb->data + nhoff); __be16 type = eh->h_proto; @@ -467,7 +467,7 @@ int eth_gro_complete(struct sk_buff *skb, int nhoff) if (ptype != NULL) err = INDIRECT_CALL_INET(ptype->callbacks.gro_complete, ipv6_gro_complete, inet_gro_complete, - skb, nhoff + sizeof(*eh)); + skb, p_off, nhoff + sizeof(*eh)); return err; } diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index 5daebdcbca326aa1fc042e1e1ff1e82a18bd283d..bef9b222f3c90487c7d44015efb23c08168cf164 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -1640,7 +1640,7 @@ int inet_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len) } EXPORT_SYMBOL(inet_recv_error); -int inet_gro_complete(struct sk_buff *skb, int nhoff) +int inet_gro_complete(struct sk_buff *skb, int prior_off, int nhoff) { struct iphdr *iph = (struct iphdr *)(skb->data + nhoff); const struct net_offload *ops; @@ -1666,17 +1666,17 @@ int inet_gro_complete(struct sk_buff *skb, int nhoff) */ err = INDIRECT_CALL_2(ops->callbacks.gro_complete, tcp4_gro_complete, udp4_gro_complete, - skb, nhoff + sizeof(*iph)); + skb, nhoff, nhoff + sizeof(*iph)); out: return err; } -static int ipip_gro_complete(struct sk_buff *skb, int nhoff) +static int ipip_gro_complete(struct sk_buff *skb, int prior_off, int nhoff) { skb->encapsulation = 1; skb_shinfo(skb)->gso_type |= SKB_GSO_IPXIP4; - return inet_gro_complete(skb, nhoff); + return inet_gro_complete(skb, prior_off, nhoff); } int inet_ctl_sock_create(struct sock **sk, unsigned short family, diff --git a/net/ipv4/fou_core.c b/net/ipv4/fou_core.c index 0c41076e31edadd16f8e55ebc50f84db262a2f0d..ac4a6595d5cdf7e1e4b0eef417173d8d2d4ad76d 100644 --- a/net/ipv4/fou_core.c +++ b/net/ipv4/fou_core.c @@ -260,7 +260,7 @@ static struct sk_buff *fou_gro_receive(struct sock *sk, } static int fou_gro_complete(struct sock *sk, struct sk_buff *skb, - int nhoff) + int p_off, int nhoff) { const struct net_offload __rcu **offloads; u8 proto = fou_from_sock(sk)->protocol; @@ -272,7 +272,7 @@ static int fou_gro_complete(struct sock *sk, struct sk_buff *skb, if (WARN_ON(!ops || !ops->callbacks.gro_complete)) goto out; - err = ops->callbacks.gro_complete(skb, nhoff); + err = ops->callbacks.gro_complete(skb, p_off, nhoff); skb_set_inner_mac_header(skb, nhoff); @@ -445,7 +445,8 @@ static struct sk_buff *gue_gro_receive(struct sock *sk, return pp; } -static int gue_gro_complete(struct sock *sk, struct sk_buff *skb, int nhoff) +static int gue_gro_complete(struct sock *sk, struct sk_buff *skb, + int p_off, int nhoff) { struct guehdr *guehdr = (struct guehdr *)(skb->data + nhoff); const struct net_offload __rcu **offloads; @@ -480,7 +481,7 @@ static int gue_gro_complete(struct sock *sk, struct sk_buff *skb, int nhoff) if (WARN_ON(!ops || !ops->callbacks.gro_complete)) goto out; - err = ops->callbacks.gro_complete(skb, nhoff + guehlen); + err = ops->callbacks.gro_complete(skb, p_off, nhoff + guehlen); skb_set_inner_mac_header(skb, nhoff + guehlen); diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c index 311e70bfce407a2cadaa33fbef9a3976375711f4..803a8498f3030ec80a0ed41313e314021416a207 100644 --- a/net/ipv4/gre_offload.c +++ b/net/ipv4/gre_offload.c @@ -233,7 +233,7 @@ static struct sk_buff *gre_gro_receive(struct list_head *head, return pp; } -static int gre_gro_complete(struct sk_buff *skb, int nhoff) +static int gre_gro_complete(struct sk_buff *skb, int p_off, int nhoff) { struct gre_base_hdr *greh = (struct gre_base_hdr *)(skb->data + nhoff); struct packet_offload *ptype; @@ -253,7 +253,7 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff) ptype = gro_find_complete_by_type(type); if (ptype) - err = ptype->callbacks.gro_complete(skb, nhoff + grehlen); + err = ptype->callbacks.gro_complete(skb, p_off, nhoff + grehlen); skb_set_inner_mac_header(skb, nhoff + grehlen); diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c index 8311c38267b55ba97e59924c3c1c5b59f133fdcd..6f126f7d806d60e4d884c4f95b53b8e4bd9fbb8a 100644 --- a/net/ipv4/tcp_offload.c +++ b/net/ipv4/tcp_offload.c @@ -328,10 +328,10 @@ struct sk_buff *tcp4_gro_receive(struct list_head *head, struct sk_buff *skb) return tcp_gro_receive(head, skb); } -INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff) +INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int nhoff, int thoff) { - const struct iphdr *iph = ip_hdr(skb); - struct tcphdr *th = tcp_hdr(skb); + const struct iphdr *iph = (const struct iphdr *)(skb->data + nhoff); + struct tcphdr *th = (struct tcphdr *)(skb->data + thoff); th->check = ~tcp_v4_check(skb->len - thoff, iph->saddr, iph->daddr, 0); diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index a8acea17b4e5344d022ae8f8eb674d1a36f8035a..70a6d174855f76fb5dd289e3ef222a5158890e4f 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -532,9 +532,10 @@ static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb, } struct sock *udp4_lib_lookup_skb(const struct sk_buff *skb, + int nhoff, __be16 sport, __be16 dport) { - const struct iphdr *iph = ip_hdr(skb); + const struct iphdr *iph = (const struct iphdr *)(skb->data + nhoff); struct net *net = dev_net(skb->dev); int iif, sdif; diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index 6c95d28d0c4a7e56d587a986113b3711f8de964c..fa82a5a08688086415bae1d425d034f12311b0ab 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -669,18 +669,18 @@ static int udp_gro_complete_segment(struct sk_buff *skb) return 0; } -int udp_gro_complete(struct sk_buff *skb, int nhoff, +int udp_gro_complete(struct sk_buff *skb, int nhoff, int thoff, udp_lookup_t lookup) { - __be16 newlen = htons(skb->len - nhoff); - struct udphdr *uh = (struct udphdr *)(skb->data + nhoff); + struct udphdr *uh = (struct udphdr *)(skb->data + thoff); + __be16 newlen = htons(skb->len - thoff); struct sock *sk; int err; uh->len = newlen; sk = INDIRECT_CALL_INET(lookup, udp6_lib_lookup_skb, - udp4_lib_lookup_skb, skb, uh->source, uh->dest); + udp4_lib_lookup_skb, skb, nhoff, uh->source, uh->dest); if (sk && udp_sk(sk)->gro_complete) { skb_shinfo(skb)->gso_type = uh->check ? SKB_GSO_UDP_TUNNEL_CSUM : SKB_GSO_UDP_TUNNEL; @@ -694,8 +694,8 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff, * functions to make them set up the inner offsets. */ skb->encapsulation = 1; - err = udp_sk(sk)->gro_complete(sk, skb, - nhoff + sizeof(struct udphdr)); + err = udp_sk(sk)->gro_complete(sk, skb, nhoff, + thoff + sizeof(struct udphdr)); } else { err = udp_gro_complete_segment(skb); } @@ -707,14 +707,14 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff, } EXPORT_SYMBOL(udp_gro_complete); -INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff) +INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff, int thoff) { - const struct iphdr *iph = ip_hdr(skb); - struct udphdr *uh = (struct udphdr *)(skb->data + nhoff); + const struct iphdr *iph = (const struct iphdr *)(skb->data + nhoff); + struct udphdr *uh = (struct udphdr *)(skb->data + thoff); /* do fraglist only if there is no outer UDP encap (or we already processed it) */ if (NAPI_GRO_CB(skb)->is_flist && !NAPI_GRO_CB(skb)->encap_mark) { - uh->len = htons(skb->len - nhoff); + uh->len = htons(skb->len - thoff); skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4); skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count; @@ -731,10 +731,10 @@ INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff) } if (uh->check) - uh->check = ~udp_v4_check(skb->len - nhoff, iph->saddr, + uh->check = ~udp_v4_check(skb->len - thoff, iph->saddr, iph->daddr, 0); - return udp_gro_complete(skb, nhoff, udp4_lib_lookup_skb); + return udp_gro_complete(skb, nhoff, thoff, udp4_lib_lookup_skb); } static const struct net_offload udpv4_offload = { diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c index cca64c7809bee9a0360cbfab6a645d3f8d2ffea3..e3a05b84c76ab9d55afafa94a53130b3b0d6c7f7 100644 --- a/net/ipv6/ip6_offload.c +++ b/net/ipv6/ip6_offload.c @@ -346,12 +346,14 @@ static struct sk_buff *ip4ip6_gro_receive(struct list_head *head, return inet_gro_receive(head, skb); } -INDIRECT_CALLABLE_SCOPE int ipv6_gro_complete(struct sk_buff *skb, int nhoff) +INDIRECT_CALLABLE_SCOPE int ipv6_gro_complete(struct sk_buff *skb, + int p_off, int nhoff) { const struct net_offload *ops; struct ipv6hdr *iph; int err = -ENOSYS; u32 payload_len; + int nhlen; if (skb->encapsulation) { skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IPV6)); @@ -387,36 +389,36 @@ INDIRECT_CALLABLE_SCOPE int ipv6_gro_complete(struct sk_buff *skb, int nhoff) iph->payload_len = htons(payload_len); } - nhoff += sizeof(*iph) + ipv6_exthdrs_len(iph, &ops); - if (WARN_ON(!ops || !ops->callbacks.gro_complete)) + nhlen = sizeof(*iph) + ipv6_exthdrs_len(iph, &ops); + if (WARN_ON_ONCE(!ops || !ops->callbacks.gro_complete)) goto out; err = INDIRECT_CALL_L4(ops->callbacks.gro_complete, tcp6_gro_complete, - udp6_gro_complete, skb, nhoff); + udp6_gro_complete, skb, nhoff, nhoff + nhlen); out: return err; } -static int sit_gro_complete(struct sk_buff *skb, int nhoff) +static int sit_gro_complete(struct sk_buff *skb, int p_off, int nhoff) { skb->encapsulation = 1; skb_shinfo(skb)->gso_type |= SKB_GSO_IPXIP4; - return ipv6_gro_complete(skb, nhoff); + return ipv6_gro_complete(skb, p_off, nhoff); } -static int ip6ip6_gro_complete(struct sk_buff *skb, int nhoff) +static int ip6ip6_gro_complete(struct sk_buff *skb, int p_off, int nhoff) { skb->encapsulation = 1; skb_shinfo(skb)->gso_type |= SKB_GSO_IPXIP6; - return ipv6_gro_complete(skb, nhoff); + return ipv6_gro_complete(skb, p_off, nhoff); } -static int ip4ip6_gro_complete(struct sk_buff *skb, int nhoff) +static int ip4ip6_gro_complete(struct sk_buff *skb, int p_off, int nhoff) { skb->encapsulation = 1; skb_shinfo(skb)->gso_type |= SKB_GSO_IPXIP6; - return inet_gro_complete(skb, nhoff); + return inet_gro_complete(skb, p_off, nhoff); } static struct packet_offload ipv6_packet_offload __read_mostly = { diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c index bf0c957e4b5eaaabc0ac3a7e55c7de6608cec156..5043d2ff34eb8a7f9aeb7db6b4347a0f976b8502 100644 --- a/net/ipv6/tcpv6_offload.c +++ b/net/ipv6/tcpv6_offload.c @@ -27,10 +27,11 @@ struct sk_buff *tcp6_gro_receive(struct list_head *head, struct sk_buff *skb) return tcp_gro_receive(head, skb); } -INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int thoff) +INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, + int nhoff, int thoff) { - const struct ipv6hdr *iph = ipv6_hdr(skb); - struct tcphdr *th = tcp_hdr(skb); + const struct ipv6hdr *iph = (const struct ipv6hdr *)(skb->data + nhoff); + struct tcphdr *th = (struct tcphdr *)(skb->data + thoff); th->check = ~tcp_v6_check(skb->len - thoff, &iph->saddr, &iph->daddr, 0); diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c index 3f2249b4cd5f6a594dd9768e29f20f0d9a57faed..400243c89d8234ff98b3991c049e83237fa6686f 100644 --- a/net/ipv6/udp.c +++ b/net/ipv6/udp.c @@ -273,9 +273,10 @@ static struct sock *__udp6_lib_lookup_skb(struct sk_buff *skb, } struct sock *udp6_lib_lookup_skb(const struct sk_buff *skb, + int nhoff, __be16 sport, __be16 dport) { - const struct ipv6hdr *iph = ipv6_hdr(skb); + const struct ipv6hdr *iph = (const struct ipv6hdr *)(skb->data + nhoff); struct net *net = dev_net(skb->dev); int iif, sdif; diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c index 6b95ba241ebe2af7e5f2760d8a9c1d78f08579c5..7bf0fb609451586ab9fb1de955d1ae2ea2dbee64 100644 --- a/net/ipv6/udp_offload.c +++ b/net/ipv6/udp_offload.c @@ -162,14 +162,14 @@ struct sk_buff *udp6_gro_receive(struct list_head *head, struct sk_buff *skb) return NULL; } -INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff) +INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff, int thoff) { - const struct ipv6hdr *ipv6h = ipv6_hdr(skb); - struct udphdr *uh = (struct udphdr *)(skb->data + nhoff); + const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)(skb->data + nhoff); + struct udphdr *uh = (struct udphdr *)(skb->data + thoff); /* do fraglist only if there is no outer UDP encap (or we already processed it) */ if (NAPI_GRO_CB(skb)->is_flist && !NAPI_GRO_CB(skb)->encap_mark) { - uh->len = htons(skb->len - nhoff); + uh->len = htons(skb->len - thoff); skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4); skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count; @@ -186,10 +186,10 @@ INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff) } if (uh->check) - uh->check = ~udp_v6_check(skb->len - nhoff, &ipv6h->saddr, + uh->check = ~udp_v6_check(skb->len - thoff, &ipv6h->saddr, &ipv6h->daddr, 0); - return udp_gro_complete(skb, nhoff, udp6_lib_lookup_skb); + return udp_gro_complete(skb, nhoff, thoff, udp6_lib_lookup_skb); } static const struct net_offload udpv6_offload = { ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 1/3] net: gro: set {inner_,}network_header in receive phase 2024-02-29 20:15 ` Eric Dumazet @ 2024-03-01 15:02 ` Richard Gobert 0 siblings, 0 replies; 8+ messages in thread From: Richard Gobert @ 2024-03-01 15:02 UTC (permalink / raw) To: Eric Dumazet Cc: davem, kuba, pabeni, dsahern, shuah, liujian56, horms, aleksander.lobakin, linyunsheng, therbert, netdev, linux-kernel, linux-kselftest Eric Dumazet wrote: > On Thu, Feb 29, 2024 at 2:22 PM Richard Gobert <richardbgobert@gmail.com> wrote: >> >> >> >> Eric Dumazet wrote: >>> >>> My intuition is that this patch has a high cost for normal GRO processing. >>> SW-GRO is already a bottleneck on ARM cores in smart NICS. >>> >>> I would suggest instead using parameters to give both the nhoff and thoff values >>> this would avoid many conditionals in the fast path. >>> >>> -> >>> >>> INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int >>> nhoff, int thoff) >>> { >>> const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)(skb->data + nhoff); >>> struct udphdr *uh = (struct udphdr *)(skb->data + thoff); >>> ... >>> } >>> >>> INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int >>> nhoff, int thoff) >>> { >>> const struct ipv6hdr *iph = (const struct ipv6hdr *)(skb->data + nhoff); >>> struct tcphdr *th = (struct tcphdr *)(skb->data + thoff); >>> >>> Why storing in skb fields things that really could be propagated more >>> efficiently as function parameters ? >> >> Hi Eric, >> Thanks for the review! >> >> I agree, the conditionals could be a problem and are actually not needed. >> The third commit in this patch series introduces an optimisation for >> ipv6/ipv4 using the correct {inner_}network_header. We can remove the >> conditionals; I thought about multiple ways to do so. First, remove the >> conditional in skb_gro_network_offset: >> >> static inline int skb_gro_network_offset(const struct sk_buff *skb) >> { >> const u32 mask = NAPI_GRO_CB(skb)->encap_mark - 1; >> return (skb_network_offset(skb) & mask) | (skb_inner_network_offset(skb) & ~mask); >> } > > I was trying to say that we do not need all these helpers, storing > state in NAPI_GRO_CB(skb), > dirtying cache lines... > > Ideally, the skb network/transport/... headers could be set at the > last stage, in gro_complete(big_gro_skb), > instead of doing this for each segment. > > All the gro_receive() could be much faster by using additional > parameters (nhoff, thoff) > > skb_gro_offset() could be replaced by the current offset (nhoff or > other name), passed as a parameter. > > Here is a WIP for gro_complete() step, this looks large but this is > only adding a 2nd 'offset' parameter > > Prior offset (typically network offset), called p_off > Old argument nhoff, (renamed thoff if that makes sense), pointing to > the current offset. > You're right, it seemed to me like a broad change but it is mainly cosmetic. I'll finish your version and submit it to fix the bug. I still believe that setting inner_network_header is a valuable change. For example, although skb_gro_network_offset is used - setting it in encapsulation protocol functions (such as ipip_gro_receive) allow us to remove conditionals from {ipv6,inet}_gro_receive gro_list loop and remove flush_id from napi_gro_cb as written in the 3rd commit. What are your thoughts about it as a separate patch? ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next 2/3] selftests/net: add local address bind in vxlan selftest 2024-02-22 21:00 [PATCH net-next 0/3] net: gro: encapsulation bug fix and flush checks improvements Richard Gobert 2024-02-22 21:05 ` [PATCH net-next 1/3] net: gro: set {inner_,}network_header in receive phase Richard Gobert @ 2024-02-22 21:07 ` Richard Gobert 2024-02-22 21:12 ` [PATCH net-next 3/3] net: gro: move L3 flush checks to tcp_gro_receive Richard Gobert 2 siblings, 0 replies; 8+ messages in thread From: Richard Gobert @ 2024-02-22 21:07 UTC (permalink / raw) To: davem, edumazet, kuba, pabeni, dsahern, shuah, liujian56, horms, aleksander.lobakin, linyunsheng, therbert, netdev, linux-kernel, linux-kselftest Add local address bind support to existing udpgro_fwd.sh vxlan selftest, to ensure UDP socket lookup in GRO is working. Signed-off-by: Richard Gobert <richardbgobert@gmail.com> --- tools/testing/selftests/net/udpgro_fwd.sh | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/net/udpgro_fwd.sh b/tools/testing/selftests/net/udpgro_fwd.sh index c079565add39..8cc9b6ff3910 100755 --- a/tools/testing/selftests/net/udpgro_fwd.sh +++ b/tools/testing/selftests/net/udpgro_fwd.sh @@ -56,11 +56,13 @@ create_vxlan_endpoint() { local -r bm_rem_addr=$3 local -r vxlan_dev=$4 local -r vxlan_id=$5 + local -r bm_local_addr=$6 local -r vxlan_port=4789 ip -n $netns link set dev $bm_dev up ip -n $netns link add dev $vxlan_dev type vxlan id $vxlan_id \ - dstport $vxlan_port remote $bm_rem_addr + dstport $vxlan_port local $bm_local_addr \ + remote $bm_rem_addr ip -n $netns link set dev $vxlan_dev up } @@ -71,11 +73,13 @@ create_vxlan_pair() { for ns in $SRC $DST; do # note that 3 - $SRC == $DST and 3 - $DST == $SRC - create_vxlan_endpoint $BASE$ns veth$ns $BM_NET_V4$((3 - $ns)) vxlan$ns 4 + create_vxlan_endpoint $BASE$ns veth$ns $BM_NET_V4$((3 - $ns)) \ + vxlan$ns 4 $BM_NET_V4$ns ip -n $BASE$ns addr add dev vxlan$ns $OL_NET_V4$ns/24 done for ns in $SRC $DST; do - create_vxlan_endpoint $BASE$ns veth$ns $BM_NET_V6$((3 - $ns)) vxlan6$ns 6 + create_vxlan_endpoint $BASE$ns veth$ns $BM_NET_V6$((3 - $ns)) \ + vxlan6$ns 6 $BM_NET_V6$ns ip -n $BASE$ns addr add dev vxlan6$ns $OL_NET_V6$ns/24 nodad done } -- 2.36.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net-next 3/3] net: gro: move L3 flush checks to tcp_gro_receive 2024-02-22 21:00 [PATCH net-next 0/3] net: gro: encapsulation bug fix and flush checks improvements Richard Gobert 2024-02-22 21:05 ` [PATCH net-next 1/3] net: gro: set {inner_,}network_header in receive phase Richard Gobert 2024-02-22 21:07 ` [PATCH net-next 2/3] selftests/net: add local address bind in vxlan selftest Richard Gobert @ 2024-02-22 21:12 ` Richard Gobert 2 siblings, 0 replies; 8+ messages in thread From: Richard Gobert @ 2024-02-22 21:12 UTC (permalink / raw) To: davem, edumazet, kuba, pabeni, dsahern, shuah, liujian56, horms, aleksander.lobakin, linyunsheng, therbert, netdev, linux-kernel, linux-kselftest {inet,ipv6}_gro_receive functions perform flush checks (ttl, flags, iph->id, ...) against all packets in a loop. These flush checks are relevant only to tcp flows, and as such they're used to determine whether the packets can be merged later in tcp_gro_receive. These checks are not relevant to UDP packets. Furthermore, they need to be done only once in tcp_gro_receive and only against the found p skb, since they only affect flush and not same_flow. Leveraging the initial commit in the series, correct network header offsets are saved for both outer and inner network headers - allowing these checks to be done only once, in tcp_gro_receive. As a result, NAPI_GRO_CB(p)->flush is not used at all. In addition - flush_id checks are more declarative and contained in inet_gro_flush_id, thus removing the need for flush_id in napi_gro_cb. This results in less parsing code for UDP flows and non-loop flush tests for TCP flows. For example, running 64 IP/UDP netperf connections: ./super_netperf.sh 64 -H 1.1.1.2 -t UDP_STREAM -l 120 Running perf top for 90s we can see that relatively less time is spent on inet_gro_receive: net-next: Samples: 330K of event 'cycles:P', 4000 Hz, Event count (approx.): 45945660156 lost: 0/0 drop: 0/0 1.43% [kernel] [k] inet_gro_receive patch applied: Samples: 295K of event 'cycles:P', 4000 Hz, Event count (approx.): 44726408099 lost: 0/0 drop: 0/0 1.22% [kernel] [k] inet_gro_receive Signed-off-by: Richard Gobert <richardbgobert@gmail.com> --- include/net/gro.h | 9 ++---- net/core/gro.c | 3 -- net/ipv4/af_inet.c | 36 --------------------- net/ipv4/tcp_offload.c | 71 ++++++++++++++++++++++++++++++++++-------- net/ipv6/ip6_offload.c | 11 ------- 5 files changed, 61 insertions(+), 69 deletions(-) diff --git a/include/net/gro.h b/include/net/gro.h index 89502a7e35ed..e6db591cdc32 100644 --- a/include/net/gro.h +++ b/include/net/gro.h @@ -35,15 +35,15 @@ struct napi_gro_cb { /* This is non-zero if the packet cannot be merged with the new skb. */ u16 flush; - /* Save the IP ID here and check when we get to the transport layer */ - u16 flush_id; - /* Number of segments aggregated. */ u16 count; /* Used in ipv6_gro_receive() and foo-over-udp and esp-in-udp */ u16 proto; + /* used to support CHECKSUM_COMPLETE for tunneling protocols */ + __wsum csum; + /* Used in napi_gro_cb::free */ #define NAPI_GRO_FREE 1 #define NAPI_GRO_FREE_STOLEN_HEAD 2 @@ -83,9 +83,6 @@ struct napi_gro_cb { /* GRO is done by frag_list pointer chaining. */ u8 is_flist:1; ); - - /* used to support CHECKSUM_COMPLETE for tunneling protocols */ - __wsum csum; }; #define NAPI_GRO_CB(skb) ((struct napi_gro_cb *)(skb)->cb) diff --git a/net/core/gro.c b/net/core/gro.c index 0759277dc14e..b7dced5bcef7 100644 --- a/net/core/gro.c +++ b/net/core/gro.c @@ -332,8 +332,6 @@ static void gro_list_prepare(const struct list_head *head, list_for_each_entry(p, head, list) { unsigned long diffs; - NAPI_GRO_CB(p)->flush = 0; - if (hash != skb_get_hash_raw(p)) { NAPI_GRO_CB(p)->same_flow = 0; continue; @@ -466,7 +464,6 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff sizeof(u32))); /* Avoid slow unaligned acc */ *(u32 *)&NAPI_GRO_CB(skb)->zeroed = 0; NAPI_GRO_CB(skb)->flush = skb_has_frag_list(skb); - NAPI_GRO_CB(skb)->is_atomic = 1; NAPI_GRO_CB(skb)->count = 1; if (unlikely(skb_is_gso(skb))) { NAPI_GRO_CB(skb)->count = skb_shinfo(skb)->gso_segs; diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index c0f3c162bf73..fd7378ecc4e3 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -1509,7 +1509,6 @@ struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb) list_for_each_entry(p, head, list) { struct iphdr *iph2; - u16 flush_id; if (!NAPI_GRO_CB(p)->same_flow) continue; @@ -1526,43 +1525,8 @@ struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb) NAPI_GRO_CB(p)->same_flow = 0; continue; } - - /* All fields must match except length and checksum. */ - NAPI_GRO_CB(p)->flush |= - (iph->ttl ^ iph2->ttl) | - (iph->tos ^ iph2->tos) | - ((iph->frag_off ^ iph2->frag_off) & htons(IP_DF)); - - NAPI_GRO_CB(p)->flush |= flush; - - /* We need to store of the IP ID check to be included later - * when we can verify that this packet does in fact belong - * to a given flow. - */ - flush_id = (u16)(id - ntohs(iph2->id)); - - /* This bit of code makes it much easier for us to identify - * the cases where we are doing atomic vs non-atomic IP ID - * checks. Specifically an atomic check can return IP ID - * values 0 - 0xFFFF, while a non-atomic check can only - * return 0 or 0xFFFF. - */ - if (!NAPI_GRO_CB(p)->is_atomic || - !(iph->frag_off & htons(IP_DF))) { - flush_id ^= NAPI_GRO_CB(p)->count; - flush_id = flush_id ? 0xFFFF : 0; - } - - /* If the previous IP ID value was based on an atomic - * datagram we can overwrite the value and ignore it. - */ - if (NAPI_GRO_CB(skb)->is_atomic) - NAPI_GRO_CB(p)->flush_id = flush_id; - else - NAPI_GRO_CB(p)->flush_id |= flush_id; } - NAPI_GRO_CB(skb)->is_atomic = !!(iph->frag_off & htons(IP_DF)); NAPI_GRO_CB(skb)->flush |= flush; if (NAPI_GRO_CB(skb)->encap_mark) skb_set_inner_network_header(skb, off); diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c index 8bbcd3f502ac..962c1b900e11 100644 --- a/net/ipv4/tcp_offload.c +++ b/net/ipv4/tcp_offload.c @@ -178,6 +178,60 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb, return segs; } +static inline bool inet_gro_flush(struct iphdr *iph, struct iphdr *iph2) +{ + /* All fields must match except length and checksum. */ + return (iph->ttl ^ iph2->ttl) | + (iph->tos ^ iph2->tos) | + ((iph->frag_off ^ iph2->frag_off) & htons(IP_DF)); +} + +static inline bool ipv6_gro_flush(struct ipv6hdr *iph, struct ipv6hdr *iph2) +{ + /* <Version:4><Traffic_Class:8><Flow_Label:20> */ + __be32 first_word = *(__be32 *)iph ^ *(__be32 *)iph2; + + /* Flush if Traffic Class fields are different. */ + return (first_word & htonl(0x0FF00000)) | + (__force __be32)(iph->hop_limit ^ iph2->hop_limit); +} + +static inline bool inet_gro_flush_id(struct sk_buff *p, struct iphdr *iph, + struct iphdr *iph2) +{ + u16 flush_id = ntohs(iph->id) - ntohs(iph2->id); + u16 is_atomic = !!(iph->frag_off & htons(IP_DF)); + u16 count_masked; + + /* 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 (NAPI_GRO_CB(p)->count == 1) + NAPI_GRO_CB(p)->is_atomic = flush_id == 0; + + /* Ignore outer IP ID value if based on atomic datagram. */ + if (NAPI_GRO_CB(p)->encap_mark && is_atomic && iph2 == ip_hdr(p)) + return false; + + is_atomic &= NAPI_GRO_CB(p)->is_atomic; + count_masked = NAPI_GRO_CB(p)->count & ~(-is_atomic | is_atomic); + + return flush_id ^ count_masked; +} + +static bool tcp_gro_network_flush(struct sk_buff *p, struct sk_buff *skb, + int offset) +{ + void *nh = (NAPI_GRO_CB(skb)->frag0 ?: skb->data) + offset; + void *nh2 = p->data + offset; + + if (((struct iphdr *)nh)->version == 6) + return ipv6_gro_flush(nh, nh2); + + return inet_gro_flush(nh, nh2) | inet_gro_flush_id(p, nh, nh2); +} + struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb) { struct sk_buff *pp = NULL; @@ -232,8 +286,10 @@ struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb) goto out_check_final; found: - /* Include the IP ID check below from the inner most IP hdr */ - flush = NAPI_GRO_CB(p)->flush; + flush = tcp_gro_network_flush(p, skb, skb_network_offset(skb)); + if (NAPI_GRO_CB(skb)->encap_mark) + flush |= tcp_gro_network_flush(p, skb, skb_inner_network_offset(skb)); + flush |= (__force int)(flags & TCP_FLAG_CWR); flush |= (__force int)((flags ^ tcp_flag_word(th2)) & ~(TCP_FLAG_CWR | TCP_FLAG_FIN | TCP_FLAG_PSH)); @@ -242,17 +298,6 @@ struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb) flush |= *(u32 *)((u8 *)th + i) ^ *(u32 *)((u8 *)th2 + i); - /* When we receive our second frame we can made a decision on if we - * continue this flow as an atomic flow with a fixed ID or if we use - * an incrementing ID. - */ - if (NAPI_GRO_CB(p)->flush_id != 1 || - NAPI_GRO_CB(p)->count != 1 || - !NAPI_GRO_CB(p)->is_atomic) - flush |= NAPI_GRO_CB(p)->flush_id; - else - NAPI_GRO_CB(p)->is_atomic = false; - mss = skb_shinfo(p)->gso_size; /* If skb is a GRO packet, make sure its gso_size matches prior packet mss. diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c index db7e3db587b9..8bf93a0e9669 100644 --- a/net/ipv6/ip6_offload.c +++ b/net/ipv6/ip6_offload.c @@ -291,19 +291,8 @@ INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head, nlen - sizeof(struct ipv6hdr))) goto not_same_flow; } - /* flush if Traffic Class fields are different */ - NAPI_GRO_CB(p)->flush |= !!((first_word & htonl(0x0FF00000)) | - (__force __be32)(iph->hop_limit ^ iph2->hop_limit)); - NAPI_GRO_CB(p)->flush |= flush; - - /* If the previous IP ID value was based on an atomic - * datagram we can overwrite the value and ignore it. - */ - if (NAPI_GRO_CB(skb)->is_atomic) - NAPI_GRO_CB(p)->flush_id = 0; } - NAPI_GRO_CB(skb)->is_atomic = true; NAPI_GRO_CB(skb)->flush |= flush; skb_gro_postpull_rcsum(skb, iph, nlen); -- 2.36.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-03-01 15:02 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-22 21:00 [PATCH net-next 0/3] net: gro: encapsulation bug fix and flush checks improvements Richard Gobert
2024-02-22 21:05 ` [PATCH net-next 1/3] net: gro: set {inner_,}network_header in receive phase Richard Gobert
2024-02-22 21:25 ` Eric Dumazet
2024-02-29 13:22 ` Richard Gobert
2024-02-29 20:15 ` Eric Dumazet
2024-03-01 15:02 ` Richard Gobert
2024-02-22 21:07 ` [PATCH net-next 2/3] selftests/net: add local address bind in vxlan selftest Richard Gobert
2024-02-22 21:12 ` [PATCH net-next 3/3] net: gro: move L3 flush checks to tcp_gro_receive Richard Gobert
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).