[PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Paul E. McKenney]

Hello!

Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
in RCU callbacks accessing a kmem_cache after it had been destroyed.

The following untested (might not even compile) patch proposes a fix.

Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---

 slab.c |    2 +-
 slob.c |    2 ++
 slub.c |    2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/slab.c b/mm/slab.c
index e74a16e..5241b65 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2547,7 +2547,7 @@ void kmem_cache_destroy(struct kmem_cache *cachep)
 	}
 
 	if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
-		synchronize_rcu();
+		rcu_barrier();
 
 	__kmem_cache_destroy(cachep);
 	mutex_unlock(&cache_chain_mutex);
diff --git a/mm/slob.c b/mm/slob.c
index c78742d..9641da3 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -595,6 +595,8 @@ EXPORT_SYMBOL(kmem_cache_create);
 void kmem_cache_destroy(struct kmem_cache *c)
 {
 	kmemleak_free(c);
+	if (c->flags & SLAB_DESTROY_BY_RCU)
+		rcu_barrier();
 	slob_free(c, sizeof(struct kmem_cache));
 }
 EXPORT_SYMBOL(kmem_cache_destroy);
diff --git a/mm/slub.c b/mm/slub.c
index 819f056..a9201d8 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2595,6 +2595,8 @@ static inline int kmem_cache_close(struct kmem_cache *s)
  */
 void kmem_cache_destroy(struct kmem_cache *s)
 {
+	if (s->flags & SLAB_DESTROY_BY_RCU)
+		rcu_barrier();
 	down_write(&slub_lock);
 	s->refcount--;
 	if (!s->refcount) {

[PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Paul E. McKenney]

Hello!

Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
in RCU callbacks accessing a kmem_cache after it had been destroyed.

The following untested (might not even compile) patch proposes a fix.

Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---

 slab.c |    2 +-
 slob.c |    2 ++
 slub.c |    2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/slab.c b/mm/slab.c
index e74a16e..5241b65 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2547,7 +2547,7 @@ void kmem_cache_destroy(struct kmem_cache *cachep)
 	}
 
 	if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
-		synchronize_rcu();
+		rcu_barrier();
 
 	__kmem_cache_destroy(cachep);
 	mutex_unlock(&cache_chain_mutex);
diff --git a/mm/slob.c b/mm/slob.c
index c78742d..9641da3 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -595,6 +595,8 @@ EXPORT_SYMBOL(kmem_cache_create);
 void kmem_cache_destroy(struct kmem_cache *c)
 {
 	kmemleak_free(c);
+	if (c->flags & SLAB_DESTROY_BY_RCU)
+		rcu_barrier();
 	slob_free(c, sizeof(struct kmem_cache));
 }
 EXPORT_SYMBOL(kmem_cache_destroy);
diff --git a/mm/slub.c b/mm/slub.c
index 819f056..a9201d8 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2595,6 +2595,8 @@ static inline int kmem_cache_close(struct kmem_cache *s)
  */
 void kmem_cache_destroy(struct kmem_cache *s)
 {
+	if (s->flags & SLAB_DESTROY_BY_RCU)
+		rcu_barrier();
 	down_write(&slub_lock);
 	s->refcount--;
 	if (!s->refcount) {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Matt Mackall]

On Thu, 2009-06-25 at 12:31 -0700, Paul E. McKenney wrote:
> Hello!
> 
> Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> in RCU callbacks accessing a kmem_cache after it had been destroyed.
> 
> The following untested (might not even compile) patch proposes a fix.

Acked-by: Matt Mackall <mpm@selenic.com>

Nick, you'll want to make sure you get this in SLQB.

> Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
> 
>  slab.c |    2 +-
>  slob.c |    2 ++
>  slub.c |    2 ++
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index e74a16e..5241b65 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2547,7 +2547,7 @@ void kmem_cache_destroy(struct kmem_cache *cachep)
>  	}
>  
>  	if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
> -		synchronize_rcu();
> +		rcu_barrier();
>  
>  	__kmem_cache_destroy(cachep);
>  	mutex_unlock(&cache_chain_mutex);
> diff --git a/mm/slob.c b/mm/slob.c
> index c78742d..9641da3 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -595,6 +595,8 @@ EXPORT_SYMBOL(kmem_cache_create);
>  void kmem_cache_destroy(struct kmem_cache *c)
>  {
>  	kmemleak_free(c);
> +	if (c->flags & SLAB_DESTROY_BY_RCU)
> +		rcu_barrier();
>  	slob_free(c, sizeof(struct kmem_cache));
>  }
>  EXPORT_SYMBOL(kmem_cache_destroy);
> diff --git a/mm/slub.c b/mm/slub.c
> index 819f056..a9201d8 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2595,6 +2595,8 @@ static inline int kmem_cache_close(struct kmem_cache *s)
>   */
>  void kmem_cache_destroy(struct kmem_cache *s)
>  {
> +	if (s->flags & SLAB_DESTROY_BY_RCU)
> +		rcu_barrier();
>  	down_write(&slub_lock);
>  	s->refcount--;
>  	if (!s->refcount) {

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Matt Mackall]

On Thu, 2009-06-25 at 12:31 -0700, Paul E. McKenney wrote:
> Hello!
> 
> Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> in RCU callbacks accessing a kmem_cache after it had been destroyed.
> 
> The following untested (might not even compile) patch proposes a fix.

Acked-by: Matt Mackall <mpm@selenic.com>

Nick, you'll want to make sure you get this in SLQB.

> Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
> 
>  slab.c |    2 +-
>  slob.c |    2 ++
>  slub.c |    2 ++
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index e74a16e..5241b65 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2547,7 +2547,7 @@ void kmem_cache_destroy(struct kmem_cache *cachep)
>  	}
>  
>  	if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
> -		synchronize_rcu();
> +		rcu_barrier();
>  
>  	__kmem_cache_destroy(cachep);
>  	mutex_unlock(&cache_chain_mutex);
> diff --git a/mm/slob.c b/mm/slob.c
> index c78742d..9641da3 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -595,6 +595,8 @@ EXPORT_SYMBOL(kmem_cache_create);
>  void kmem_cache_destroy(struct kmem_cache *c)
>  {
>  	kmemleak_free(c);
> +	if (c->flags & SLAB_DESTROY_BY_RCU)
> +		rcu_barrier();
>  	slob_free(c, sizeof(struct kmem_cache));
>  }
>  EXPORT_SYMBOL(kmem_cache_destroy);
> diff --git a/mm/slub.c b/mm/slub.c
> index 819f056..a9201d8 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2595,6 +2595,8 @@ static inline int kmem_cache_close(struct kmem_cache *s)
>   */
>  void kmem_cache_destroy(struct kmem_cache *s)
>  {
> +	if (s->flags & SLAB_DESTROY_BY_RCU)
> +		rcu_barrier();
>  	down_write(&slub_lock);
>  	s->refcount--;
>  	if (!s->refcount) {

-- 
http://selenic.com : development and support for Mercurial and Linux


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Paul E. McKenney]

On Thu, Jun 25, 2009 at 04:27:19PM -0500, Matt Mackall wrote:
> On Thu, 2009-06-25 at 12:31 -0700, Paul E. McKenney wrote:
> > Hello!
> > 
> > Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> > than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> > in RCU callbacks accessing a kmem_cache after it had been destroyed.
> > 
> > The following untested (might not even compile) patch proposes a fix.
> 
> Acked-by: Matt Mackall <mpm@selenic.com>
> 
> Nick, you'll want to make sure you get this in SLQB.
> 
> > Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>

And I seem to have blown Jesper's email address (apologies, Jesper!), so:

Reported-by: Jesper Dangaard Brouer <hawk@comx.dk>

> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> > 
> >  slab.c |    2 +-
> >  slob.c |    2 ++
> >  slub.c |    2 ++
> >  3 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/slab.c b/mm/slab.c
> > index e74a16e..5241b65 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -2547,7 +2547,7 @@ void kmem_cache_destroy(struct kmem_cache *cachep)
> >  	}
> >  
> >  	if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
> > -		synchronize_rcu();
> > +		rcu_barrier();
> >  
> >  	__kmem_cache_destroy(cachep);
> >  	mutex_unlock(&cache_chain_mutex);
> > diff --git a/mm/slob.c b/mm/slob.c
> > index c78742d..9641da3 100644
> > --- a/mm/slob.c
> > +++ b/mm/slob.c
> > @@ -595,6 +595,8 @@ EXPORT_SYMBOL(kmem_cache_create);
> >  void kmem_cache_destroy(struct kmem_cache *c)
> >  {
> >  	kmemleak_free(c);
> > +	if (c->flags & SLAB_DESTROY_BY_RCU)
> > +		rcu_barrier();
> >  	slob_free(c, sizeof(struct kmem_cache));
> >  }
> >  EXPORT_SYMBOL(kmem_cache_destroy);
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 819f056..a9201d8 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -2595,6 +2595,8 @@ static inline int kmem_cache_close(struct kmem_cache *s)
> >   */
> >  void kmem_cache_destroy(struct kmem_cache *s)
> >  {
> > +	if (s->flags & SLAB_DESTROY_BY_RCU)
> > +		rcu_barrier();
> >  	down_write(&slub_lock);
> >  	s->refcount--;
> >  	if (!s->refcount) {
> 
> -- 
> http://selenic.com : development and support for Mercurial and Linux
> 
> 

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Paul E. McKenney]

On Thu, Jun 25, 2009 at 04:27:19PM -0500, Matt Mackall wrote:
> On Thu, 2009-06-25 at 12:31 -0700, Paul E. McKenney wrote:
> > Hello!
> > 
> > Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> > than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> > in RCU callbacks accessing a kmem_cache after it had been destroyed.
> > 
> > The following untested (might not even compile) patch proposes a fix.
> 
> Acked-by: Matt Mackall <mpm@selenic.com>
> 
> Nick, you'll want to make sure you get this in SLQB.
> 
> > Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>

And I seem to have blown Jesper's email address (apologies, Jesper!), so:

Reported-by: Jesper Dangaard Brouer <hawk@comx.dk>

> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> > 
> >  slab.c |    2 +-
> >  slob.c |    2 ++
> >  slub.c |    2 ++
> >  3 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/slab.c b/mm/slab.c
> > index e74a16e..5241b65 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -2547,7 +2547,7 @@ void kmem_cache_destroy(struct kmem_cache *cachep)
> >  	}
> >  
> >  	if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
> > -		synchronize_rcu();
> > +		rcu_barrier();
> >  
> >  	__kmem_cache_destroy(cachep);
> >  	mutex_unlock(&cache_chain_mutex);
> > diff --git a/mm/slob.c b/mm/slob.c
> > index c78742d..9641da3 100644
> > --- a/mm/slob.c
> > +++ b/mm/slob.c
> > @@ -595,6 +595,8 @@ EXPORT_SYMBOL(kmem_cache_create);
> >  void kmem_cache_destroy(struct kmem_cache *c)
> >  {
> >  	kmemleak_free(c);
> > +	if (c->flags & SLAB_DESTROY_BY_RCU)
> > +		rcu_barrier();
> >  	slob_free(c, sizeof(struct kmem_cache));
> >  }
> >  EXPORT_SYMBOL(kmem_cache_destroy);
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 819f056..a9201d8 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -2595,6 +2595,8 @@ static inline int kmem_cache_close(struct kmem_cache *s)
> >   */
> >  void kmem_cache_destroy(struct kmem_cache *s)
> >  {
> > +	if (s->flags & SLAB_DESTROY_BY_RCU)
> > +		rcu_barrier();
> >  	down_write(&slub_lock);
> >  	s->refcount--;
> >  	if (!s->refcount) {
> 
> -- 
> http://selenic.com : development and support for Mercurial and Linux
> 
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Pekka Enberg]

On Thu, 2009-06-25 at 15:08 -0700, Paul E. McKenney wrote:
> On Thu, Jun 25, 2009 at 04:27:19PM -0500, Matt Mackall wrote:
> > On Thu, 2009-06-25 at 12:31 -0700, Paul E. McKenney wrote:
> > > Hello!
> > > 
> > > Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> > > than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> > > in RCU callbacks accessing a kmem_cache after it had been destroyed.
> > > 
> > > The following untested (might not even compile) patch proposes a fix.
> > 
> > Acked-by: Matt Mackall <mpm@selenic.com>
> > 
> > Nick, you'll want to make sure you get this in SLQB.
> > 
> > > Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>
> 
> And I seem to have blown Jesper's email address (apologies, Jesper!), so:
> 
> Reported-by: Jesper Dangaard Brouer <hawk@comx.dk>

Compiles and boots here so I went ahead and applied the patch. ;) Thanks
a lot Paul!

			Pekka

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Pekka Enberg]

