Subject: [PATCH] net/ipv6: Use GFP_ATOMIC when a lock is held
in [Kernel]
|
From: Julia Lawall on 30 May 2010 15:50 From: Julia Lawall <julia(a)diku.dk> A spin lock is taken near the beginning of the enclosing function. The semantic patch that makes this change is as follows: (http://coccinelle.lip6.fr/) // <smpl> @@ @@ spin_lock(...) .... when != spin_unlock(...) -GFP_KERNEL +GFP_ATOMIC // </smpl> Signed-off-by: Julia Lawall <julia(a)diku.dk> --- net/ipv6/sit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff -u -p a/net/ipv6/sit.c b/net/ipv6/sit.c --- a/net/ipv6/sit.c +++ b/net/ipv6/sit.c @@ -358,7 +358,7 @@ ipip6_tunnel_add_prl(struct ip_tunnel *t goto out; } - p = kzalloc(sizeof(struct ip_tunnel_prl_entry), GFP_KERNEL); + p = kzalloc(sizeof(struct ip_tunnel_prl_entry), GFP_ATOMIC); if (!p) { err = -ENOBUFS; goto out; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Eric Dumazet on 30 May 2010 16:20 Le dimanche 30 mai 2010 à 21:48 +0200, Julia Lawall a écrit : > From: Julia Lawall <julia(a)diku.dk> > > A spin lock is taken near the beginning of the enclosing function. > > The semantic patch that makes this change is as follows: > (http://coccinelle.lip6.fr/) > > // <smpl> > @@ > @@ > > spin_lock(...) > ... when != spin_unlock(...) > -GFP_KERNEL > +GFP_ATOMIC > // </smpl> > > Signed-off-by: Julia Lawall <julia(a)diku.dk> > > --- > net/ipv6/sit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff -u -p a/net/ipv6/sit.c b/net/ipv6/sit.c > --- a/net/ipv6/sit.c > +++ b/net/ipv6/sit.c > @@ -358,7 +358,7 @@ ipip6_tunnel_add_prl(struct ip_tunnel *t > goto out; > } > > - p = kzalloc(sizeof(struct ip_tunnel_prl_entry), GFP_KERNEL); > + p = kzalloc(sizeof(struct ip_tunnel_prl_entry), GFP_ATOMIC); > if (!p) { > err = -ENOBUFS; > goto out; Nice catch, but what about allocating this outside of the locked section ? diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c index e51e650..ff3dd84 100644 --- a/net/ipv6/sit.c +++ b/net/ipv6/sit.c @@ -340,6 +340,10 @@ ipip6_tunnel_add_prl(struct ip_tunnel *t, struct ip_tunnel_prl *a, int chg) if (a->addr == htonl(INADDR_ANY)) return -EINVAL; + p = kzalloc(sizeof(struct ip_tunnel_prl_entry), GFP_KERNEL); + if (!p) + return -ENOBUFS; + spin_lock(&ipip6_prl_lock); for (p = t->prl; p; p = p->next) { @@ -358,19 +362,16 @@ ipip6_tunnel_add_prl(struct ip_tunnel *t, struct ip_tunnel_prl *a, int chg) goto out; } - p = kzalloc(sizeof(struct ip_tunnel_prl_entry), GFP_KERNEL); - if (!p) { - err = -ENOBUFS; - goto out; - } p->next = t->prl; p->addr = a->addr; p->flags = a->flags; t->prl_count++; rcu_assign_pointer(t->prl, p); + p = NULL; out: spin_unlock(&ipip6_prl_lock); + kfree(p); return err; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Julia Lawall on 30 May 2010 17:00 On Sun, 30 May 2010, Eric Dumazet wrote: > Le dimanche 30 mai 2010 � 21:48 +0200, Julia Lawall a �crit : > > From: Julia Lawall <julia(a)diku.dk> > > > > A spin lock is taken near the beginning of the enclosing function. > > > > The semantic patch that makes this change is as follows: > > (http://coccinelle.lip6.fr/) > > > > // <smpl> > > @@ > > @@ > > > > spin_lock(...) > > ... when != spin_unlock(...) > > -GFP_KERNEL > > +GFP_ATOMIC > > // </smpl> > > > > Signed-off-by: Julia Lawall <julia(a)diku.dk> > > > > --- > > net/ipv6/sit.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff -u -p a/net/ipv6/sit.c b/net/ipv6/sit.c > > --- a/net/ipv6/sit.c > > +++ b/net/ipv6/sit.c > > @@ -358,7 +358,7 @@ ipip6_tunnel_add_prl(struct ip_tunnel *t > > goto out; > > } > > > > - p = kzalloc(sizeof(struct ip_tunnel_prl_entry), GFP_KERNEL); > > + p = kzalloc(sizeof(struct ip_tunnel_prl_entry), GFP_ATOMIC); > > if (!p) { > > err = -ENOBUFS; > > goto out; > > Nice catch, but what about allocating this outside of the locked > section ? I think the proposed patch does not work, because the for loop overwrites p. That use of p looks like it is completely local to the for loop, so perhaps a new variable p1 could be added to be used there? julia > diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c > index e51e650..ff3dd84 100644 > --- a/net/ipv6/sit.c > +++ b/net/ipv6/sit.c > @@ -340,6 +340,10 @@ ipip6_tunnel_add_prl(struct ip_tunnel *t, struct ip_tunnel_prl *a, int chg) > if (a->addr == htonl(INADDR_ANY)) > return -EINVAL; > > + p = kzalloc(sizeof(struct ip_tunnel_prl_entry), GFP_KERNEL); > + if (!p) > + return -ENOBUFS; > + > spin_lock(&ipip6_prl_lock); > > for (p = t->prl; p; p = p->next) { > @@ -358,19 +362,16 @@ ipip6_tunnel_add_prl(struct ip_tunnel *t, struct ip_tunnel_prl *a, int chg) > goto out; > } > > - p = kzalloc(sizeof(struct ip_tunnel_prl_entry), GFP_KERNEL); > - if (!p) { > - err = -ENOBUFS; > - goto out; > - } > > p->next = t->prl; > p->addr = a->addr; > p->flags = a->flags; > t->prl_count++; > rcu_assign_pointer(t->prl, p); > + p = NULL; > out: > spin_unlock(&ipip6_prl_lock); > + kfree(p); > return err; > } > > > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
From: Eric Dumazet on 30 May 2010 17:00 Le dimanche 30 mai 2010 à 22:50 +0200, Julia Lawall a écrit : > I think the proposed patch does not work, because the for loop overwrites > p. That use of p looks like it is completely local to the for loop, so > perhaps a new variable p1 could be added to be used there? Please do so. I just wanted to tell you changing GFP_KERNEL to GFP_ATOMIC is not an appropriate way to solve this kind of problems. My patch was to get an idea, not a full and tested patch :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Julia Lawall on 30 May 2010 17:20 On Sun, 30 May 2010, Eric Dumazet wrote: > Le dimanche 30 mai 2010 � 22:50 +0200, Julia Lawall a �crit : > > > I think the proposed patch does not work, because the for loop overwrites > > p. That use of p looks like it is completely local to the for loop, so > > perhaps a new variable p1 could be added to be used there? > > Please do so. > > I just wanted to tell you changing GFP_KERNEL to GFP_ATOMIC is not an > appropriate way to solve this kind of problems. My patch was to get an > idea, not a full and tested patch :) Looking at it again, there is still a problem, because in the original code, the loop: for (p = t->prl; p; p = p->next) { if (p->addr == a->addr) { if (chg) { p->flags = a->flags; goto out; } err = -EEXIST; goto out; } } could exit with success without the kzalloc ever being called. If the kzalloc is moved up, it could fail and then it returns immediately without executing the loop. A solution could be to leave the NULL test on p where it is, and only move up the kzalloc. Or perhaps the change in behavior doesn't matter? julia
|
Next
|
Last
Pages: 1 2 Prev: [GIT PULL] mutex fix Next: [PATCH 1/3] kernel: Use GFP_ATOMIC when a lock is held |