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

[Paolo Abeni <pabeni@redhat.com>]

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