On Thu, 2009-06-25 at 15:08 -0700, Paul E. McKenney wrote:
> On Thu, Jun 25, 2009 at 04:27:19PM -0500, Matt Mackall wrote:
> > On Thu, 2009-06-25 at 12:31 -0700, Paul E. McKenney wrote:
> > > Hello!
> > > 
> > > Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> > > than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> > > in RCU callbacks accessing a kmem_cache after it had been destroyed.
> > > 
> > > The following untested (might not even compile) patch proposes a fix.
> > 
> > Acked-by: Matt Mackall <mpm@selenic.com>
> > 
> > Nick, you'll want to make sure you get this in SLQB.
> > 
> > > Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>
> 
> And I seem to have blown Jesper's email address (apologies, Jesper!), so:
> 
> Reported-by: Jesper Dangaard Brouer <hawk@comx.dk>

Compiles and boots here so I went ahead and applied the patch. ;) Thanks
a lot Paul!

			Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Nick Piggin]

On Thu, Jun 25, 2009 at 04:27:19PM -0500, Matt Mackall wrote:
> On Thu, 2009-06-25 at 12:31 -0700, Paul E. McKenney wrote:
> > Hello!
> > 
> > Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> > than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> > in RCU callbacks accessing a kmem_cache after it had been destroyed.
> > 
> > The following untested (might not even compile) patch proposes a fix.
> 
> Acked-by: Matt Mackall <mpm@selenic.com>
> 
> Nick, you'll want to make sure you get this in SLQB.

