net: Add sysctl to toggle early demux for tcp and udp

Certain system process significant unconnected UDP workload. It would be preferrable to disable UDP early demux for those systems and enable it for TCP only.

v1->v2: Change function pointer instead of adding conditional as suggested by Stephen.

Signed-off-by Subash Abhinov Kasiviswanathan <[email protected]>

Suggested-by Eric Dumazet <[email protected]>

Cc Stephen Hemminger <[email protected]>

---
 include/net/netns/ipv4.h   |  2 ++
 include/net/tcp.h          |  2 ++
 include/net/udp.h          |  2 ++
 net/ipv4/af_inet.c         | 22 ++++++++++++++++++++--
 net/ipv4/sysctl_net_ipv4.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 net/ipv6/tcp_ipv6.c        | 10 +++++++++-
 6 files changed, 82 insertions(+), 3 deletions(-)

include/net/netns/ipv4.h +2 -0

index 0378e88..1e74da23 100644
@@ -86,6 +86,8 @@ struct netns_ipv4 {
        /* Shall we try to damage output packets if routing dev changes? */
        int sysctl_ip_dynaddr;
        int sysctl_ip_early_demux;
+       int sysctl_tcp_early_demux;
+       int sysctl_udp_early_demux;
 
        int sysctl_fwmark_reflect;
        int sysctl_tcp_fwmark_accept;

include/net/tcp.h +2 -0

index 6061963..3b6446d 100644
@@ -1953,4 +1953,6 @@ static inline void tcp_listendrop(const struct sock *sk)
        __NET_INC_STATS(sock_net(sk), LINUX_MIB_LISTENDROPS);
 }
 
+void tcp_v4_early_demux_configure(int enable);
+void tcp_v6_early_demux_configure(int enable);
 #endif /* _TCP_H */

include/net/udp.h +2 -0

index 1661791..7de31d5 100644
@@ -373,4 +373,6 @@ struct udp_iter_state {
 #if IS_ENABLED(CONFIG_IPV6)
 void udpv6_encap_enable(void);
 #endif
+
+void udp_v4_early_demux_configure(int enable);
 #endif /* _UDP_H */

net/ipv4/af_inet.c +20 -2

index f750698..3e11d74 100644
@@ -1579,7 +1579,7 @@ u64 snmp_fold_field64(void __percpu *mib, int offt, size_t syncp_offset)
 };
 #endif
 
