On Fri, Mar 13, 2015 at 04:29:23PM +0000, Christoph Lameter wrote:
> On Fri, 13 Mar 2015, Mark Rutland wrote:
>
> > */
> > - do {
> > - tid = this_cpu_read(s->cpu_slab->tid);
> > - c = raw_cpu_ptr(s->cpu_slab);
> > - } while (IS_ENABLED(CONFIG_PREEMPT) && unlikely(tid != c->tid));
> > + c = raw_cpu_ptr(s->cpu_slab);
> > + tid = READ_ONCE(c->tid);
> >
>
> Ok that works for the !SMP case. What about SMP and PREEMPT now?
>From testing on boards I have access to, things seem fine so far with
SMP && PREEMPT. If we have any allocator stress tests I'm more than
happy to give them a go.
As I mentioned, it's not clear to me that the the READ_ONCE(c->tid) is
safe (i.e. it is atomic and non-destructive). If READ_ONCE(c->tid) is
not safe then the code added in 9aabf810a67cd97e is similarly broken
given the access in the loop condition, in addition to the hoisting done
by the compiler.
> And yes code like this was deemed safe for years and the race condition is
> very subtle and difficult to trigger (also given that PREEMPT is rarely
> used these days).
The this_cpu_cmpxchg_double is the saving grace here: if c->tid is read
from a different CPU it will fail and we'll retry the whole thing.
That's exactly what the original patch relied on in the case a
preemption occured after the loop.
w.r.t. CONFIG_PREEMPT, git grep tells me otherwise:
[mark@leverpostej:~/src/linux]% git grep -w 'CONFIG_PREEMPT' --
arch/*/configs/* | wc -l
109
[mark@leverpostej:~/src/linux]% git grep -w 'CONFIG_PREEMPT is not set' --
arch/*/configs/* | wc -l
2
[mark@leverpostej:~/src/linux]% git grep -w 'CONFIG_PREEMPT=y' --
arch/*/configs/* | wc -l
107
[mark@leverpostej:~/src/linux]% git grep -w 'CONFIG_PREEMPT=n' --
arch/*/configs/* | wc -l
0
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
