linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 1/2] indirect_call_wrapper: extend indirect wrapper to support up to 4 calls
@ 2020-06-20  3:14 Brian Vazquez
  2020-06-20  3:14 ` [PATCH net-next 2/2] ipv6: fib6: avoid indirect calls from fib6_rule_lookup Brian Vazquez
  0 siblings, 1 reply; 5+ messages in thread
From: Brian Vazquez @ 2020-06-20  3:14 UTC (permalink / raw)
  To: Brian Vazquez, Brian Vazquez, Eric Dumazet, David S . Miller
  Cc: linux-kernel, netdev

There are many places where 2 annotations are not enough. This patch
adds INDIRECT_CALL_3 and INDIRECT_CALL_4 to cover such cases.

Signed-off-by: Brian Vazquez <brianvv@google.com>
---
 include/linux/indirect_call_wrapper.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/indirect_call_wrapper.h b/include/linux/indirect_call_wrapper.h
index 00d7e8e919c6..54c02c84906a 100644
--- a/include/linux/indirect_call_wrapper.h
+++ b/include/linux/indirect_call_wrapper.h
@@ -23,6 +23,16 @@
 		likely(f == f2) ? f2(__VA_ARGS__) :			\
 				  INDIRECT_CALL_1(f, f1, __VA_ARGS__);	\
 	})
+#define INDIRECT_CALL_3(f, f3, f2, f1, ...)					\
+	({									\
+		likely(f == f3) ? f3(__VA_ARGS__) :				\
+				  INDIRECT_CALL_2(f, f2, f1, __VA_ARGS__);	\
+	})
+#define INDIRECT_CALL_4(f, f4, f3, f2, f1, ...)					\
+	({									\
+		likely(f == f4) ? f4(__VA_ARGS__) :				\
+				  INDIRECT_CALL_3(f, f3, f2, f1, __VA_ARGS__);	\
+	})
 
 #define INDIRECT_CALLABLE_DECLARE(f)	f
 #define INDIRECT_CALLABLE_SCOPE
@@ -30,6 +40,8 @@
 #else
 #define INDIRECT_CALL_1(f, f1, ...) f(__VA_ARGS__)
 #define INDIRECT_CALL_2(f, f2, f1, ...) f(__VA_ARGS__)
+#define INDIRECT_CALL_3(f, f3, f2, f1, ...) f(__VA_ARGS__)
+#define INDIRECT_CALL_4(f, f4, f3, f2, f1, ...) f(__VA_ARGS__)
 #define INDIRECT_CALLABLE_DECLARE(f)
 #define INDIRECT_CALLABLE_SCOPE		static
 #endif
-- 
2.27.0.111.gc72c7da667-goog


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH net-next 2/2] ipv6: fib6: avoid indirect calls from fib6_rule_lookup
  2020-06-20  3:14 [PATCH net-next 1/2] indirect_call_wrapper: extend indirect wrapper to support up to 4 calls Brian Vazquez
@ 2020-06-20  3:14 ` Brian Vazquez
  2020-06-22 10:13   ` Paolo Abeni
  0 siblings, 1 reply; 5+ messages in thread
From: Brian Vazquez @ 2020-06-20  3:14 UTC (permalink / raw)
  To: Brian Vazquez, Brian Vazquez, Eric Dumazet, David S . Miller
  Cc: linux-kernel, netdev, Luigi Rizzo

It was reported that a considerable amount of cycles were spent on the
expensive indirect calls on fib6_rule_lookup. This patch introduces an
inline helper called pol_route_func that uses the indirect_call_wrappers
to avoid the indirect calls.

This patch saves around 50ns per call.

Performance was measured on the receiver by checking the amount of
syncookies that server was able to generate under a synflood load.

Traffic was generated using trafgen[1] which was pushing around 1Mpps on
a single queue. Receiver was using only one rx queue which help to
create a bottle neck and make the experiment rx-bounded.

These are the syncookies generated over 10s from the different runs:

Whithout the patch:
TcpExtSyncookiesSent            3553749            0.0
TcpExtSyncookiesSent            3550895            0.0
TcpExtSyncookiesSent            3553845            0.0
TcpExtSyncookiesSent            3541050            0.0
TcpExtSyncookiesSent            3539921            0.0
TcpExtSyncookiesSent            3557659            0.0
TcpExtSyncookiesSent            3526812            0.0
TcpExtSyncookiesSent            3536121            0.0
TcpExtSyncookiesSent            3529963            0.0
TcpExtSyncookiesSent            3536319            0.0

With the patch:
TcpExtSyncookiesSent            3611786            0.0
TcpExtSyncookiesSent            3596682            0.0
TcpExtSyncookiesSent            3606878            0.0
TcpExtSyncookiesSent            3599564            0.0
TcpExtSyncookiesSent            3601304            0.0
TcpExtSyncookiesSent            3609249            0.0
TcpExtSyncookiesSent            3617437            0.0
TcpExtSyncookiesSent            3608765            0.0
TcpExtSyncookiesSent            3620205            0.0
TcpExtSyncookiesSent            3601895            0.0

Without the patch the average is 354263 pkt/s or 2822 ns/pkt and with
the patch the average is 360738 pkt/s or 2772 ns/pkt which gives an
estimate of 50 ns per packet.

[1] http://netsniff-ng.org/

Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Brian Vazquez <brianvv@google.com>
Cc: Luigi Rizzo <lrizzo@google.com>
---
 include/net/ip6_fib.h | 36 ++++++++++++++++++++++++++++++++++++
 net/ipv6/fib6_rules.c |  9 ++++++---
 net/ipv6/ip6_fib.c    |  3 ++-
 net/ipv6/route.c      |  8 ++++----
 4 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 3f615a29766e..0ff7e97216d4 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -19,6 +19,7 @@
 #include <net/netlink.h>
 #include <net/inetpeer.h>
 #include <net/fib_notifier.h>
+#include <linux/indirect_call_wrapper.h>
 
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 #define FIB6_TABLE_HASHSZ 256
@@ -552,6 +553,41 @@ struct bpf_iter__ipv6_route {
 };
 #endif
 
+INDIRECT_CALLABLE_DECLARE(struct rt6_info *ip6_pol_route_output(struct net *net,
+					     struct fib6_table *table,
+					     struct flowi6 *fl6,
+					     const struct sk_buff *skb,
+					     int flags));
+INDIRECT_CALLABLE_DECLARE(struct rt6_info *ip6_pol_route_input(struct net *net,
+					     struct fib6_table *table,
+					     struct flowi6 *fl6,
+					     const struct sk_buff *skb,
+					     int flags));
+INDIRECT_CALLABLE_DECLARE(struct rt6_info *__ip6_route_redirect(struct net *net,
+					     struct fib6_table *table,
+					     struct flowi6 *fl6,
+					     const struct sk_buff *skb,
+					     int flags));
+INDIRECT_CALLABLE_DECLARE(struct rt6_info *ip6_pol_route_lookup(struct net *net,
+					     struct fib6_table *table,
+					     struct flowi6 *fl6,
+					     const struct sk_buff *skb,
+					     int flags));
+static inline struct rt6_info *pol_lookup_func(pol_lookup_t lookup,
+						struct net *net,
+						struct fib6_table *table,
+						struct flowi6 *fl6,
+						const struct sk_buff *skb,
+						int flags)
+{
+	return INDIRECT_CALL_4(lookup,
+			       ip6_pol_route_lookup,
+			       ip6_pol_route_output,
+			       ip6_pol_route_input,
+			       __ip6_route_redirect,
+			       net, table, fl6, skb, flags);
+}
+
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 static inline bool fib6_has_custom_rules(const struct net *net)
 {
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index fafe556d21e0..6053ef851555 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -111,11 +111,13 @@ struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
 	} else {
 		struct rt6_info *rt;
 
-		rt = lookup(net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
+		rt = pol_lookup_func(lookup,
+			     net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
 		if (rt != net->ipv6.ip6_null_entry && rt->dst.error != -EAGAIN)
 			return &rt->dst;
 		ip6_rt_put_flags(rt, flags);
-		rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
+		rt = pol_lookup_func(lookup,
+			     net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
 		if (rt->dst.error != -EAGAIN)
 			return &rt->dst;
 		ip6_rt_put_flags(rt, flags);
@@ -226,7 +228,8 @@ static int __fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
 		goto out;
 	}
 
-	rt = lookup(net, table, flp6, arg->lookup_data, flags);
+	rt = pol_lookup_func(lookup,
+			     net, table, flp6, arg->lookup_data, flags);
 	if (rt != net->ipv6.ip6_null_entry) {
 		err = fib6_rule_saddr(net, rule, flags, flp6,
 				      ip6_dst_idev(&rt->dst)->dev);
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 49ee89bbcba0..25a90f3f705c 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -314,7 +314,8 @@ struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
 {
 	struct rt6_info *rt;
 
-	rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
+	rt = pol_lookup_func(lookup,
+			net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
 	if (rt->dst.error == -EAGAIN) {
 		ip6_rt_put_flags(rt, flags);
 		rt = net->ipv6.ip6_null_entry;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 82cbb46a2a4f..5852039ca9cf 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1207,7 +1207,7 @@ static struct rt6_info *ip6_create_rt_rcu(const struct fib6_result *res)
 	return nrt;
 }
 
-static struct rt6_info *ip6_pol_route_lookup(struct net *net,
+INDIRECT_CALLABLE_SCOPE struct rt6_info *ip6_pol_route_lookup(struct net *net,
 					     struct fib6_table *table,
 					     struct flowi6 *fl6,
 					     const struct sk_buff *skb,
@@ -2274,7 +2274,7 @@ struct rt6_info *ip6_pol_route(struct net *net, struct fib6_table *table,
 }
 EXPORT_SYMBOL_GPL(ip6_pol_route);
 
-static struct rt6_info *ip6_pol_route_input(struct net *net,
+INDIRECT_CALLABLE_SCOPE struct rt6_info *ip6_pol_route_input(struct net *net,
 					    struct fib6_table *table,
 					    struct flowi6 *fl6,
 					    const struct sk_buff *skb,
@@ -2465,7 +2465,7 @@ void ip6_route_input(struct sk_buff *skb)
 						      &fl6, skb, flags));
 }
 
-static struct rt6_info *ip6_pol_route_output(struct net *net,
+INDIRECT_CALLABLE_SCOPE struct rt6_info *ip6_pol_route_output(struct net *net,
 					     struct fib6_table *table,
 					     struct flowi6 *fl6,
 					     const struct sk_buff *skb,
@@ -2912,7 +2912,7 @@ struct ip6rd_flowi {
 	struct in6_addr gateway;
 };
 
-static struct rt6_info *__ip6_route_redirect(struct net *net,
+INDIRECT_CALLABLE_SCOPE struct rt6_info *__ip6_route_redirect(struct net *net,
 					     struct fib6_table *table,
 					     struct flowi6 *fl6,
 					     const struct sk_buff *skb,
-- 
2.27.0.111.gc72c7da667-goog


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next 2/2] ipv6: fib6: avoid indirect calls from fib6_rule_lookup
  2020-06-20  3:14 ` [PATCH net-next 2/2] ipv6: fib6: avoid indirect calls from fib6_rule_lookup Brian Vazquez
