net: introduce and use route hint

net/ipv4/ip_input.c +5 -0

+#else
+	       1;
+#endif
+}
+

Quoted 4 lines · Paolo Abeni

(quoted diff) +0 -0

@@ -535,9 +540,20 @@ static void ip_sublist_rcv_finish(struct list_head *head)
 	}
 }
 

Review static void ip_sublist_rcv_finish(struct list_head *head) · line 547

Quoted 5 lines

(quoted diff) +5 -0

+static bool ip_can_cache_route_hint(struct net *net, struct rtable *rt)
+{
+	return rt->rt_type != RTN_BROADCAST &&
+#ifdef CONFIG_IP_MULTIPLE_TABLES
+	       !net->ipv6.fib6_has_custom_rules;

that should be ipv4, not ipv6, right?

Also, for readability it would be better to have 2 helpers in include//net/fib_rules.h that return true false and manage the net namespace issue.

Quoted 3 lines

Also, for readability it would be better to have 2 helpers in include//net/fib_rules.h that return true false and manage the net namespace issue.

Double checking I parsed the above correctly. Do you mean something like the following - I think net/ip_fib.h fits more, as it already deals with CONFIG_IP_MULTIPLE_TABLES?

When doing RX batch packet processing, we currently always repeat the route lookup for each ingress packet. If policy routing is configured, and IPV6_SUBTREES is disabled at build time, we know that packets with the same destination address will use the same dst.

This change tries to avoid per packet route lookup caching the destination address of the latest successful lookup, and reusing it for the next packet when the above conditions are in place. Ingress traffic for most servers should fit.

The measured performance delta under UDP flood vs a recvmmsg receiver is as follow:

vanilla         patched         delta
Kpps            Kpps            %
1431            1664            +14

In the worst-case scenario - each packet has a different destination address - the performance delta is within noise range.

v1 -> v2:
- fix build issue with !CONFIG_IPV6_MULTIPLE_TABLES
- fix potential race when fib6_has_custom_rules is set while processing a packet batch

Signed-off-by Paolo Abeni <[email protected]>

---
 net/ipv6/ip6_input.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

net/ipv6/ip6_input.c +7 -1

index ef7f707d9ae3..f559ad6b09ef 100644
@@ -44,10 +44,16 @@
 #include <net/inet_ecn.h>
 #include <net/dst_metadata.h>
 
+struct ip6_route_input_hint {
+       unsigned long   refdst;
+       struct in6_addr daddr;
+};
+
 INDIRECT_CALLABLE_DECLARE(void udp_v6_early_demux(struct sk_buff *));
 INDIRECT_CALLABLE_DECLARE(void tcp_v6_early_demux(struct sk_buff *));
 static void ip6_rcv_finish_core(struct net *net, struct sock *sk,
-                               struct sk_buff *skb)
+                               struct sk_buff *skb,
+                               struct ip6_route_input_hint *hint)
 {
        void (*edemux)(struct sk_buff *skb);
 

@@ -59,7 +65,13 @@ static void ip6_rcv_finish_core(struct net *net, struct sock *sk,
                        INDIRECT_CALL_2(edemux, tcp_v6_early_demux,
                                        udp_v6_early_demux, skb);

net/ipv6/ip6_input.c +0 -0

 static void ip6_list_rcv_finish(struct net *net, struct sock *sk,
                                struct list_head *head)

{

net/ipv6/ip6_input.c +2 -1

+       struct ip6_route_input_hint _hint, *hint = NULL;
        struct dst_entry *curr_dst = NULL;
        struct sk_buff *skb, *next;
        struct list_head sublist;

@@ -104,9 +127,18 @@ static void ip6_list_rcv_finish(struct net *net, struct sock *sk,
                skb = l3mdev_ip6_rcv(skb);
                if (!skb)
                        continue;
-               ip6_rcv_finish_core(net, sk, skb);
+               ip6_rcv_finish_core(net, sk, skb, hint);

net/ipv6/ip6_input.c +1 -1

+
                        /* dispatch old sublist */
                        if (!list_empty(&sublist))
                                ip6_sublist_rcv_finish(&sublist);
--

2.21.0

Quoted 14 lines · Paolo Abeni

When doing RX batch packet processing, we currently always repeat the route lookup for each ingress packet. If policy routing is configured, and IPV6_SUBTREES is disabled at build time, we know that packets with the same destination address will use the same dst.

This change tries to avoid per packet route lookup caching the destination address of the latest successful lookup, and reusing it for the next packet when the above conditions are in place. Ingress traffic for most servers should fit.

The measured performance delta under UDP flood vs a recvmmsg receiver is as follow:

Quoted 3 lines

vanilla         patched         delta
Kpps            Kpps            %
1431            1664            +14

Since IPv4 speed-up is almost half and code considerably more complex, maybe only do IPv6?

Quoted 46 lines

In the worst-case scenario - each packet has a different destination address - the performance delta is within noise range.

v1 -> v2:
- fix build issue with !CONFIG_IPV6_MULTIPLE_TABLES
- fix potential race when fib6_has_custom_rules is set while processing a packet batch

Signed-off-by Paolo Abeni <[email protected]>

---
 net/ipv6/ip6_input.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

net/ipv6/ip6_input.c +12 -2

index ef7f707d9ae3..f559ad6b09ef 100644
@@ -44,10 +44,16 @@
 #include <net/inet_ecn.h>
 #include <net/dst_metadata.h>
 
+struct ip6_route_input_hint {
+       unsigned long   refdst;
+       struct in6_addr daddr;
+};
+
 INDIRECT_CALLABLE_DECLARE(void udp_v6_early_demux(struct sk_buff *));
 INDIRECT_CALLABLE_DECLARE(void tcp_v6_early_demux(struct sk_buff *));
 static void ip6_rcv_finish_core(struct net *net, struct sock *sk,
-                               struct sk_buff *skb)
+                               struct sk_buff *skb,
+                               struct ip6_route_input_hint *hint)
 {
        void (*edemux)(struct sk_buff *skb);
 

@@ -59,7 +65,13 @@ static void ip6_rcv_finish_core(struct net *net, struct sock *sk,
                        INDIRECT_CALL_2(edemux, tcp_v6_early_demux,
                                        udp_v6_early_demux, skb);
        }
-       if (!skb_valid_dst(skb))
+
+       if (skb_valid_dst(skb))
+               return;
+
+       if (hint && ipv6_addr_equal(&hint->daddr, &ipv6_hdr(skb)->daddr))

net/ipv6/ip6_input.c static void ip6_rcv_finish_core(struct net *net, struct sock *sk, · line 75

Quoted 3 lines

net/ipv6/ip6_input.c +2 -0

+               __skb_dst_copy(skb, hint->refdst);
+       else
                ip6_route_input(skb);

Is it possible to do the address comparison in ip6_list_rcv_finish itself and pass a pointer to refdst if safe? To avoid new struct definition, memcpy and to have all logic in one place. Need to keep a pointer to the prev skb, then, instead.

Quoted 19 lines

net/ipv6/ip6_input.c +0 -0

 static void ip6_list_rcv_finish(struct net *net, struct sock *sk,
                                struct list_head *head)

{

net/ipv6/ip6_input.c +6 -1

+       struct ip6_route_input_hint _hint, *hint = NULL;
        struct dst_entry *curr_dst = NULL;
        struct sk_buff *skb, *next;
        struct list_head sublist;

@@ -104,9 +127,18 @@ static void ip6_list_rcv_finish(struct net *net, struct sock *sk,
                skb = l3mdev_ip6_rcv(skb);
                if (!skb)
                        continue;
-               ip6_rcv_finish_core(net, sk, skb);
+               ip6_rcv_finish_core(net, sk, skb, hint);
                dst = skb_dst(skb);
                if (curr_dst != dst) {
+                       if (ip6_can_cache_route_hint(net)) {
+                               _hint.refdst = skb->_skb_refdst;
+                               memcpy(&_hint.daddr, &ipv6_hdr(skb)->daddr,
+                                      sizeof(_hint.daddr));

net/ipv6/ip6_input.c static void ip6_list_rcv_finish(struct net *net, struct sock *sk, · line 140

Quoted 4 lines

net/ipv6/ip6_input.c +4 -0

+                               hint = &_hint;
+                       } else {
+                               hint = NULL;
+                       }

not needed. ip6_can_cache_route_hit is the same for all iterations of the loop (indeed, compile time static), so if false, hint is never set.

Quoted 19 lines · Willem de Bruijn

Quoted 17 lines · Paolo Abeni

When doing RX batch packet processing, we currently always repeat the route lookup for each ingress packet. If policy routing is configured, and IPV6_SUBTREES is disabled at build time, we know that packets with the same destination address will use the same dst.

This change tries to avoid per packet route lookup caching the destination address of the latest successful lookup, and reusing it for the next packet when the above conditions are in place. Ingress traffic for most servers should fit.

The measured performance delta under UDP flood vs a recvmmsg receiver is as follow:

vanilla         patched         delta
Kpps            Kpps            %
1431            1664            +14

Quoted 2 lines

Since IPv4 speed-up is almost half and code considerably more complex, maybe only do IPv6?

uhmm... I would avoid that kind of assimmetry, and I would not look down on a 8% speedup, if possible.

Quoted 12 lines

Quoted 12 lines

net/ipv6/ip6_input.c +5 -1

@@ -104,9 +127,18 @@ static void ip6_list_rcv_finish(struct net *net, struct sock *sk,
                skb = l3mdev_ip6_rcv(skb);
                if (!skb)
                        continue;
-               ip6_rcv_finish_core(net, sk, skb);
+               ip6_rcv_finish_core(net, sk, skb, hint);
                dst = skb_dst(skb);
                if (curr_dst != dst) {
+                       if (ip6_can_cache_route_hint(net)) {
+                               _hint.refdst = skb->_skb_refdst;
+                               memcpy(&_hint.daddr, &ipv6_hdr(skb)->daddr,
+                                      sizeof(_hint.daddr));

Quoted 8 lines

net/ipv6/ip6_input.c static void ip6_list_rcv_finish(struct net *net, struct sock *sk, · line 140

Quoted 4 lines

net/ipv6/ip6_input.c +4 -0

+                               hint = &_hint;
+                       } else {
+                               hint = NULL;
+                       }

not needed. ip6_can_cache_route_hit is the same for all iterations of the loop (indeed, compile time static), so if false, hint is never set.

I think this is needed, instead: if CONFIG_MULTIPLE_TABLES=y, fib6_has_custom_rules can change at runtime - from 'false' to 'true'. If we don't reset 'hint', we could end-up with use-after-free.