diff mbox series

[net-next,3/5] ipv6: honor RT6_LOOKUP_F_DST_NOREF in rule lookup logic

Message ID 20190618182543.65477-4-tracywwnj@gmail.com
State Changes Requested
Delegated to: David Miller
Headers show
Series ipv6: avoid taking refcnt on dst during route lookup | expand

Commit Message

Wei Wang June 18, 2019, 6:25 p.m. UTC 
From: Wei Wang <weiwan@google.com>

This patch specifically converts the rule lookup logic to honor this
flag and not release refcnt when traversing each rule and calling
lookup() on each routing table.
Similar to previous patch, we also need some special handling of dst
entries in uncached list because there is always 1 refcnt taken for them
even if RT6_LOOKUP_F_DST_NOREF flag is set.

Signed-off-by: Wei Wang <weiwan@google.com>
Acked-by: Eric Dumazet <edumazet@google.com>
Acked-by: Mahesh Bandewar <maheshb@google.com>
---
 net/ipv6/fib6_rules.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

David Miller June 19, 2019, 4:07 p.m. UTC | #1 
From: Wei Wang <tracywwnj@gmail.com>
Date: Tue, 18 Jun 2019 11:25:41 -0700

> @@ -237,13 +240,16 @@ static int __fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
>  			goto out;
>  	}
>  again:
> -	ip6_rt_put(rt);
> +	if (!(flags & RT6_LOOKUP_F_DST_NOREF) ||
> +	    !list_empty(&rt->rt6i_uncached))
> +		ip6_rt_put(rt);

This conditional release logic, with the special treatment of uncache items
when using DST_NOREF, seems error prone.

Maybe you can put this logic into a helper like ip6_rt_put_any() and do the
list empty test etc. there?

	ip6_rt_put_any(struct rt6_info *rt, int flags);

What do you think?
Wei Wang June 19, 2019, 4:51 p.m. UTC | #2 
On Wed, Jun 19, 2019 at 9:07 AM David Miller <davem@davemloft.net> wrote:
>
> From: Wei Wang <tracywwnj@gmail.com>
> Date: Tue, 18 Jun 2019 11:25:41 -0700
>
> > @@ -237,13 +240,16 @@ static int __fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
> >                       goto out;
> >       }
> >  again:
> > -     ip6_rt_put(rt);
> > +     if (!(flags & RT6_LOOKUP_F_DST_NOREF) ||
> > +         !list_empty(&rt->rt6i_uncached))
> > +             ip6_rt_put(rt);
>
> This conditional release logic, with the special treatment of uncache items
> when using DST_NOREF, seems error prone.
>
> Maybe you can put this logic into a helper like ip6_rt_put_any() and do the
> list empty test etc. there?
>
>         ip6_rt_put_any(struct rt6_info *rt, int flags);
>
> What do you think?

Thanks David. Sure. Will update.
diff mbox series

Patch

diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index bcfae13409b5..9bbcf550cceb 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -113,14 +113,17 @@  struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
 		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(rt);
+		if (!(flags & RT6_LOOKUP_F_DST_NOREF))
+			ip6_rt_put(rt);
 		rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
 		if (rt->dst.error != -EAGAIN)
 			return &rt->dst;
-		ip6_rt_put(rt);
+		if (!(flags & RT6_LOOKUP_F_DST_NOREF))
+			ip6_rt_put(rt);
 	}
 
-	dst_hold(&net->ipv6.ip6_null_entry->dst);
+	if (!(flags & RT6_LOOKUP_F_DST_NOREF))
+		dst_hold(&net->ipv6.ip6_null_entry->dst);
 	return &net->ipv6.ip6_null_entry->dst;
 }
 
@@ -237,13 +240,16 @@  static int __fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
 			goto out;
 	}
 again:
-	ip6_rt_put(rt);
+	if (!(flags & RT6_LOOKUP_F_DST_NOREF) ||
+	    !list_empty(&rt->rt6i_uncached))
+		ip6_rt_put(rt);
 	err = -EAGAIN;
 	rt = NULL;
 	goto out;
 
 discard_pkt:
-	dst_hold(&rt->dst);
+	if (!(flags & RT6_LOOKUP_F_DST_NOREF))
+		dst_hold(&rt->dst);
 out:
 	res->rt6 = rt;
 	return err;