Message-ID: <20131119160502.GG11778@mudshark.cambridge.arm.com>
Date:	Tue, 19 Nov 2013 16:05:02 +0000
From:	Will Deacon <will.deacon@....com>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Catalin Marinas <Catalin.Marinas@....com>,
	Peter Zijlstra <peterz@...radead.org>,
	"lttng-dev@...ts.lttng.org" <lttng-dev@...ts.lttng.org>,
	Nathan Lynch <Nathan_Lynch@...tor.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: current_thread_info() not respecting program order with gcc 4.8.x

On Tue, Nov 19, 2013 at 03:29:12PM +0000, Mathieu Desnoyers wrote:
> Hi,

Hi Mathieu,

> I got a bug report on ARM which appears to be caused by an aggressive gcc optimisation starting from gcc 4.8.x due to lack of constraints on the current_thread_info() inline assembly. The only logical explanation for his issue I see so far is that read of the preempt_count within might_sleep() is reordered with preempt_enable() or preempt_disable(). AFAIU, this kind of issue also applies to other architectures.
> 
> First thing, preempt enable/disable only contains barrier() between the inc/dec and the inside of the critical section, not the outside. Therefore, we are relying on program order to ensure that the preempt_count() read in might_sleep() is not reordered with the preempt count inc/dec.

This sounds almost identical to an issue I experienced with our optimised
per-cpu code (more below).

> However, looking at ARM arch/arm/include/asm/thread_info.h:
> 
> static inline struct thread_info *current_thread_info(void)
> {
>         register unsigned long sp asm ("sp");
>         return (struct thread_info *)(sp & ~(THREAD_SIZE - 1));
> }
> 
> The inline assembly has no clobber and is not volatile. (this is also true for all other architectures I've looked at so far, which includes x86 and powerpc)
> 
> As long as someone does:
> 
> struct thread_info *myinfo = current_thread_info();
> 
> load from myinfo->preempt_count;
> store to myinfo->preempt_count;
> 
> The program order should be preserved, because the read and the write are done wrt same base. However, what we have here in the case of might_sleep() followed by preempt_enable() is:
> 
> load from current_thread_info()->preempt_count;
> store to current_thread_info()->preempt_count;
> 
> Since each current_thread_info() is a different asm ("sp") without clobber nor volatile, AFAIU, the compiler is within its right to reorder them.
> 
> One possible solution to this might be to add "memory" clobber and volatile to this inline asm, but I fear it would put way too much constraints on the compiler optimizations (too heavyweight).

Yup, that sucks, because you end up unable to cache the value when you know
it hasn't changed.

> Have any of you experienced this issue ? Any thoughts on the matter ?

The way I got around this for the per-cpu code is to include a dummy memory
constraint for the stack. This has a couple of advantages:

	(1) It hazards against a "memory" clobber, so doesn't require use of
	    `volatile'

	(2) It doesn't require GCC to emit any address generation code,
	    since dereferencing the sp is valid in ARM assembly

so adding something like:

	asm("" :: "Q" (*sp));

immediately after the declaration of sp in current_therad_info might do the
trick. Do you have a way to test that?

Cheers,

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists