From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753646AbcEPNR4 (ORCPT ); Mon, 16 May 2016 09:17:56 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:60354 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752846AbcEPNRz (ORCPT ); Mon, 16 May 2016 09:17:55 -0400 Date: Mon, 16 May 2016 15:17:51 +0200 From: Peter Zijlstra To: Thomas Gleixner Cc: Vikram Mulukutla , linux-kernel@vger.kernel.org, Ingo Molnar Subject: Re: Additional compiler barrier required in sched_preempt_enable_no_resched? Message-ID: <20160516131751.GH3205@twins.programming.kicks-ass.net> References: <573576B3.50908@codeaurora.org> <20160513145814.GS3192@twins.programming.kicks-ass.net> <573658D1.2080601@codeaurora.org> <20160516105557.GL3193@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160516105557.GL3193@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 16, 2016 at 12:55:57PM +0200, Peter Zijlstra wrote: > > So the barriers already forbid that the compiler reorders code. How on earth > > is the compiler supposed to optimize the dec/inc out? > > Order things like: > > > #define sched_preempt_enable_no_resched() \ > > do { \ > > barrier(); \ > > preempt_count_dec(); \ > > } while (0) > > > #define preempt_disable() \ > > do { \ > > preempt_count_inc(); \ > > barrier(); \ > > } while (0) > > And there is no barrier between the dec and inc, and a smarty pants > compiler could just decide to forgo the update, since in program order > there is no observable difference either way. > > Making the thing volatile tells the compiler there can be external > observations of the memory and it cannot assume things like that and > must emit the operations. ... > So I'll go write a proper changelog for the volatile thing and get it > merged with a Cc to stable. --- Subject: sched,preempt: Fix preempt_count manipulations From: Peter Zijlstra Date: Mon May 16 15:01:11 CEST 2016 Vikram reported that his ARM64 compiler managed to 'optimize' away the preempt_count manipulations in code like: preempt_enable_no_resched(); put_user(); preempt_disable(); Irrespective of that fact that that is horrible code that should be fixed for many reasons, it does highlight a deficiency in the generic preempt_count manipulators. As it is never right to combine/elide preempt_count manipulations like this. Therefore sprinkle some volatile in the two generic accessors to ensure the compiler is aware of the fact that the preempt_count is observed outside of the regular program-order view and thus cannot be optimized away like this. x86; the only arch not using the generic code is not affected as we do all this in asm in order to use the segment base per-cpu stuff. Cc: stable@vger.kernel.org Cc: Thomas Gleixner Fixes: a787870924db ("sched, arch: Create asm/preempt.h") Reported-by: Vikram Mulukutla Tested-by: Vikram Mulukutla Signed-off-by: Peter Zijlstra (Intel) --- include/asm-generic/preempt.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- a/include/asm-generic/preempt.h +++ b/include/asm-generic/preempt.h @@ -7,10 +7,10 @@ static __always_inline int preempt_count(void) { - return current_thread_info()->preempt_count; + return READ_ONCE(current_thread_info()->preempt_count); } -static __always_inline int *preempt_count_ptr(void) +static __always_inline volatile int *preempt_count_ptr(void) { return ¤t_thread_info()->preempt_count; }