@ 2020-06-22 10:13   ` Paolo Abeni
       [not found]     ` <CAMzD94QUSR8T3vMAfjVf_MxCPrj_gtPYhEqNCyPqsJ1rdA=G9g@mail.gmail.com>
  0 siblings, 1 reply; 5+ messages in thread
From: Paolo Abeni @ 2020-06-22 10:13 UTC (permalink / raw)
  To: Brian Vazquez, Brian Vazquez, Eric Dumazet, David S . Miller
  Cc: linux-kernel, netdev, Luigi Rizzo

Hi,

On Fri, 2020-06-19 at 20:14 -0700, Brian Vazquez wrote:
> @@ -111,11 +111,13 @@ struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
>  	} else {
>  		struct rt6_info *rt;
>  
> -		rt = lookup(net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
> +		rt = pol_lookup_func(lookup,
> +			     net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
>  		if (rt != net->ipv6.ip6_null_entry && rt->dst.error != -EAGAIN)
>  			return &rt->dst;
>  		ip6_rt_put_flags(rt, flags);
> -		rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
> +		rt = pol_lookup_func(lookup,
> +			     net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
>  		if (rt->dst.error != -EAGAIN)
>  			return &rt->dst;
>  		ip6_rt_put_flags(rt, flags);

Have you considered instead factoring out the slice of
fib6_rule_lookup() using indirect calls to an header file? it looks
like here (gcc 10.1.1) it sufficent let the compiler use direct calls
and will avoid the additional branches.

Thanks!

Paolo



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next 2/2] ipv6: fib6: avoid indirect calls from fib6_rule_lookup
       [not found]     ` <CAMzD94QUSR8T3vMAfjVf_MxCPrj_gtPYhEqNCyPqsJ1rdA=G9g@mail.gmail.com>
@ 2020-06-22 18:00       ` Paolo Abeni
       [not found]         ` <CAMzD94T8Sm8_8B2=3iST8P6RZOnUEAEViM4Q5FKdTh7keOpd5Q@mail.gmail.com>
  0 siblings, 1 reply; 5+ messages in thread
From: Paolo Abeni @ 2020-06-22 18:00 UTC (permalink / raw)
  To: Brian Vazquez
  Cc: Brian Vazquez, Eric Dumazet, David S . Miller, open list,
	Linux NetDev, Luigi Rizzo

On Mon, 2020-06-22 at 09:25 -0700, Brian Vazquez wrote:
> 
> Hi Paolo
> On Mon, Jun 22, 2020 at 3:13 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > Hi,
> > 
> > On Fri, 2020-06-19 at 20:14 -0700, Brian Vazquez wrote:
> > > @@ -111,11 +111,13 @@ struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
> > >       } else {
> > >               struct rt6_info *rt;
> > >  
> > > -             rt = lookup(net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
> > > +             rt = pol_lookup_func(lookup,
> > > +                          net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
> > >               if (rt != net->ipv6.ip6_null_entry && rt->dst.error != -EAGAIN)
> > >                       return &rt->dst;
> > >               ip6_rt_put_flags(rt, flags);
> > > -             rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
> > > +             rt = pol_lookup_func(lookup,
> > > +                          net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
> > >               if (rt->dst.error != -EAGAIN)
> > >                       return &rt->dst;
> > >               ip6_rt_put_flags(rt, flags);
> > 
> > Have you considered instead factoring out the slice of
> > fib6_rule_lookup() using indirect calls to an header file? it looks
> > like here (gcc 10.1.1) it sufficent let the compiler use direct calls
> > and will avoid the additional branches.
> > 
> 
> Sorry I couldn't get your point. Could you elaborate more, please? Or provide an example?

I mean: I think we can avoid the indirect calls even without using the
ICW, just moving the relevant code around - in a inline function in the
header files.

e.g. with the following code - rough, build-tested only experiment -
the gcc 10.1.1 is able to use direct calls
for ip6_pol_route_lookup(), ip6_pol_route_output(), etc.

Cheers,

Paolo
---
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 3f615a29766e..c1b5ac890cd2 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -430,9 +430,6 @@ struct fib6_entry_notifier_info {
 
 struct fib6_table *fib6_get_table(struct net *net, u32 id);
 struct fib6_table *fib6_new_table(struct net *net, u32 id);
-struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
-				   const struct sk_buff *skb,
-				   int flags, pol_lookup_t lookup);
 
 /* called with rcu lock held; can return error pointer
  * caller needs to select path
diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index 2a5277758379..fe1c2102ffe8 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -336,4 +336,50 @@ u32 ip6_mtu_from_fib6(const struct fib6_result *res,
 struct neighbour *ip6_neigh_lookup(const struct in6_addr *gw,
 				   struct net_device *dev, struct sk_buff *skb,
 				   const void *daddr);
+
+#ifdef CONFIG_IPV6_MULTIPLE_TABLES
+struct rt6_info *__fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
+				    const struct sk_buff *skb,
+				    int flags, pol_lookup_t lookup);
+static inline struct dst_entry *
+fib6_rule_lookup(struct net *net, struct flowi6 *fl6, const struct sk_buff *skb,
+		 int flags, pol_lookup_t lookup)
+{
+	struct rt6_info *rt;
+
+	if (!net->ipv6.fib6_has_custom_rules) {
+		rt = lookup(net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
+		if (rt != net->ipv6.ip6_null_entry && rt->dst.error != -EAGAIN)
+			return &rt->dst;
+		ip6_rt_put_flags(rt, flags);
+		rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
+		if (rt->dst.error != -EAGAIN)
+			return &rt->dst;
+		ip6_rt_put_flags(rt, flags);
+	} else {
+		rt = __fib6_rule_lookup(net, fl6, skb, flags, lookup);
+	}
+
+	if (!(flags & RT6_LOOKUP_F_DST_NOREF))
+		dst_hold(&net->ipv6.ip6_null_entry->dst);
+	return &net->ipv6.ip6_null_entry->dst;
+}
+#else
+static inline struct dst_entry *
+fib6_rule_lookup(struct net *net, struct flowi6 *fl6, const struct sk_buff *skb,
+		 int flags, pol_lookup_t lookup)
+{
+	struct rt6_info *rt;
+
+	rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
+	if (rt->dst.error == -EAGAIN) {
+		ip6_rt_put_flags(rt, flags);
+		rt = net->ipv6.ip6_null_entry;
+		if (!(flags & RT6_LOOKUP_F_DST_NOREF))
+			dst_hold(&rt->dst);
+	}
+
+	return &rt->dst;
+}
+#endif
 #endif
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index fafe556d21e0..4672cb04c562 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -87,43 +87,27 @@ int fib6_lookup(struct net *net, int oif, struct flowi6 *fl6,
 	return err;
 }
 
-struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
-				   const struct sk_buff *skb,
-				   int flags, pol_lookup_t lookup)
+struct rt6_info *__fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
+				  const struct sk_buff *skb,
+				  int flags, pol_lookup_t lookup)
 {
-	if (net->ipv6.fib6_has_custom_rules) {
-		struct fib6_result res = {};
-		struct fib_lookup_arg arg = {
-			.lookup_ptr = lookup,
-			.lookup_data = skb,
-			.result = &res,
-			.flags = FIB_LOOKUP_NOREF,
-		};
-
-		/* update flow if oif or iif point to device enslaved to l3mdev */
-		l3mdev_update_flow(net, flowi6_to_flowi(fl6));
-
-		fib_rules_lookup(net->ipv6.fib6_rules_ops,
-				 flowi6_to_flowi(fl6), flags, &arg);
-
-		if (res.rt6)
-			return &res.rt6->dst;
-	} else {
-		struct rt6_info *rt;
-
-		rt = lookup(net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
-		if (rt != net->ipv6.ip6_null_entry && rt->dst.error != -EAGAIN)
-			return &rt->dst;
-		ip6_rt_put_flags(rt, flags);
-		rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
-		if (rt->dst.error != -EAGAIN)
-			return &rt->dst;
-		ip6_rt_put_flags(rt, flags);
-	}
-
-	if (!(flags & RT6_LOOKUP_F_DST_NOREF))
-		dst_hold(&net->ipv6.ip6_null_entry->dst);
-	return &net->ipv6.ip6_null_entry->dst;
+	struct fib6_result res = {};
+	struct fib_lookup_arg arg = {
+		.lookup_ptr = lookup,
+		.lookup_data = skb,
+		.result = &res,
+		.flags = FIB_LOOKUP_NOREF,
+	};
+
+	/* update flow if oif or iif point to device enslaved to l3mdev */
+	l3mdev_update_flow(net, flowi6_to_flowi(fl6));
+
+	fib_rules_lookup(net->ipv6.fib6_rules_ops,
+			 flowi6_to_flowi(fl6), flags, &arg);
+
+	if (res.rt6)
+		return res.rt6;
+	return NULL;
 }
 
 static int fib6_rule_saddr(struct net *net, struct fib_rule *rule, int flags,
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 49ee89bbcba0..242d4cdd2666 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -308,23 +308,6 @@ struct fib6_table *fib6_get_table(struct net *net, u32 id)
 	  return net->ipv6.fib6_main_tbl;
 }
 
-struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
-				   const struct sk_buff *skb,
-				   int flags, pol_lookup_t lookup)
-{
-	struct rt6_info *rt;
-
-	rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
-	if (rt->dst.error == -EAGAIN) {
-		ip6_rt_put_flags(rt, flags);
-		rt = net->ipv6.ip6_null_entry;
-		if (!(flags & RT6_LOOKUP_F_DST_NOREF))
-			dst_hold(&rt->dst);
-	}
-
-	return &rt->dst;
-}
-
 /* called with rcu lock held; no reference taken on fib6_info */
 int fib6_lookup(struct net *net, int oif, struct flowi6 *fl6,
 		struct fib6_result *res, int flags)


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next 2/2] ipv6: fib6: avoid indirect calls from fib6_rule_lookup
       [not found]         ` <CAMzD94T8Sm8_8B2=3iST8P6RZOnUEAEViM4Q5FKdTh7keOpd5Q@mail.gmail.com>
@ 2020-06-23  8:26           ` Paolo Abeni
  0 siblings, 0 replies; 5+ messages in thread
From: Paolo Abeni @ 2020-06-23  8:26 UTC (permalink / raw)
  To: Brian Vazquez
  Cc: Brian Vazquez, Eric Dumazet, David S . Miller, open list,
	Linux NetDev, Luigi Rizzo

On Mon, 2020-06-22 at 12:26 -0700, Brian Vazquez wrote:
> 
> 
> On Mon, Jun 22, 2020 at 11:00 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > On Mon, 2020-06-22 at 09:25 -0700, Brian Vazquez wrote:
> > > 
> > > Hi Paolo
> > > On Mon, Jun 22, 2020 at 3:13 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > > > Hi,
> > > > 
> > > > On Fri, 2020-06-19 at 20:14 -0700, Brian Vazquez wrote:
> > > > > @@ -111,11 +111,13 @@ struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
> > > > >       } else {
> > > > >               struct rt6_info *rt;
> > > > >  
> > > > > -             rt = lookup(net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
> > > > > +             rt = pol_lookup_func(lookup,
> > > > > +                          net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
> > > > >               if (rt != net->ipv6.ip6_null_entry && rt->dst.error != -EAGAIN)
> > > > >                       return &rt->dst;
> > > > >               ip6_rt_put_flags(rt, flags);
> > > > > -             rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
> > > > > +             rt = pol_lookup_func(lookup,
> > > > > +                          net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
> > > > >               if (rt->dst.error != -EAGAIN)
> > > > >                       return &rt->dst;
> > > > >               ip6_rt_put_flags(rt, flags);
> > > > 
> > > > Have you considered instead factoring out the slice of
> > > > fib6_rule_lookup() using indirect calls to an header file? it looks
> > > > like here (gcc 10.1.1) it sufficent let the compiler use direct calls
> > > > and will avoid the additional branches.
> > > > 
> > > 
> > > Sorry I couldn't get your point. Could you elaborate more, please? Or provide an example?
> > 
> > I mean: I think we can avoid the indirect calls even without using the
> > ICW, just moving the relevant code around - in a inline function in the
> > header files.
> 
>  
> Oh I see your point now, yeah this could work but only for the path where there's no custom_rules, right?? So we still need the ICW for the other case. 
> 
> > e.g. with the following code - rough, build-tested only experiment -
> > the gcc 10.1.1 is able to use direct calls
> > for ip6_pol_route_lookup(), ip6_pol_route_output(), etc.
> > 
> > Cheers,
> > 
> > Paolo
> > ---
> > diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
> > index 3f615a29766e..c1b5ac890cd2 100644
> > --- a/include/net/ip6_fib.h
> > +++ b/include/net/ip6_fib.h
> > @@ -430,9 +430,6 @@ struct fib6_entry_notifier_info {
> > 
> >  struct fib6_table *fib6_get_table(struct net *net, u32 id);
> >  struct fib6_table *fib6_new_table(struct net *net, u32 id);
> > -struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
> > -                                  const struct sk_buff *skb,
> > -                                  int flags, pol_lookup_t lookup);
> > 
> >  /* called with rcu lock held; can return error pointer
> >   * caller needs to select path
> > diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> > index 2a5277758379..fe1c2102ffe8 100644
> > --- a/include/net/ip6_route.h
> > +++ b/include/net/ip6_route.h
> > @@ -336,4 +336,50 @@ u32 ip6_mtu_from_fib6(const struct fib6_result *res,
> >  struct neighbour *ip6_neigh_lookup(const struct in6_addr *gw,
> >                                    struct net_device *dev, struct sk_buff *skb,
> >                                    const void *daddr);
> > +
> > +#ifdef CONFIG_IPV6_MULTIPLE_TABLES
> > +struct rt6_info *__fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
> > +                                   const struct sk_buff *skb,
> > +                                   int flags, pol_lookup_t lookup);
> > +static inline struct dst_entry *
> > +fib6_rule_lookup(struct net *net, struct flowi6 *fl6, const struct sk_buff *skb,
> > +                int flags, pol_lookup_t lookup)
> > +{
> > +       struct rt6_info *rt;
> > +
> > +       if (!net->ipv6.fib6_has_custom_rules) {
> > +               rt = lookup(net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
> > +               if (rt != net->ipv6.ip6_null_entry && rt->dst.error != -EAGAIN)
> > +                       return &rt->dst;
> > +               ip6_rt_put_flags(rt, flags);
> > +               rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
> > +               if (rt->dst.error != -EAGAIN)
> > +                       return &rt->dst;
> > +               ip6_rt_put_flags(rt, flags);
> > +       } else {
> > +               rt = __fib6_rule_lookup(net, fl6, skb, flags, lookup); 
> 
>  
> When we have custom rules this function will do the following stack trace:
> fib_rules_lookup
> |_  fib6_rule_action
>       |_ __fib6_rule_action
>   So we will still need the ICW in __fib6_rule_action, right?

Indeed! I originally did not notice your patch takes care also of the
custum rules code path.

I'm ok with plain usage of ICW everywhere.

Just an additional question, why this order:

+       return INDIRECT_CALL_4(lookup,
+                              ip6_pol_route_lookup,
+                              ip6_pol_route_output,
+                              ip6_pol_route_input,
+                              __ip6_route_redirect,

?

aren't *route_output and *route_input more frequent than *route_lookup?

Thanks!

Paolo


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-06-23  8:26 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-20  3:14 [PATCH net-next 1/2] indirect_call_wrapper: extend indirect wrapper to support up to 4 calls Brian Vazquez
2020-06-20  3:14 ` [PATCH net-next 2/2] ipv6: fib6: avoid indirect calls from fib6_rule_lookup Brian Vazquez
2020-06-22 10:13   ` Paolo Abeni
     [not found]     ` <CAMzD94QUSR8T3vMAfjVf_MxCPrj_gtPYhEqNCyPqsJ1rdA=G9g@mail.gmail.com>
2020-06-22 18:00       ` Paolo Abeni
     [not found]         ` <CAMzD94T8Sm8_8B2=3iST8P6RZOnUEAEViM4Q5FKdTh7keOpd5Q@mail.gmail.com>
2020-06-23  8:26           ` Paolo Abeni

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).