From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1030357AbbCMSSB (ORCPT <rfc822;w@1wt.eu>);
	Fri, 13 Mar 2015 14:18:01 -0400
Received: from foss.arm.com ([217.140.101.70]:52738 "EHLO foss.arm.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1756061AbbCMSR1 (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Fri, 13 Mar 2015 14:17:27 -0400
Date: Fri, 13 Mar 2015 18:16:50 +0000
From: Mark Rutland <mark.rutland@arm.com>
To: Christoph Lameter <cl@linux.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Catalin Marinas <Catalin.Marinas@arm.com>,
        David Rientjes <rientjes@google.com>,
        Jesper Dangaard Brouer <brouer@redhat.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Pekka Enberg <penberg@kernel.org>,
        Steve Capper <steve.capper@linaro.org>
Subject: Re: [PATCH] mm/slub: fix lockups on PREEMPT && !SMP kernels
Message-ID: <20150313181650.GA11058@leverpostej>
References: <1426261632-8911-1-git-send-email-mark.rutland@arm.com>
 <alpine.DEB.2.11.1503131126550.13899@gentwo.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.11.1503131126550.13899@gentwo.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.