Re: [PATCH net-next 2/2] ipv6: fib6: avoid indirect calls from fib6_rule_lookup
From: Paolo Abeni <pabeni@redhat.com>
Date: 2020-06-22 18:00:19
Also in:
lkml
Subsystem:
networking [general], networking [ipv4/ipv6], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, David Ahern, Ido Schimmel, Linus Torvalds
Possibly related (same subject, not in this thread)
- 2020-06-23 · Re: [PATCH net-next 2/2] ipv6: fib6: avoid indirect calls from fib6_rule_lookup · Paolo Abeni <pabeni@redhat.com>
- 2020-06-22 · Re: [PATCH net-next 2/2] ipv6: fib6: avoid indirect calls from fib6_rule_lookup · Paolo Abeni <pabeni@redhat.com>
- 2020-06-20 · [PATCH net-next 2/2] ipv6: fib6: avoid indirect calls from fib6_rule_lookup · Brian Vazquez <hidden>
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 [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. 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)