Thanks Matt. Paul, I think this should be appropriate for
stable@kernel.org too?

> 
> > Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> > 
> >  slab.c |    2 +-
> >  slob.c |    2 ++
> >  slub.c |    2 ++
> >  3 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/slab.c b/mm/slab.c
> > index e74a16e..5241b65 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -2547,7 +2547,7 @@ void kmem_cache_destroy(struct kmem_cache *cachep)
> >  	}
> >  
> >  	if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
> > -		synchronize_rcu();
> > +		rcu_barrier();
> >  
> >  	__kmem_cache_destroy(cachep);
> >  	mutex_unlock(&cache_chain_mutex);
> > diff --git a/mm/slob.c b/mm/slob.c
> > index c78742d..9641da3 100644
> > --- a/mm/slob.c
> > +++ b/mm/slob.c
> > @@ -595,6 +595,8 @@ EXPORT_SYMBOL(kmem_cache_create);
> >  void kmem_cache_destroy(struct kmem_cache *c)
> >  {
> >  	kmemleak_free(c);
> > +	if (c->flags & SLAB_DESTROY_BY_RCU)
> > +		rcu_barrier();
> >  	slob_free(c, sizeof(struct kmem_cache));
> >  }
> >  EXPORT_SYMBOL(kmem_cache_destroy);
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 819f056..a9201d8 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -2595,6 +2595,8 @@ static inline int kmem_cache_close(struct kmem_cache *s)
> >   */
> >  void kmem_cache_destroy(struct kmem_cache *s)
> >  {
> > +	if (s->flags & SLAB_DESTROY_BY_RCU)
> > +		rcu_barrier();
> >  	down_write(&slub_lock);
> >  	s->refcount--;
> >  	if (!s->refcount) {
> 
> -- 
> http://selenic.com : development and support for Mercurial and Linux
> 

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Nick Piggin]

On Thu, Jun 25, 2009 at 04:27:19PM -0500, Matt Mackall wrote:
> On Thu, 2009-06-25 at 12:31 -0700, Paul E. McKenney wrote:
> > Hello!
> > 
> > Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> > than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> > in RCU callbacks accessing a kmem_cache after it had been destroyed.
> > 
> > The following untested (might not even compile) patch proposes a fix.
> 
> Acked-by: Matt Mackall <mpm@selenic.com>
> 
> Nick, you'll want to make sure you get this in SLQB.

Thanks Matt. Paul, I think this should be appropriate for
stable@kernel.org too?

> 
> > Reported-by: Jesper Dangaard Brouer <jdb@comx.dk>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> > 
> >  slab.c |    2 +-
> >  slob.c |    2 ++
> >  slub.c |    2 ++
> >  3 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/slab.c b/mm/slab.c
> > index e74a16e..5241b65 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -2547,7 +2547,7 @@ void kmem_cache_destroy(struct kmem_cache *cachep)
> >  	}
> >  
> >  	if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
> > -		synchronize_rcu();
> > +		rcu_barrier();
> >  
> >  	__kmem_cache_destroy(cachep);
> >  	mutex_unlock(&cache_chain_mutex);
> > diff --git a/mm/slob.c b/mm/slob.c
> > index c78742d..9641da3 100644
> > --- a/mm/slob.c
> > +++ b/mm/slob.c
> > @@ -595,6 +595,8 @@ EXPORT_SYMBOL(kmem_cache_create);
> >  void kmem_cache_destroy(struct kmem_cache *c)
> >  {
> >  	kmemleak_free(c);
> > +	if (c->flags & SLAB_DESTROY_BY_RCU)
> > +		rcu_barrier();
> >  	slob_free(c, sizeof(struct kmem_cache));
> >  }
> >  EXPORT_SYMBOL(kmem_cache_destroy);
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 819f056..a9201d8 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -2595,6 +2595,8 @@ static inline int kmem_cache_close(struct kmem_cache *s)
> >   */
> >  void kmem_cache_destroy(struct kmem_cache *s)
> >  {
> > +	if (s->flags & SLAB_DESTROY_BY_RCU)
> > +		rcu_barrier();
> >  	down_write(&slub_lock);
> >  	s->refcount--;
> >  	if (!s->refcount) {
> 
> -- 
> http://selenic.com : development and support for Mercurial and Linux
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Pekka Enberg]

On Fri, 2009-06-26 at 11:03 +0200, Nick Piggin wrote:
> On Thu, Jun 25, 2009 at 04:27:19PM -0500, Matt Mackall wrote:
> > On Thu, 2009-06-25 at 12:31 -0700, Paul E. McKenney wrote:
> > > Hello!
> > > 
> > > Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> > > than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> > > in RCU callbacks accessing a kmem_cache after it had been destroyed.
> > > 
> > > The following untested (might not even compile) patch proposes a fix.
> > 
> > Acked-by: Matt Mackall <mpm@selenic.com>
> > 
> > Nick, you'll want to make sure you get this in SLQB.
> 
> Thanks Matt. Paul, I think this should be appropriate for
> stable@kernel.org too?

Yup, I added cc to the patch. Thanks!

			Pekka

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Pekka Enberg]