-static const struct net_protocol tcp_protocol = {
+static struct net_protocol tcp_protocol = {
        .early_demux    =       tcp_v4_early_demux,
        .handler        =       tcp_v4_rcv,
        .err_handler    =       tcp_v4_err,

@@ -1588,7 +1588,7 @@ u64 snmp_fold_field64(void __percpu *mib, int offt, size_t syncp_offset)
        .icmp_strict_tag_validation = 1,
 };
 
-static const struct net_protocol udp_protocol = {
+static struct net_protocol udp_protocol = {
        .early_demux =  udp_v4_early_demux,
        .handler =      udp_rcv,
        .err_handler =  udp_err,

@@ -1596,6 +1596,22 @@ u64 snmp_fold_field64(void __percpu *mib, int offt, size_t syncp_offset)
        .netns_ok =     1,
 };
 
+void tcp_v4_early_demux_configure(int enable)
+{
+       if (enable)
+               tcp_protocol.early_demux = tcp_v4_early_demux;
+       else
+               tcp_protocol.early_demux = NULL;
+}
+
+void udp_v4_early_demux_configure(int enable)
+{
+       if (enable)
+               udp_protocol.early_demux = udp_v4_early_demux;
+       else
+               udp_protocol.early_demux = NULL;
+}
+
 static const struct net_protocol icmp_protocol = {
        .handler =      icmp_rcv,
        .err_handler =  icmp_err,

@@ -1700,6 +1716,8 @@ static __net_init int inet_init_net(struct net *net)
        net->ipv4.sysctl_ip_default_ttl = IPDEFTTL;
        net->ipv4.sysctl_ip_dynaddr = 0;
        net->ipv4.sysctl_ip_early_demux = 1;
+       net->ipv4.sysctl_udp_early_demux = 1;
+       net->ipv4.sysctl_tcp_early_demux = 1;
 
        return 0;
 }

net/ipv4/sysctl_net_ipv4.c +47 -0

index b2fa498..c61383b 100644
@@ -253,6 +253,39 @@ static int proc_tcp_fastopen_key(struct ctl_table *ctl, int write,
        return ret;
 }
 
+static int proc_tcp_early_demux(struct ctl_table *table, int write,
+                               void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+       int ret = 0;
+
+       ret = proc_dointvec(table, write, buffer, lenp, ppos);
+
+       if (write && !ret) {
+               int enabled = init_net.ipv4.sysctl_tcp_early_demux;
+
+               tcp_v4_early_demux_configure(enabled);
+               tcp_v6_early_demux_configure(enabled);
+       }
+
+       return ret;
+}
+
+static int proc_udp_early_demux(struct ctl_table *table, int write,
+                               void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+       int ret = 0;
+
+       ret = proc_dointvec(table, write, buffer, lenp, ppos);
+
+       if (write && !ret) {
+               int enabled = init_net.ipv4.sysctl_udp_early_demux;
+
+               udp_v4_early_demux_configure(enabled);
+       }
+
+       return ret;
+}
+
 static struct ctl_table ipv4_table[] = {
        {
                .procname       = "tcp_timestamps",

@@ -737,6 +770,20 @@ static int proc_tcp_fastopen_key(struct ctl_table *ctl, int write,
                .proc_handler   = proc_dointvec
        },
        {
+               .procname       = "udp_early_demux",
+               .data           = &init_net.ipv4.sysctl_udp_early_demux,
+               .maxlen         = sizeof(int),
+               .mode           = 0644,
+               .proc_handler   = proc_udp_early_demux
+       },
+       {
+               .procname       = "tcp_early_demux",
+               .data           = &init_net.ipv4.sysctl_tcp_early_demux,
+               .maxlen         = sizeof(int),
+               .mode           = 0644,
+               .proc_handler   = proc_tcp_early_demux
+       },
+       {
                .procname       = "ip_default_ttl",
                .data           = &init_net.ipv4.sysctl_ip_default_ttl,
                .maxlen         = sizeof(int),

net/ipv6/tcp_ipv6.c +9 -2

index 4c60c6f..0dd761c 100644
@@ -1926,13 +1926,21 @@ struct proto tcpv6_prot = {
        .diag_destroy           = tcp_abort,
 };
 
-static const struct inet6_protocol tcpv6_protocol = {
+static struct inet6_protocol tcpv6_protocol = {
        .early_demux    =       tcp_v6_early_demux,
        .handler        =       tcp_v6_rcv,
        .err_handler    =       tcp_v6_err,
        .flags          =       INET6_PROTO_NOPOLICY|INET6_PROTO_FINAL,
 };
 
+void tcp_v6_early_demux_configure(int enable)
+{
+       if (enable)
+               tcpv6_protocol.early_demux = tcp_v6_early_demux;
+       else
+               tcpv6_protocol.early_demux = NULL;
+}
+
 static struct inet_protosw tcpv6_protosw = {
        .type           =       SOCK_STREAM,
        .protocol       =       IPPROTO_TCP,
--

1.9.1

Quoted 4 lines · Subash Abhinov Kasiviswanathan

Certain system process significant unconnected UDP workload. It would be preferrable to disable UDP early demux for those systems and enable it for TCP only.

Presumably you want this for performance reasons. Can you provide some before and after numbers?

Quoted 10 lines · Tom Herbert

Quoted 5 lines · Subash Abhinov Kasiviswanathan

Certain system process significant unconnected UDP workload. It would be preferrable to disable UDP early demux for those systems and enable it for TCP only.

Presumably you want this for performance reasons. Can you provide some before and after numbers?

Hi Tom

We are working on UDPv6 performance issues seen on an Android ARM64 system. Adding an early demux handler (link below) for it helped to increase performance
(800Mbps -> 870Mbps). This helps because Android statistics rules do multiple
socket lookup when no socket is associated with the skb.

https://www.mail-archive.com/[email protected]/msg157003.html

Eric mentioned that server loads usually see more unconnected load and he preferred to turn off early demux for UDP, hence this patch. I don't have numbers
for unconnected loads as of now though.

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Okay, now I'm confused. You're saying that when early demux was added for IPv6 performance improved, but this patch is allowing early demux to be disabled on the basis that it hurts performance for unconnected UDP workloads. While it's true that early demux in the case results in another UDP lookup, Eric's changes to make it lockless have made that lookup very cheap. So we really need numbers to justify this patch.

Even if the numbers were to show a benefit, we still have the problem that this creates a bimodal performance characteristic, e.g. what if the work load were 1/2 connected and 1/2 unconnected in real life, or what it the user incorrectly guesses the actual workload. Maybe a deeper solution to investigate is making early demux work with unconnected sockets.

Quoted 7 lines · Tom Herbert

Okay, now I'm confused. You're saying that when early demux was added for IPv6 performance improved, but this patch is allowing early demux to be disabled on the basis that it hurts performance for unconnected UDP workloads. While it's true that early demux in the case results in another UDP lookup, Eric's changes to make it lockless have made that lookup very cheap. So we really need numbers to justify this patch.

Fact that the lookup is lockless does not avoid a cache line miss.

Early demux computes a hash based on the 4-tuple, and lookups a hash table with does not fit in cpu caches.

A cache line miss per packet is expensive, when handling millions of UDP packets per second, (with millions of 4-tuples)

Quoted 6 lines

Even if the numbers were to show a benefit, we still have the problem that this creates a bimodal performance characteristic, e.g. what if the work load were 1/2 connected and 1/2 unconnected in real life, or what it the user incorrectly guesses the actual workload. Maybe a deeper solution to investigate is making early demux work with unconnected sockets.

Sure, but forcing all UDP applications to perform IP early demux is not better.

All these hypotheses are quite testable, and it should be obvious that if a patch is supposed to improve performance there should be some effort to quantify the impact.

Quoted 3 lines

All these hypotheses are quite testable, and it should be obvious that if a patch is supposed to improve performance there should be some effort to quantify the impact.

Hi Tom

I ran a single stream unconnected UDPv4 test on an ARM64 system and I see a slight increase in performance (782 -> 788Mbps) with udp demux disabled. A rudimentary app which does a sendto/recvfrom along with some statistics collection was used for this.

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,

a Linux Foundation Collaborative Project

Less than 1% performance improvement in a benchmark doesn't justify the complexity of the patch. Eric's hypothesis was that an unconnected UDP socket may show issues because of cache misses in look-ups due to so many different sources. This should be fairly easy to benchmark by randomly setting source address in your test (IP any and routing my need to be set appropriately).

Quoted 7 lines

Less than 1% performance improvement in a benchmark doesn't justify the complexity of the patch. Eric's hypothesis was that an unconnected UDP socket may show issues because of cache misses in look-ups due to so many different sources. This should be fairly easy to benchmark by randomly setting source address in your test (IP any and routing my need to be set appropriately).

With different source addresses, a larger increase is seen here (633->654Mbps).

Quoted 7 lines

Less than 1% performance improvement in a benchmark doesn't justify the complexity of the patch. Eric's hypothesis was that an unconnected UDP socket may show issues because of cache misses in look-ups due to so many different sources. This should be fairly easy to benchmark by randomly setting source address in your test (IP any and routing my need to be set appropriately).

With different source addresses, a larger increase is seen here (633->654Mbps).

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Certain system process significant unconnected UDP workload. It would be preferrable to disable UDP early demux for those systems and enable it for TCP only.

net/ipv4/af_inet.c +6 -0

+void tcp_v4_early_demux_configure(int enable)
+{
+	if (enable)
+		tcp_protocol.early_demux = tcp_v4_early_demux;
+	else
+		tcp_protocol.early_demux = NULL;

Well, then you need to make sure ipprot->early_demux is read once in the callers, like ip_rcv_finish(), otherwise compiler could read it twice and we could deref a NULL pointer.

ipprot = rcu_dereference(inet_protos[protocol]);

if (ipprot && ipprot->early_demux) {
    ipprot->early_demux(skb);   // crash