| Message ID | 1437522680-3515872-1-git-send-email-kafai@fb.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi, Martin KaFai Lau wrote: > The patch checks neigh->nud_state before acquiring the writer lock. > Note that rt6_probe() is only used in CONFIG_IPV6_ROUTER_PREF. You have to take "some" lock when accessing neigh->nud_state theoretically. > > I also take this chance to re-arrange the code. No, please do not mix multiple changes. > > 40 udpflood processes and a /64 gateway route are used. > The gateway has NUD_PERMANENT. Each of them is run for 30s. > At the end, the total number of finished sendto(): > > Before After > 55M 95M > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > Cc: Hannes Frederic Sowa <hannes@stressinduktion.org> > --- > net/ipv6/route.c | 41 ++++++++++++++++++++--------------------- > 1 file changed, 20 insertions(+), 21 deletions(-) > > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > index 6090969..a6c6b5a 100644 > --- a/net/ipv6/route.c > +++ b/net/ipv6/route.c > @@ -544,6 +544,7 @@ static void rt6_probe_deferred(struct work_struct *w) > > static void rt6_probe(struct rt6_info *rt) > { > + struct __rt6_probe_work *work; > struct neighbour *neigh; > /* > * Okay, this does not seem to be appropriate > @@ -558,34 +559,32 @@ static void rt6_probe(struct rt6_info *rt) > rcu_read_lock_bh(); > neigh = __ipv6_neigh_lookup_noref(rt->dst.dev, &rt->rt6i_gateway); > if (neigh) { > - write_lock(&neigh->lock); > if (neigh->nud_state & NUD_VALID) > goto out; > - } > - > - if (!neigh || > - time_after(jiffies, neigh->updated + rt->rt6i_idev->cnf.rtr_probe_interval)) { > - struct __rt6_probe_work *work; > > + work = NULL; > + write_lock(&neigh->lock); > + if (!(neigh->nud_state & NUD_VALID) && > + time_after(jiffies, neigh->updated + rt->rt6i_idev->cnf.rtr_probe_interval)) { > + work = kmalloc(sizeof(*work), GFP_ATOMIC); > + if (work) { > + __neigh_set_probe_once(neigh); > + } > + } > + write_unlock(&neigh->lock); > + } else { > work = kmalloc(sizeof(*work), GFP_ATOMIC); > + } > > - if (neigh && work) > - __neigh_set_probe_once(neigh); > - > - if (neigh) > - write_unlock(&neigh->lock); > + if (work) { > + INIT_WORK(&work->work, rt6_probe_deferred); > + work->target = rt->rt6i_gateway; > + dev_hold(rt->dst.dev); > + work->dev = rt->dst.dev; > + schedule_work(&work->work); > + } > > - if (work) { > - INIT_WORK(&work->work, rt6_probe_deferred); > - work->target = rt->rt6i_gateway; > - dev_hold(rt->dst.dev); > - work->dev = rt->dst.dev; > - schedule_work(&work->work); > - } > - } else { > out: > - write_unlock(&neigh->lock); > - } > rcu_read_unlock_bh(); > } > #else >
Hello, On Tue, 21 Jul 2015, Martin KaFai Lau wrote: > The patch checks neigh->nud_state before acquiring the writer lock. > Note that rt6_probe() is only used in CONFIG_IPV6_ROUTER_PREF. Locking usage is absolutely correct. > + if (!(neigh->nud_state & NUD_VALID) && > + time_after(jiffies, neigh->updated + rt->rt6i_idev->cnf.rtr_probe_interval)) { but this line is too long... > + work = kmalloc(sizeof(*work), GFP_ATOMIC); > + if (work) { > + __neigh_set_probe_once(neigh); > + } scripts/checkpatch.pl --strict /tmp/file.patch Regards -- Julian Anastasov <ja@ssi.bg> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 22, 2015 at 11:10:59AM +0900, YOSHIFUJI Hideaki wrote: > You have to take "some" lock when accessing neigh->nud_state > theoretically. I don't think read_lock can buy us a lot of extra protection either. If it has missed the train, the next ip6_pol_route() call will trigger rt6_probe(). There are other places checking nud_state without lock also. Thanks, --Martin -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv6/route.c b/net/ipv6/route.c index 6090969..a6c6b5a 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -544,6 +544,7 @@ static void rt6_probe_deferred(struct work_struct *w) static void rt6_probe(struct rt6_info *rt) { + struct __rt6_probe_work *work; struct neighbour *neigh; /* * Okay, this does not seem to be appropriate @@ -558,34 +559,32 @@ static void rt6_probe(struct rt6_info *rt) rcu_read_lock_bh(); neigh = __ipv6_neigh_lookup_noref(rt->dst.dev, &rt->rt6i_gateway); if (neigh) { - write_lock(&neigh->lock); if (neigh->nud_state & NUD_VALID) goto out; - } - - if (!neigh || - time_after(jiffies, neigh->updated + rt->rt6i_idev->cnf.rtr_probe_interval)) { - struct __rt6_probe_work *work; + work = NULL; + write_lock(&neigh->lock); + if (!(neigh->nud_state & NUD_VALID) && + time_after(jiffies, neigh->updated + rt->rt6i_idev->cnf.rtr_probe_interval)) { + work = kmalloc(sizeof(*work), GFP_ATOMIC); + if (work) { + __neigh_set_probe_once(neigh); + } + } + write_unlock(&neigh->lock); + } else { work = kmalloc(sizeof(*work), GFP_ATOMIC); + } - if (neigh && work) - __neigh_set_probe_once(neigh); - - if (neigh) - write_unlock(&neigh->lock); + if (work) { + INIT_WORK(&work->work, rt6_probe_deferred); + work->target = rt->rt6i_gateway; + dev_hold(rt->dst.dev); + work->dev = rt->dst.dev; + schedule_work(&work->work); + } - if (work) { - INIT_WORK(&work->work, rt6_probe_deferred); - work->target = rt->rt6i_gateway; - dev_hold(rt->dst.dev); - work->dev = rt->dst.dev; - schedule_work(&work->work); - } - } else { out: - write_unlock(&neigh->lock); - } rcu_read_unlock_bh(); } #else
The patch checks neigh->nud_state before acquiring the writer lock. Note that rt6_probe() is only used in CONFIG_IPV6_ROUTER_PREF. I also take this chance to re-arrange the code. 40 udpflood processes and a /64 gateway route are used. The gateway has NUD_PERMANENT. Each of them is run for 30s. At the end, the total number of finished sendto(): Before After 55M 95M Signed-off-by: Martin KaFai Lau <kafai@fb.com> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org> --- net/ipv6/route.c | 41 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 21 deletions(-)