On Fri, 2009-06-26 at 11:03 +0200, Nick Piggin wrote:
> On Thu, Jun 25, 2009 at 04:27:19PM -0500, Matt Mackall wrote:
> > On Thu, 2009-06-25 at 12:31 -0700, Paul E. McKenney wrote:
> > > Hello!
> > > 
> > > Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> > > than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> > > in RCU callbacks accessing a kmem_cache after it had been destroyed.
> > > 
> > > The following untested (might not even compile) patch proposes a fix.
> > 
> > Acked-by: Matt Mackall <mpm@selenic.com>
> > 
> > Nick, you'll want to make sure you get this in SLQB.
> 
> Thanks Matt. Paul, I think this should be appropriate for
> stable@kernel.org too?

Yup, I added cc to the patch. Thanks!

			Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Christoph Lameter]

On Thu, 25 Jun 2009, Paul E. McKenney wrote:

> Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> in RCU callbacks accessing a kmem_cache after it had been destroyed.
>
> The following untested (might not even compile) patch proposes a fix.

It could be seen to be the responsibility of the caller of
kmem_cache_destroy to insure that no accesses are pending.

If the caller specified destroy by rcu on cache creation then he also
needs to be aware of not destroying the cache itself until all rcu actions
are complete. This is similar to the caution that has to be execised then
accessing cache data itself.

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Christoph Lameter]

