Patchwork slub: fix slab_pad_check()

login
register
mail settings
Submitter Eric Dumazet
Date Sept. 3, 2009, 2:08 p.m.
Message ID <4A9FCDC6.3060003@gmail.com>
Download mbox | patch
Permalink /patch/32889/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - Sept. 3, 2009, 2:08 p.m.
Christoph Lameter a écrit :
> On Thu, 3 Sep 2009, Eric Dumazet wrote:
> 
>> on a SLAB_DESTROY_BY_RCU cache, there is no need to try to optimize this
>> rcu_barrier() call, unless we want superfast reboot/halt sequences...
> 
> I stilll think that the action to quiesce rcu is something that the caller
> of kmem_cache_destroy must take care of.

Do you mean :

if (kmem_cache_shrink(s) == 0) {
	rcu_barrier();
	kmem_cache_destroy_no_rcu_barrier(s);
} else {
	kmem_cache_destroy_with_rcu_barrier_because_SLAB_DESTROY_BY_RCU_cache(s);
}

What would be the point ?



> 
> Could you split this into two patches: One that addresses the poison and
> another that deals with rcu?
> 

Sure, here is the poison thing

[PATCH] slub: fix slab_pad_check()

When SLAB_POISON is used and slab_pad_check() finds an overwrite of the
slab padding, we call restore_bytes() on the whole slab, not only
on the padding.

Reported-by: Zdenek Kabelac <zdenek.kabelac@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
--
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
Paul E. McKenney - Sept. 3, 2009, 3:01 p.m.
On Thu, Sep 03, 2009 at 01:38:50PM -0500, Christoph Lameter wrote:
> On Thu, 3 Sep 2009, Eric Dumazet wrote:
> 
> > Christoph Lameter a ?crit :
> > > On Thu, 3 Sep 2009, Eric Dumazet wrote:
> > >
> > >> on a SLAB_DESTROY_BY_RCU cache, there is no need to try to optimize this
> > >> rcu_barrier() call, unless we want superfast reboot/halt sequences...
> > >
> > > I stilll think that the action to quiesce rcu is something that the caller
> > > of kmem_cache_destroy must take care of.
> >
> > Do you mean :
> >
> > if (kmem_cache_shrink(s) == 0) {
> > 	rcu_barrier();
> > 	kmem_cache_destroy_no_rcu_barrier(s);
> > } else {
> > 	kmem_cache_destroy_with_rcu_barrier_because_SLAB_DESTROY_BY_RCU_cache(s);
> > }
> >
> > What would be the point ?
> 
> The above is port of slub?
> 
> I mean that (in this case) the net subsystem would have to deal with RCU quietness
> before disposing of the slab cache. There may be multiple ways of dealing
> with RCU. The RCU barrier may be unnecessary for future uses. Typically
> one would expect that all deferred handling of structures must be complete
> for correctness before disposing of the whole cache.

Which is precisely the point of the rcu_barrier(), right?

							Thanx, Paul

> > [PATCH] slub: fix slab_pad_check()
> 
> Acked-by: Christoph Lameter <cl@linux-foundation.org>
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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
Eric Dumazet - Sept. 3, 2009, 3:02 p.m.
Christoph Lameter a écrit :
> On Thu, 3 Sep 2009, Eric Dumazet wrote:
> 
>> Christoph Lameter a ?crit :
>>> On Thu, 3 Sep 2009, Eric Dumazet wrote:
>>>
>>>> on a SLAB_DESTROY_BY_RCU cache, there is no need to try to optimize this
>>>> rcu_barrier() call, unless we want superfast reboot/halt sequences...
>>> I stilll think that the action to quiesce rcu is something that the caller
>>> of kmem_cache_destroy must take care of.
>> Do you mean :
>>
>> if (kmem_cache_shrink(s) == 0) {
>> 	rcu_barrier();
>> 	kmem_cache_destroy_no_rcu_barrier(s);
>> } else {
>> 	kmem_cache_destroy_with_rcu_barrier_because_SLAB_DESTROY_BY_RCU_cache(s);
>> }
>>
>> What would be the point ?
> 
> The above is port of slub?

