| Message ID | 1411407496.26859.141.camel@edumazet-glaptop2.roam.corp.google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Sep 22, 2014 at 10:38 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > this_cpu_ptr() in preemptible context is generally bad > > Sep 22 05:05:55 br kernel: [ 94.608310] BUG: using smp_processor_id() > in > preemptible [00000000] code: ip/2261 > Sep 22 05:05:55 br kernel: [ 94.608316] caller is > tunnel_dst_set.isra.28+0x20/0x60 [ip_tunnel] > Sep 22 05:05:55 br kernel: [ 94.608319] CPU: 3 PID: 2261 Comm: ip Not > tainted > 3.17.0-rc5 #82 > > We can simply use raw_cpu_ptr(), as preemption is safe in these > contexts. > > Should fix https://bugzilla.kernel.org/show_bug.cgi?id=84991 > > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Joe <joe9mail@gmail.com> > Fixes: 9a4aa9af447f ("ipv4: Use percpu Cache route in IP tunnels") Acked-by: Tom Herbert <therbert@google.com> > --- > v2: use latest and shiny raw_cpu_ptr(), as it seems the latest > incantation of ever changing percpu interface. > > net/ipv4/ip_tunnel.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c > index afed1aac2638..bd41dd1948b6 100644 > --- a/net/ipv4/ip_tunnel.c > +++ b/net/ipv4/ip_tunnel.c > @@ -79,10 +79,10 @@ static void __tunnel_dst_set(struct ip_tunnel_dst *idst, > idst->saddr = saddr; > } > > -static void tunnel_dst_set(struct ip_tunnel *t, > +static noinline void tunnel_dst_set(struct ip_tunnel *t, > struct dst_entry *dst, __be32 saddr) > { > - __tunnel_dst_set(this_cpu_ptr(t->dst_cache), dst, saddr); > + __tunnel_dst_set(raw_cpu_ptr(t->dst_cache), dst, saddr); > } > > static void tunnel_dst_reset(struct ip_tunnel *t) > @@ -106,7 +106,7 @@ static struct rtable *tunnel_rtable_get(struct ip_tunnel *t, > struct dst_entry *dst; > > rcu_read_lock(); > - idst = this_cpu_ptr(t->dst_cache); > + idst = raw_cpu_ptr(t->dst_cache); > dst = rcu_dereference(idst->dst); > if (dst && !atomic_inc_not_zero(&dst->__refcnt)) > dst = NULL; > > -- 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
Hello Eric, Thanks for the quick fix. > > v2: use latest and shiny raw_cpu_ptr(), as it seems the latest > > incantation of ever changing percpu interface. > > > > net/ipv4/ip_tunnel.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > The below grep shows 3 uses of this_cpu_ptr, whereas the patch only replaces 2 instances. Just want to check if that is ok. grep --ignore-case --exclude-dir=".git" --recursive this_cpu_ptr ip_tunnel.c __tunnel_dst_set(this_cpu_ptr(t->dst_cache), dst, saddr); idst = this_cpu_ptr(t->dst_cache); tstats = this_cpu_ptr(tunnel->dev->tstats); Thanks Joe
Hello Eric, Tom, > > this_cpu_ptr() in preemptible context is generally bad > > > > Sep 22 05:05:55 br kernel: [ 94.608310] BUG: using smp_processor_id() > > in > > preemptible [00000000] code: ip/2261 > > Sep 22 05:05:55 br kernel: [ 94.608316] caller is > > tunnel_dst_set.isra.28+0x20/0x60 [ip_tunnel] > > Sep 22 05:05:55 br kernel: [ 94.608319] CPU: 3 PID: 2261 Comm: ip Not > > tainted > > 3.17.0-rc5 #82 > > > > We can simply use raw_cpu_ptr(), as preemption is safe in these > > contexts. > > > > Should fix https://bugzilla.kernel.org/show_bug.cgi?id=84991 > > Thanks, the fix worked. I do not see the BUG message anymore. Joe
Hello Eric, > > > v2: use latest and shiny raw_cpu_ptr(), as it seems the latest > > > incantation of ever changing percpu interface. > > > > > > net/ipv4/ip_tunnel.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > The below grep shows 3 uses of this_cpu_ptr, whereas the patch only > replaces 2 instances. Just want to check if that is ok. > > grep --ignore-case --exclude-dir=".git" --recursive this_cpu_ptr ip_tunnel.c > __tunnel_dst_set(this_cpu_ptr(t->dst_cache), dst, saddr); > idst = this_cpu_ptr(t->dst_cache); > tstats = this_cpu_ptr(tunnel->dev->tstats); grep --ignore-case --exclude-dir=".git" --recursive this_cpu_ptr *.c ip_input.c: struct ip_rt_acct *st = this_cpu_ptr(ip_rt_acct); ip_vti.c: tstats = this_cpu_ptr(dev->tstats); route.c: p = (struct rtable **)__this_cpu_ptr(nh->nh_pcpu_rth_output); route.c: prth = __this_cpu_ptr(nh->nh_pcpu_rth_output); tcp.c: return __this_cpu_ptr(p); Is it ok for ip_vti.c and ip_input.c to use this_cpu_ptr? Thanks Joe
On Mon, 2014-09-22 at 14:34 -0500, Joe M wrote: > p --ignore-case --exclude-dir=".git" --recursive this_cpu_ptr *.c > ip_input.c: struct ip_rt_acct *st = this_cpu_ptr(ip_rt_acct); > ip_vti.c: tstats = this_cpu_ptr(dev->tstats); > route.c: p = (struct rtable **)__this_cpu_ptr(nh->nh_pcpu_rth_output); > route.c: prth = __this_cpu_ptr(nh->nh_pcpu_rth_output); > tcp.c: return __this_cpu_ptr(p); > > Is it ok for ip_vti.c and ip_input.c to use this_cpu_ptr? It is ok. We run with BH disabled in these contexts. -- 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/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c index afed1aac2638..bd41dd1948b6 100644 --- a/net/ipv4/ip_tunnel.c +++ b/net/ipv4/ip_tunnel.c @@ -79,10 +79,10 @@ static void __tunnel_dst_set(struct ip_tunnel_dst *idst, idst->saddr = saddr; } -static void tunnel_dst_set(struct ip_tunnel *t, +static noinline void tunnel_dst_set(struct ip_tunnel *t, struct dst_entry *dst, __be32 saddr) { - __tunnel_dst_set(this_cpu_ptr(t->dst_cache), dst, saddr); + __tunnel_dst_set(raw_cpu_ptr(t->dst_cache), dst, saddr); } static void tunnel_dst_reset(struct ip_tunnel *t) @@ -106,7 +106,7 @@ static struct rtable *tunnel_rtable_get(struct ip_tunnel *t, struct dst_entry *dst; rcu_read_lock(); - idst = this_cpu_ptr(t->dst_cache); + idst = raw_cpu_ptr(t->dst_cache); dst = rcu_dereference(idst->dst); if (dst && !atomic_inc_not_zero(&dst->__refcnt)) dst = NULL;