On Thu, 25 Jun 2009, Paul E. McKenney wrote:

> Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> in RCU callbacks accessing a kmem_cache after it had been destroyed.
>
> The following untested (might not even compile) patch proposes a fix.

It could be seen to be the responsibility of the caller of
kmem_cache_destroy to insure that no accesses are pending.

If the caller specified destroy by rcu on cache creation then he also
needs to be aware of not destroying the cache itself until all rcu actions
are complete. This is similar to the caution that has to be execised then
accessing cache data itself.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Matt Mackall]

On Mon, 2009-06-29 at 18:30 -0400, Christoph Lameter wrote:
> On Thu, 25 Jun 2009, Paul E. McKenney wrote:
> 
> > Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> > than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> > in RCU callbacks accessing a kmem_cache after it had been destroyed.
> >
> > The following untested (might not even compile) patch proposes a fix.
> 
> It could be seen to be the responsibility of the caller of
> kmem_cache_destroy to insure that no accesses are pending.
> 
> If the caller specified destroy by rcu on cache creation then he also
> needs to be aware of not destroying the cache itself until all rcu actions
> are complete. This is similar to the caution that has to be execised then
> accessing cache data itself.

This is a reasonable point, and in keeping with the design principle
'callers should handle their own special cases'. However, I think it
would be more than a little surprising for kmem_cache_free() to do the
right thing, but not kmem_cache_destroy().

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Matt Mackall]

