[PATCH -mm] slub: update cpu after new_slab()

[PATCH -mm] slub: update cpu after new_slab()

[Hugh Dickins]

SLUB gave me a NULL pointer dereference in slab_alloc(), in the
slab_lock(page) of its "Current cpuslab is acceptable" block: cpu
1 had been looking at cpu_slab[2], which then went NULL beneath it.
Since new_slab() may reenable interrupts and sleep (when __GFP_WAIT),
we may get rescheduled on a different cpu: so need to reevaluate it.

Signed-off-by: Hugh Dickins <hugh@veritas.com>

--- 2.6.21-rc7-mm1/mm/slub.c	2007-04-24 20:26:48.000000000 +0100
+++ linux/mm/slub.c	2007-04-25 15:49:12.000000000 +0100
@@ -1234,11 +1234,12 @@ have_slab:
 
 	page = new_slab(s, gfpflags, node);
 	if (page) {
+		cpu = smp_processor_id();
 		if (s->cpu_slab[cpu]) {
 			/*
-			 * Someone else populated the cpu_slab while
-			 * we enabled interrupts. The page may not
-			 * be on the requested node.
+			 * Someone else populated the cpu_slab while we enabled
+			 * interrupts, or we have got scheduled on another cpu.
+			 * The page may not be on the requested node.
 			 */
 			if (node == -1 ||
 				page_to_nid(s->cpu_slab[cpu]) == node) {

Re: [PATCH -mm] slub: update cpu after new_slab()

[Christoph Lameter]

On Wed, 25 Apr 2007, Hugh Dickins wrote:

> SLUB gave me a NULL pointer dereference in slab_alloc(), in the
> slab_lock(page) of its "Current cpuslab is acceptable" block: cpu
> 1 had been looking at cpu_slab[2], which then went NULL beneath it.
> Since new_slab() may reenable interrupts and sleep (when __GFP_WAIT),
> we may get rescheduled on a different cpu: so need to reevaluate it.

Right. local_irq_save does not switch off preemption as I thought.

Acked-by: Christoph Lameter <clameter@sgi.com>

Re: [PATCH -mm] slub: update cpu after new_slab()

[Hugh Dickins]

On Wed, 25 Apr 2007, Christoph Lameter wrote:
> On Wed, 25 Apr 2007, Hugh Dickins wrote:
> 
> > SLUB gave me a NULL pointer dereference in slab_alloc(), in the
> > slab_lock(page) of its "Current cpuslab is acceptable" block: cpu
> > 1 had been looking at cpu_slab[2], which then went NULL beneath it.
> > Since new_slab() may reenable interrupts and sleep (when __GFP_WAIT),
> > we may get rescheduled on a different cpu: so need to reevaluate it.
> 
> Right. local_irq_save does not switch off preemption as I thought.

Strange comment.  Preemption is not possible while IRQs are disabled,
but new_slab() rightly reenables them within itself in the __GFP_WAIT
case, since it's going off to do a page allocation and may need to wait.

(And actually, this kernel was a CONFIG_PREEMPT_NONE=y one.)

> 
> Acked-by: Christoph Lameter <clameter@sgi.com>

Thanks,
Hugh

Re: [PATCH -mm] slub: update cpu after new_slab()

[Christoph Lameter]

On Wed, 25 Apr 2007, Hugh Dickins wrote:

> > Right. local_irq_save does not switch off preemption as I thought.
> 
> Strange comment.  Preemption is not possible while IRQs are disabled,
> but new_slab() rightly reenables them within itself in the __GFP_WAIT
> case, since it's going off to do a page allocation and may need to wait.

Yes I expected local_irq_save to increase the preempt count and then 
local_irq_enable to simply enable interrupts without affecting the preempt 
count. Thus the process should stay on the same processor.

Never thought it would be possible to move to a different processor in mid 
flight.

Re: [PATCH -mm] slub: update cpu after new_slab()

[Hugh Dickins]

On Wed, 25 Apr 2007, Christoph Lameter wrote:
> On Wed, 25 Apr 2007, Hugh Dickins wrote:
> 
> > > Right. local_irq_save does not switch off preemption as I thought.
> > 
> > Strange comment.  Preemption is not possible while IRQs are disabled,
> > but new_slab() rightly reenables them within itself in the __GFP_WAIT
> > case, since it's going off to do a page allocation and may need to wait.
> 
> Yes I expected local_irq_save to increase the preempt count and then 
> local_irq_enable to simply enable interrupts without affecting the preempt 
> count. Thus the process should stay on the same processor.
> 
> Never thought it would be possible to move to a different processor in mid 
> flight.

But, surely you wouldn't have expected it to stay on the processor
throughout the waiting page allocation??  I think you're misremembering
your expectations, and this was just a simple, understandable, oversight.

Quite a serious one, though: it got caught in my case by the NULL
dereference, but it's probably been switching cpu there much more
often - one cpu diddling with what's private to another, with
unpredictable results.

Hugh

Re: [PATCH -mm] slub: update cpu after new_slab()

[Christoph Lameter]

On Wed, 25 Apr 2007, Hugh Dickins wrote:

> But, surely you wouldn't have expected it to stay on the processor
> throughout the waiting page allocation??  I think you're misremembering
> your expectations, and this was just a simple, understandable, oversight.

You do not know my fuzzy brain... Some thoughts go wrong once and 
twice and then are stuck in there and I need people like you to 
straighten me out.

> Quite a serious one, though: it got caught in my case by the NULL
> dereference, but it's probably been switching cpu there much more
> often - one cpu diddling with what's private to another, with
> unpredictable results.

Right. Thanks for catching it.