Re: [PATCH net-next 2/2] ipv6: fib6: avoid indirect calls from fib6_rule_lookup

On Mon, 2020-06-22 at 12:26 -0700, Brian Vazquez wrote:

On Mon, Jun 22, 2020 at 11:00 AM Paolo Abeni [off-list ref] wrote:

quoted

On Mon, 2020-06-22 at 09:25 -0700, Brian Vazquez wrote:

quoted

Hi Paolo
On Mon, Jun 22, 2020 at 3:13 AM Paolo Abeni [off-list ref] wrote:

quoted

Hi,

On Fri, 2020-06-19 at 20:14 -0700, Brian Vazquez wrote:

quoted

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

quoted

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