On Mon, 2009-06-29 at 18:30 -0400, Christoph Lameter wrote:
> On Thu, 25 Jun 2009, Paul E. McKenney wrote:
> 
> > Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather
> > than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result
> > in RCU callbacks accessing a kmem_cache after it had been destroyed.
> >
> > The following untested (might not even compile) patch proposes a fix.
> 
> It could be seen to be the responsibility of the caller of
> kmem_cache_destroy to insure that no accesses are pending.
> 
> If the caller specified destroy by rcu on cache creation then he also
> needs to be aware of not destroying the cache itself until all rcu actions
> are complete. This is similar to the caution that has to be execised then
> accessing cache data itself.

This is a reasonable point, and in keeping with the design principle
'callers should handle their own special cases'. However, I think it
would be more than a little surprising for kmem_cache_free() to do the
right thing, but not kmem_cache_destroy().

-- 
http://selenic.com : development and support for Mercurial and Linux


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Christoph Lameter]

On Mon, 29 Jun 2009, Matt Mackall wrote:

> This is a reasonable point, and in keeping with the design principle
> 'callers should handle their own special cases'. However, I think it
> would be more than a little surprising for kmem_cache_free() to do the
> right thing, but not kmem_cache_destroy().

kmem_cache_free() must be used carefully when using SLAB_DESTROY_BY_RCU.
The freed object can be accessed after free until the rcu interval
expires (well sortof, it may even be reallocated within the interval).

There are special RCU considerations coming already with the use of
kmem_cache_free().

Adding RCU operations to the kmem_cache_destroy() logic may result in
unnecessary RCU actions for slabs where the coder is ensuring that the
RCU interval has passed by other means.

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Christoph Lameter]

On Mon, 29 Jun 2009, Matt Mackall wrote:

> This is a reasonable point, and in keeping with the design principle
> 'callers should handle their own special cases'. However, I think it
> would be more than a little surprising for kmem_cache_free() to do the
> right thing, but not kmem_cache_destroy().

kmem_cache_free() must be used carefully when using SLAB_DESTROY_BY_RCU.
The freed object can be accessed after free until the rcu interval
expires (well sortof, it may even be reallocated within the interval).

There are special RCU considerations coming already with the use of
kmem_cache_free().

Adding RCU operations to the kmem_cache_destroy() logic may result in
unnecessary RCU actions for slabs where the coder is ensuring that the
RCU interval has passed by other means.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Matt Mackall]

On Mon, 2009-06-29 at 19:19 -0400, Christoph Lameter wrote:
> On Mon, 29 Jun 2009, Matt Mackall wrote:
> 
> > This is a reasonable point, and in keeping with the design principle
> > 'callers should handle their own special cases'. However, I think it
> > would be more than a little surprising for kmem_cache_free() to do the
> > right thing, but not kmem_cache_destroy().
> 
> kmem_cache_free() must be used carefully when using SLAB_DESTROY_BY_RCU.
> The freed object can be accessed after free until the rcu interval
> expires (well sortof, it may even be reallocated within the interval).
> 
> There are special RCU considerations coming already with the use of
> kmem_cache_free().
> 
> Adding RCU operations to the kmem_cache_destroy() logic may result in
> unnecessary RCU actions for slabs where the coder is ensuring that the
> RCU interval has passed by other means.

Do we care? Cache destruction shouldn't be in anyone's fast path.
Correctness is more important and users are more liable to be correct
with this patch.

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Matt Mackall]

On Mon, 2009-06-29 at 19:19 -0400, Christoph Lameter wrote:
> On Mon, 29 Jun 2009, Matt Mackall wrote:
> 
> > This is a reasonable point, and in keeping with the design principle
> > 'callers should handle their own special cases'. However, I think it
> > would be more than a little surprising for kmem_cache_free() to do the
> > right thing, but not kmem_cache_destroy().
> 
> kmem_cache_free() must be used carefully when using SLAB_DESTROY_BY_RCU.
> The freed object can be accessed after free until the rcu interval
> expires (well sortof, it may even be reallocated within the interval).
> 
> There are special RCU considerations coming already with the use of
> kmem_cache_free().
> 
> Adding RCU operations to the kmem_cache_destroy() logic may result in
> unnecessary RCU actions for slabs where the coder is ensuring that the
> RCU interval has passed by other means.

Do we care? Cache destruction shouldn't be in anyone's fast path.
Correctness is more important and users are more liable to be correct
with this patch.

-- 
http://selenic.com : development and support for Mercurial and Linux


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Paul E. McKenney]

