Re: [PATCH net-next RFC 0/8] udp and configurable gro
On Fri, Oct 05, 2018 at 10:41:47AM -0400, Willem de Bruijn wrote: > On Fri, Oct 5, 2018 at 9:53 AM Paolo Abeni wrote: > > > > Hi all, > > > > On Fri, 2018-09-14 at 13:59 -0400, Willem de Bruijn wrote: > > > This is a *very rough* draft. Mainly for discussion while we also > > > look at another partially overlapping approach [1]. > > > > I'm wondering how we go on from this ? I'm fine with either approaches. > > Let me send the udp gro static_key patch. Then we don't need > the enable udp on demand logic (patch 2/4). > > Your implementation of GRO is more fleshed out (patch 3/4) than > my quick hack. My only request would be to use a separate > UDP_GRO socket option instead of adding this to the existing > UDP_SEGMENT. > > Sounds good? > > > Also, I'm interested in [try to] enable GRO/GSO batching in the > > forwarding path, as you outlined initially in the GSO series > > submission. That should cover Steffen use-case, too, right? > > Great. Indeed. Though there is some unresolved discussion on > one large gso skb vs frag list. There has been various concerns > around the use of frag lists for GSO in the past, and it does not > match h/w offload. So I think the answer would be the first unless > the second proves considerably faster (in which case it could also > be added later as optimization). I think it depends a bit on the usecase and hardware etc. if the first or the second approach is faster. So it would be good if we can choose which one to use depending on that. For local socket receiving, building big GSO packets is likely faster than the chaining method. But on forwarding the chaining method might be faster because we don't have the overhead of creating GSO packets and of segmenting them back to their native form (at least as long as we don't have NICs that support hardware UDP GSO). Same applies to packets that undergo IPsec transformation. Another thing where the chaining method could be intersting is when we receive already big LRO or HW GRO packets from the NIC. Packets of the same flow could still travel together through the stack with the chaining method. I've never tried this, though. For now it is just an idea. I have the code for the chaining mehthod here, I'd just need some method to hook it in. Maybe it could be done with some sort of an inet_update_offload() as Paolo already propsed in his pachset, or we could make it configurable per device...
Re: [PATCH net-next RFC 0/8] udp and configurable gro
On Fri, Oct 5, 2018 at 12:05 PM Paolo Abeni wrote: > > On Fri, 2018-10-05 at 11:45 -0400, Willem de Bruijn wrote: > > On Fri, Oct 5, 2018 at 11:30 AM Paolo Abeni wrote: > > > Would love that. We need to care of key decr, too (and possibly don't > > > be fooled by encap_rcv() users). > > > > I just sent http://patchwork.ozlabs.org/patch/979525/ > > > > Right now all users are those that call setup_udp_tunnel_sock > > to register encap_rcv. > > plus setsockopt(UDP_ENCAP) > > > If accepted, I'll add a separate patch to decrement the key. That's > > probably in udp_tunnel_sock_release, but I need to take a closer > > look. > > l2tp calls setup_udp_tunnel_sock() but don't use > udp_tunnel_sock_release(). Possibly it would be safer checking for: > > up->encap_type || up(sk)->gro_receive > > in udp_destroy_sock() Ah indeed. And gtp might be another example. Thanks!
Re: [PATCH net-next RFC 0/8] udp and configurable gro
On Fri, 2018-10-05 at 11:45 -0400, Willem de Bruijn wrote: > On Fri, Oct 5, 2018 at 11:30 AM Paolo Abeni wrote: > > Would love that. We need to care of key decr, too (and possibly don't > > be fooled by encap_rcv() users). > > I just sent http://patchwork.ozlabs.org/patch/979525/ > > Right now all users are those that call setup_udp_tunnel_sock > to register encap_rcv. plus setsockopt(UDP_ENCAP) > If accepted, I'll add a separate patch to decrement the key. That's > probably in udp_tunnel_sock_release, but I need to take a closer > look. l2tp calls setup_udp_tunnel_sock() but don't use udp_tunnel_sock_release(). Possibly it would be safer checking for: up->encap_type || up(sk)->gro_receive in udp_destroy_sock() Cheers, Paolo
Re: [PATCH net-next RFC 0/8] udp and configurable gro
On Fri, Oct 5, 2018 at 11:30 AM Paolo Abeni wrote: > > Hi, > > Thank you for the prompt reply! > Not at all. Thanks for moving this forward :) > On Fri, 2018-10-05 at 10:41 -0400, Willem de Bruijn wrote: > > On Fri, Oct 5, 2018 at 9:53 AM Paolo Abeni wrote: > > > > > > Hi all, > > > > > > On Fri, 2018-09-14 at 13:59 -0400, Willem de Bruijn wrote: > > > > This is a *very rough* draft. Mainly for discussion while we also > > > > look at another partially overlapping approach [1]. > > > > > > I'm wondering how we go on from this ? I'm fine with either approaches. > > > > Let me send the udp gro static_key patch. > > Would love that. We need to care of key decr, too (and possibly don't > be fooled by encap_rcv() users). I just sent http://patchwork.ozlabs.org/patch/979525/ Right now all users are those that call setup_udp_tunnel_sock to register encap_rcv. If accepted, I'll add a separate patch to decrement the key. That's probably in udp_tunnel_sock_release, but I need to take a closer look. > > Then we don't need the enable udp on demand logic (patch 2/4). > > ok. > > > Your implementation of GRO is more fleshed out (patch 3/4) than > > my quick hack. My only request would be to use a separate > > UDP_GRO socket option instead of adding this to the existing > > UDP_SEGMENT. > > > > Sounds good? > > Indeed! > I need also to add a cmsg to expose to the user the skb gro_size, and > some test cases. Locally I'm [ab-]using the GRO functionality > introduced recently on veth to test the code in a namespace pair > (attaching a dummy XDP program to the RX-side veth). I'm not sure if > that could fit a selftest. Very nice. Yes, veth only implements napi in xdp mode. > > > > Also, I'm interested in [try to] enable GRO/GSO batching in the > > > forwarding path, as you outlined initially in the GSO series > > > submission. That should cover Steffen use-case, too, right? > > > > Great. Indeed. Though there is some unresolved discussion on > > one large gso skb vs frag list. There has been various concerns > > around the use of frag lists for GSO in the past, and it does not > > match h/w offload. So I think the answer would be the first unless > > the second proves considerably faster (in which case it could also > > be added later as optimization). > > Agreed. > > Let's try the first step first ;) > > Final but relevant note: I'll try my best to avoid delaying this, but > lately I tend to be pre-empted by other tasks, it's difficult for me to > assure a deadline here. > > Cheers, > > Paolo >
Re: [PATCH net-next RFC 0/8] udp and configurable gro
Hi, Thank you for the prompt reply! On Fri, 2018-10-05 at 10:41 -0400, Willem de Bruijn wrote: > On Fri, Oct 5, 2018 at 9:53 AM Paolo Abeni wrote: > > > > Hi all, > > > > On Fri, 2018-09-14 at 13:59 -0400, Willem de Bruijn wrote: > > > This is a *very rough* draft. Mainly for discussion while we also > > > look at another partially overlapping approach [1]. > > > > I'm wondering how we go on from this ? I'm fine with either approaches. > > Let me send the udp gro static_key patch. Would love that. We need to care of key decr, too (and possibly don't be fooled by encap_rcv() users). > Then we don't need the enable udp on demand logic (patch 2/4). ok. > Your implementation of GRO is more fleshed out (patch 3/4) than > my quick hack. My only request would be to use a separate > UDP_GRO socket option instead of adding this to the existing > UDP_SEGMENT. > > Sounds good? Indeed! I need also to add a cmsg to expose to the user the skb gro_size, and some test cases. Locally I'm [ab-]using the GRO functionality introduced recently on veth to test the code in a namespace pair (attaching a dummy XDP program to the RX-side veth). I'm not sure if that could fit a selftest. > > Also, I'm interested in [try to] enable GRO/GSO batching in the > > forwarding path, as you outlined initially in the GSO series > > submission. That should cover Steffen use-case, too, right? > > Great. Indeed. Though there is some unresolved discussion on > one large gso skb vs frag list. There has been various concerns > around the use of frag lists for GSO in the past, and it does not > match h/w offload. So I think the answer would be the first unless > the second proves considerably faster (in which case it could also > be added later as optimization). Agreed. Let's try the first step first ;) Final but relevant note: I'll try my best to avoid delaying this, but lately I tend to be pre-empted by other tasks, it's difficult for me to assure a deadline here. Cheers, Paolo
Re: [PATCH net-next RFC 0/8] udp and configurable gro
On Fri, Oct 5, 2018 at 9:53 AM Paolo Abeni wrote: > > Hi all, > > On Fri, 2018-09-14 at 13:59 -0400, Willem de Bruijn wrote: > > This is a *very rough* draft. Mainly for discussion while we also > > look at another partially overlapping approach [1]. > > I'm wondering how we go on from this ? I'm fine with either approaches. Let me send the udp gro static_key patch. Then we don't need the enable udp on demand logic (patch 2/4). Your implementation of GRO is more fleshed out (patch 3/4) than my quick hack. My only request would be to use a separate UDP_GRO socket option instead of adding this to the existing UDP_SEGMENT. Sounds good? > Also, I'm interested in [try to] enable GRO/GSO batching in the > forwarding path, as you outlined initially in the GSO series > submission. That should cover Steffen use-case, too, right? Great. Indeed. Though there is some unresolved discussion on one large gso skb vs frag list. There has been various concerns around the use of frag lists for GSO in the past, and it does not match h/w offload. So I think the answer would be the first unless the second proves considerably faster (in which case it could also be added later as optimization).
Re: [PATCH net-next RFC 0/8] udp and configurable gro
Hi all, On Fri, 2018-09-14 at 13:59 -0400, Willem de Bruijn wrote: > This is a *very rough* draft. Mainly for discussion while we also > look at another partially overlapping approach [1]. I'm wondering how we go on from this ? I'm fine with either approaches. Also, I'm interested in [try to] enable GRO/GSO batching in the forwarding path, as you outlined initially in the GSO series submission. That should cover Steffen use-case, too, right? Cheers, Paolo
[PATCH net-next RFC 0/8] udp and configurable gro
From: Willem de Bruijn This is a *very rough* draft. Mainly for discussion while we also look at another partially overlapping approach [1]. Reduce UDP receive cost for bulk traffic by enabling datagram coalescing with GRO. Before adding more GRO callbacks, make GRO configurable by the administrator to optionally reduce the attack surface of this early receive path. See also [2]. Introduce sysctls net.(core|ipv4|ipv6).gro that expose the table of protocols for which GRO is support. Allow the administrator to disable individual entries in the table. To have a single infrastructure, convert dev_offloads to the table-based approach to existing inet(6)_offloads. Additional small benefit is that ipv6 will no longer take two list lookups to find. Patch 1 converts dev_offloads to the infra of inet(6)_offloads Patch 2 deduplicates gro_complete logic now that all share infra Patch 3 does the same for gro_receive, in anticipation of adding a branch to check whether gro_receive is enabled Patch 4 harmonizes ipv6 header opts, so that those, too can be optionally disabled. Patch 5 makes inet(6)_offloads non-const to allow disabling a flag Patch 6 introduces the administrative sysctl Patch 7 avoids udp gro cost if no udp gro callback is register Patch 8 introduces udp gro [1] http://patchwork.ozlabs.org/project/netdev/list/?series=65741 [2] http://vger.kernel.org/netconf2017_files/rx_hardening_and_udp_gso.pdf Willem de Bruijn (8): gro: convert device offloads to net_offload gro: deduplicate gro_complete gro: add net_gro_receive ipv6: remove offload exception for hopopts net: deconstify net_offload net: make gro configurable udp: gro behind static key udp: add gro drivers/net/geneve.c | 11 +--- drivers/net/vxlan.c| 8 +++ include/linux/netdevice.h | 64 +++-- include/net/protocol.h | 19 ++- include/net/udp.h | 2 + include/uapi/linux/udp.h | 1 + net/8021q/vlan.c | 12 +--- net/core/dev.c | 112 - net/core/sysctl_net_core.c | 60 net/ethernet/eth.c | 13 + net/ipv4/af_inet.c | 21 ++- net/ipv4/esp4_offload.c| 2 +- net/ipv4/fou.c | 41 -- net/ipv4/gre_offload.c | 26 - net/ipv4/protocol.c| 10 ++-- net/ipv4/sysctl_net_ipv4.c | 7 +++ net/ipv4/tcp_offload.c | 2 +- net/ipv4/udp.c | 73 +++- net/ipv4/udp_offload.c | 19 +++ net/ipv6/esp6_offload.c| 2 +- net/ipv6/exthdrs_offload.c | 17 +- net/ipv6/ip6_offload.c | 69 +-- net/ipv6/protocol.c| 10 ++-- net/ipv6/sysctl_net_ipv6.c | 8 +++ net/ipv6/tcpv6_offload.c | 2 +- net/ipv6/udp.c | 2 +- net/ipv6/udp_offload.c | 4 +- net/sctp/offload.c | 2 +- 28 files changed, 344 insertions(+), 275 deletions(-) -- 2.19.0.397.gdd90340f6a-goog