No, I am trying to code what you suggest, and I could not find a clean way with
current API (SLAB/SLUB/SLOB)

> 
> I mean that (in this case) the net subsystem would have to deal with RCU quietness
> before disposing of the slab cache. There may be multiple ways of dealing
> with RCU. The RCU barrier may be unnecessary for future uses. Typically
> one would expect that all deferred handling of structures must be complete
> for correctness before disposing of the whole cache.

Point is we cannot deal with RCU quietness before disposing the slab cache,
(if SLAB_DESTROY_BY_RCU was set on the cache) since this disposing *will* 
make call_rcu() calls when a full slab is freed/purged.
And when RCU grace period is elapsed, the callback *will* need access to 
the cache we want to dismantle. Better to not have kfreed()/poisoned it...

I believe you mix two RCU uses here.

1) The one we all know, is use normal caches (!SLAB_DESTROY_BY_RCU) 
(or kmalloc()), and use call_rcu(... kfree_something)

   In this case, you are 100% right that the subsystem itself has
   to call rcu_barrier() (or respect whatever self-synchro) itself,
   before calling kmem_cache_destroy()

2) The SLAB_DESTROY_BY_RCU one.

   Part of cache dismantle needs to call rcu_barrier() itself.
   Caller doesnt have to use rcu_barrier(). It would be a waste of time,
   as kmem_cache_destroy() will refill rcu wait queues with its own stuff.





--
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
Christoph Lameter - Sept. 3, 2009, 6:38 p.m.
On Thu, 3 Sep 2009, Eric Dumazet wrote:

> Christoph Lameter a ?crit :
> > On Thu, 3 Sep 2009, Eric Dumazet wrote:
> >
> >> on a SLAB_DESTROY_BY_RCU cache, there is no need to try to optimize this
> >> rcu_barrier() call, unless we want superfast reboot/halt sequences...
> >
> > I stilll think that the action to quiesce rcu is something that the caller
> > of kmem_cache_destroy must take care of.
>
> Do you mean :
>
> if (kmem_cache_shrink(s) == 0) {
> 	rcu_barrier();
> 	kmem_cache_destroy_no_rcu_barrier(s);
> } else {
> 	kmem_cache_destroy_with_rcu_barrier_because_SLAB_DESTROY_BY_RCU_cache(s);
> }
>
> What would be the point ?

The above is port of slub?

I mean that (in this case) the net subsystem would have to deal with RCU quietness
before disposing of the slab cache. There may be multiple ways of dealing
with RCU. The RCU barrier may be unnecessary for future uses. Typically
one would expect that all deferred handling of structures must be complete
for correctness before disposing of the whole cache.

> [PATCH] slub: fix slab_pad_check()

Acked-by: Christoph Lameter <cl@linux-foundation.org>
--
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
Pekka Enberg - Sept. 3, 2009, 7:34 p.m.
On Thu, Sep 3, 2009 at 5:08 PM, Eric Dumazet<eric.dumazet@gmail.com> wrote:
> Sure, here is the poison thing
>
> [PATCH] slub: fix slab_pad_check()
>
> When SLAB_POISON is used and slab_pad_check() finds an overwrite of the
> slab padding, we call restore_bytes() on the whole slab, not only
> on the padding.
>
> Reported-by: Zdenek Kabelac <zdenek.kabelac@gmail.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks!
--
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

Patch

diff --git a/mm/slub.c b/mm/slub.c
index b9f1491..0ac839f 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -646,7 +646,7 @@  static int slab_pad_check(struct kmem_cache *s, struct page *page)
 	slab_err(s, page, "Padding overwritten. 0x%p-0x%p", fault, end - 1);
 	print_section("Padding", end - remainder, remainder);
 
-	restore_bytes(s, "slab padding", POISON_INUSE, start, end);
+	restore_bytes(s, "slab padding", POISON_INUSE, end - remainder, end);
 	return 0;
 }