On Mon, Jun 29, 2009 at 07:06:34PM -0500, Matt Mackall wrote:
> On Mon, 2009-06-29 at 19:19 -0400, Christoph Lameter wrote:
> > On Mon, 29 Jun 2009, Matt Mackall wrote:
> > 
> > > This is a reasonable point, and in keeping with the design principle
> > > 'callers should handle their own special cases'. However, I think it
> > > would be more than a little surprising for kmem_cache_free() to do the
> > > right thing, but not kmem_cache_destroy().
> > 
> > kmem_cache_free() must be used carefully when using SLAB_DESTROY_BY_RCU.
> > The freed object can be accessed after free until the rcu interval
> > expires (well sortof, it may even be reallocated within the interval).
> > 
> > There are special RCU considerations coming already with the use of
> > kmem_cache_free().
> > 
> > Adding RCU operations to the kmem_cache_destroy() logic may result in
> > unnecessary RCU actions for slabs where the coder is ensuring that the
> > RCU interval has passed by other means.
> 
> Do we care? Cache destruction shouldn't be in anyone's fast path.
> Correctness is more important and users are more liable to be correct
> with this patch.

I am with Matt on this one -- if we are going to hand the users of
SLAB_DESTROY_BY_RCU a hand grenade, let's at least leave the pin in.

							Thanx, Paul

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Paul E. McKenney]

On Mon, Jun 29, 2009 at 07:06:34PM -0500, Matt Mackall wrote:
> On Mon, 2009-06-29 at 19:19 -0400, Christoph Lameter wrote:
> > On Mon, 29 Jun 2009, Matt Mackall wrote:
> > 
> > > This is a reasonable point, and in keeping with the design principle
> > > 'callers should handle their own special cases'. However, I think it
> > > would be more than a little surprising for kmem_cache_free() to do the
> > > right thing, but not kmem_cache_destroy().
> > 
> > kmem_cache_free() must be used carefully when using SLAB_DESTROY_BY_RCU.
> > The freed object can be accessed after free until the rcu interval
> > expires (well sortof, it may even be reallocated within the interval).
> > 
> > There are special RCU considerations coming already with the use of
> > kmem_cache_free().
> > 
> > Adding RCU operations to the kmem_cache_destroy() logic may result in
> > unnecessary RCU actions for slabs where the coder is ensuring that the
> > RCU interval has passed by other means.
> 
> Do we care? Cache destruction shouldn't be in anyone's fast path.
> Correctness is more important and users are more liable to be correct
> with this patch.

I am with Matt on this one -- if we are going to hand the users of
SLAB_DESTROY_BY_RCU a hand grenade, let's at least leave the pin in.

							Thanx, Paul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Pekka Enberg]

On Tue, Jun 30, 2009 at 9:00 AM, Paul E.
McKenney<paulmck@linux.vnet.ibm.com> wrote:
> On Mon, Jun 29, 2009 at 07:06:34PM -0500, Matt Mackall wrote:
>> On Mon, 2009-06-29 at 19:19 -0400, Christoph Lameter wrote:
>> > On Mon, 29 Jun 2009, Matt Mackall wrote:
>> >
>> > > This is a reasonable point, and in keeping with the design principle
>> > > 'callers should handle their own special cases'. However, I think it
>> > > would be more than a little surprising for kmem_cache_free() to do the
>> > > right thing, but not kmem_cache_destroy().
>> >
>> > kmem_cache_free() must be used carefully when using SLAB_DESTROY_BY_RCU.
>> > The freed object can be accessed after free until the rcu interval
>> > expires (well sortof, it may even be reallocated within the interval).
>> >
>> > There are special RCU considerations coming already with the use of
>> > kmem_cache_free().
>> >
>> > Adding RCU operations to the kmem_cache_destroy() logic may result in
>> > unnecessary RCU actions for slabs where the coder is ensuring that the
>> > RCU interval has passed by other means.
>>
>> Do we care? Cache destruction shouldn't be in anyone's fast path.
>> Correctness is more important and users are more liable to be correct
>> with this patch.
>
> I am with Matt on this one -- if we are going to hand the users of
> SLAB_DESTROY_BY_RCU a hand grenade, let's at least leave the pin in.

I don't even claim to understand all the RCU details here but I don't
see why we should care about _kmem_cache_destroy()_ performance at
this level. Christoph, hmmm?

                              Pekka

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Pekka Enberg]

