Re: [block IO crash] Re: 2.6.39-rc5-git2 boot crashs

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 4 May 2011, Tejun Heo wrote:
> Hello,
> 
> On Wed, May 04, 2011 at 03:00:37PM +0200, Thomas Gleixner wrote:
> > On Wed, 4 May 2011, Tejun Heo wrote:
> > > > > And that code runs with preemption enabled. So when the task gets
> > > > > preempted _BEFORE_ it has actuallty written back the data, then the
> > > > > race window is wide open.
> > > 
> > > Hmmm... if it's a race caused by preemtion enabled where it shouldn't
> > > be, it's most likely the wrong type of this_cpu_cmpxchg_double() being
> > > used in SLUB?  ie. __this_cpu_cmpxchg_double() where it should have
> > > been this_cpu_cmpxchg_double()?  Christoph?
> > 
> > No, the problem is that ELAN prevents the cmpxchg8b, but keeps
> > CONFIG_CMPXCHG_LOCAL=y which then results in the unprotected code for
> > the following reason:
> ...
> > So the question is whether CMPXCHG_LOCAL for x86 wants to depend on
> > X86_CMPXCHG64.
> > 
> > The other solution is to use irqsafe_cpu_cmpxchg_double() instead of
> > this_cpu_cmpxchg_double() in slub.c. 
> 
> I think this is the root cause.  CMPXCHG_LOCAL is an optimization
> flag, indicating that the processor provides fast local cmpxchg, it
> doesn't say anything about local synchronization properties and if the
> code required irq exclusion, it should have used
> irqsafe_cpu_cmpxchg_double() whether the processor supports it
> natively or not, so there's the bug.  Pekka, can you please change the
> offending cmpxchg_double() to irqsafe variant?

Patch below. Ingo, can you test that please ?

> As for CMPXCHG_LOCAL being set spuriously, maybe introduce
> CMPXCHG_DOUBLE_LOCAL?  I don't know.  It's pretty nasty to implement

Oh no, please not another CONFIG_WTF and more #ifdeffery.

> different high-level code paths depending on CPU features.  We can't
> even determine whether the feature will be actually available at
> compile time.  But, then again, it might incur noticeable slowdown for

Right, and the x86 implementation should not do 

#ifdef CONFIG_X86_CMPXCHG64
#define percpu_cmpxchg8b_double(pcp1, o1, o2, n1, n2)
#endif

And let the code fallback to the generic variant. It should have an
#else path using the the cmpxchg64_local() implementation which has
alternatives for runtime selection of cmpxchg8b or the cli protected
emulation.

> cases where the generic implementation is used.  Has anyone measured
> the difference against before the whole this_cpu conversion?

Yes, that really wants to be done. The whole CMPXCHG_LOCAL ifdeffery
should have been avoided in the first place. this_cpu_cmpxchg can
really be implemented with preempt_enable/disable and the irqsafe
variant in any case.

Thanks,

	tglx

--------->
Subject: slub-hmm.patch
From: Thomas Gleixner <tglx@linutronix.de>
Date: Wed, 04 May 2011 15:38:19 +0200

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/percpu.h |    2 +-
 mm/slub.c              |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6/include/linux/percpu.h
===================================================================
--- linux-2.6.orig/include/linux/percpu.h
+++ linux-2.6/include/linux/percpu.h
@@ -948,7 +948,7 @@ do {									\
 	irqsafe_generic_cpu_cmpxchg_double(pcp1, pcp2, oval1, oval2, nval1, nval2)
 # endif
 # define irqsafe_cpu_cmpxchg_double(pcp1, pcp2, oval1, oval2, nval1, nval2)	\
-	__pcpu_double_call_return_int(irqsafe_cpu_cmpxchg_double_, (pcp1), (pcp2), (oval1), (oval2), (nval1), (nval2))
+	__pcpu_double_call_return_bool(irqsafe_cpu_cmpxchg_double_, (pcp1), (pcp2), (oval1), (oval2), (nval1), (nval2))
 #endif
 
 #endif /* __LINUX_PERCPU_H */
Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c
+++ linux-2.6/mm/slub.c
@@ -1940,7 +1940,7 @@ redo:
 		 * Since this is without lock semantics the protection is only against
 		 * code executing on this cpu *not* from access by other cpus.
 		 */
-		if (unlikely(!this_cpu_cmpxchg_double(
+		if (unlikely(!irqsafe_cpu_cmpxchg_double(
 				s->cpu_slab->freelist, s->cpu_slab->tid,
 				object, tid,
 				get_freepointer(s, object), next_tid(tid)))) {
@@ -2145,7 +2145,7 @@ redo:
 		set_freepointer(s, object, c->freelist);
 
 #ifdef CONFIG_CMPXCHG_LOCAL
-		if (unlikely(!this_cpu_cmpxchg_double(
+		if (unlikely(!irqsafe_cpu_cmpxchg_double(
 				s->cpu_slab->freelist, s->cpu_slab->tid,
 				c->freelist, tid,
 				object, next_tid(tid)))) {