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 <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
---
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) {
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 <[email protected]>
Nick, you'll want to make sure you get this in SLQB.
> Reported-by: Jesper Dangaard Brouer <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
> ---
>
> 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
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 <[email protected]>
>
> Nick, you'll want to make sure you get this in SLQB.
>
> > Reported-by: Jesper Dangaard Brouer <[email protected]>
And I seem to have blown Jesper's email address (apologies, Jesper!), so:
Reported-by: Jesper Dangaard Brouer <[email protected]>
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > ---
> >
> > 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
>
>
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 <[email protected]>
> >
> > Nick, you'll want to make sure you get this in SLQB.
> >
> > > Reported-by: Jesper Dangaard Brouer <[email protected]>
>
> And I seem to have blown Jesper's email address (apologies, Jesper!), so:
>
> Reported-by: Jesper Dangaard Brouer <[email protected]>
Compiles and boots here so I went ahead and applied the patch. ;) Thanks
a lot Paul!
Pekka
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 <[email protected]>
>
> Nick, you'll want to make sure you get this in SLQB.
Thanks Matt. Paul, I think this should be appropriate for
[email protected] too?
>
> > Reported-by: Jesper Dangaard Brouer <[email protected]>
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > ---
> >
> > 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
>
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 <[email protected]>
> >
> > Nick, you'll want to make sure you get this in SLQB.
>
> Thanks Matt. Paul, I think this should be appropriate for
> [email protected] too?
Yup, I added cc to the patch. Thanks!
Pekka
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.
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
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.
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
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
On Tue, Jun 30, 2009 at 9:00 AM, Paul E.
McKenney<[email protected]> 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
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.
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<[email protected]> 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