On Tue, Jun 30, 2009 at 9:00 AM, Paul E.
McKenney<paulmck@linux.vnet.ibm.com> wrote:
> On Mon, Jun 29, 2009 at 07:06:34PM -0500, Matt Mackall wrote:
>> On Mon, 2009-06-29 at 19:19 -0400, Christoph Lameter wrote:
>> > On Mon, 29 Jun 2009, Matt Mackall wrote:
>> >
>> > > This is a reasonable point, and in keeping with the design principle
>> > > 'callers should handle their own special cases'. However, I think it
>> > > would be more than a little surprising for kmem_cache_free() to do the
>> > > right thing, but not kmem_cache_destroy().
>> >
>> > kmem_cache_free() must be used carefully when using SLAB_DESTROY_BY_RCU.
>> > The freed object can be accessed after free until the rcu interval
>> > expires (well sortof, it may even be reallocated within the interval).
>> >
>> > There are special RCU considerations coming already with the use of
>> > kmem_cache_free().
>> >
>> > Adding RCU operations to the kmem_cache_destroy() logic may result in
>> > unnecessary RCU actions for slabs where the coder is ensuring that the
>> > RCU interval has passed by other means.
>>
>> Do we care? Cache destruction shouldn't be in anyone's fast path.
>> Correctness is more important and users are more liable to be correct
>> with this patch.
>
> I am with Matt on this one -- if we are going to hand the users of
> SLAB_DESTROY_BY_RCU a hand grenade, let's at least leave the pin in.

I don't even claim to understand all the RCU details here but I don't
see why we should care about _kmem_cache_destroy()_ performance at
this level. Christoph, hmmm?

                              Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Christoph Lameter]

On Tue, 30 Jun 2009, Pekka Enberg wrote:

> I don't even claim to understand all the RCU details here but I don't
> see why we should care about _kmem_cache_destroy()_ performance at
> this level. Christoph, hmmm?

Well it was surprising to me that kmem_cache_destroy() would perform rcu
actions in the first place. RCU is usually handled externally and not
within the slab allocator. The only reason that SLAB_DESTROY_BY_RCU exists
is because the user cannot otherwise control the final release of memory
to the page allocator.

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Christoph Lameter]

On Tue, 30 Jun 2009, Pekka Enberg wrote:

> I don't even claim to understand all the RCU details here but I don't
> see why we should care about _kmem_cache_destroy()_ performance at
> this level. Christoph, hmmm?

Well it was surprising to me that kmem_cache_destroy() would perform rcu
actions in the first place. RCU is usually handled externally and not
within the slab allocator. The only reason that SLAB_DESTROY_BY_RCU exists
is because the user cannot otherwise control the final release of memory
to the page allocator.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Pekka Enberg]

Hi Christoph,

On Tue, 30 Jun 2009, Pekka Enberg wrote:
>> I don't even claim to understand all the RCU details here but I don't
>> see why we should care about _kmem_cache_destroy()_ performance at
>> this level. Christoph, hmmm?

On Tue, Jun 30, 2009 at 5:20 PM, Christoph
Lameter<cl@linux-foundation.org> wrote:
> Well it was surprising to me that kmem_cache_destroy() would perform rcu
> actions in the first place. RCU is usually handled externally and not
> within the slab allocator. The only reason that SLAB_DESTROY_BY_RCU exists
> is because the user cannot otherwise control the final release of memory
> to the page allocator.

Right. A quick grep for git logs reveals that it's been like that in
mm/slab.c at least since 2.6.12-rc2 so I think we should consider it
as part of the slab API and Paul's patch is an obvious bugfix to it.

                                           Pekka

Re: [PATCH RFC] fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

[Pekka Enberg]

Hi Christoph,

On Tue, 30 Jun 2009, Pekka Enberg wrote:
>> I don't even claim to understand all the RCU details here but I don't
>> see why we should care about _kmem_cache_destroy()_ performance at
>> this level. Christoph, hmmm?

On Tue, Jun 30, 2009 at 5:20 PM, Christoph
Lameter<cl@linux-foundation.org> wrote:
> Well it was surprising to me that kmem_cache_destroy() would perform rcu
> actions in the first place. RCU is usually handled externally and not
> within the slab allocator. The only reason that SLAB_DESTROY_BY_RCU exists
> is because the user cannot otherwise control the final release of memory
> to the page allocator.

Right. A quick grep for git logs reveals that it's been like that in
mm/slab.c at least since 2.6.12-rc2 so I think we should consider it
as part of the slab API and Paul's patch is an obvious bugfix to it.

                                           Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>