On Mon, 2020-06-22 at 12:26 -0700, Brian Vazquez wrote:
>
>
> On Mon, Jun 22, 2020 at 11:00 AM Paolo Abeni <pab...@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 <pab...@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
