lkml.org 
[lkml]   [2011]   [May]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [block IO crash] Re: 2.6.39-rc5-git2 boot crashs
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)))) {

\
 
 \ /
  Last update: 2011-05-04 16:13    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site