From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752321AbdGaOX1 (ORCPT ); Mon, 31 Jul 2017 10:23:27 -0400 Received: from mail.kernel.org ([198.145.29.99]:55714 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750987AbdGaOX0 (ORCPT ); Mon, 31 Jul 2017 10:23:26 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C5EC322B5D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=goodmis.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=rostedt@goodmis.org Date: Mon, 31 Jul 2017 10:23:23 -0400 From: Steven Rostedt To: Peter Zijlstra Cc: jkosina@suse.cz, masami.hiramatsu.pt@hitachi.com, hpa@linux.intel.com, linux-kernel@vger.kernel.org, x86@kernel.org Subject: Re: [RFC][PATCH]: x86: clarify/fix no-op barriers for text_poke_bp() Message-ID: <20170731102323.096376f5@gandalf.local.home> In-Reply-To: <20170731102154.f57cvkjtnbmtctk6@hirez.programming.kicks-ass.net> References: <20170731102154.f57cvkjtnbmtctk6@hirez.programming.kicks-ass.net> X-Mailer: Claws Mail 3.14.0 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 31 Jul 2017 12:21:54 +0200 Peter Zijlstra wrote: > So I was looking at text_poke_bp() today and I couldn't make sense of > the barriers there. > > How's for something like so? > > --- > arch/x86/kernel/alternative.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > index 32e14d137416..3344d3382e91 100644 > --- a/arch/x86/kernel/alternative.c > +++ b/arch/x86/kernel/alternative.c > @@ -742,7 +742,16 @@ static void *bp_int3_handler, *bp_int3_addr; > > int poke_int3_handler(struct pt_regs *regs) > { > - /* bp_patching_in_progress */ > + /* > + * Having observed our INT3 instruction, we now must observe > + * bp_patching_in_progress. > + * > + * in_progress = TRUE INT3 > + * WMB RMB > + * write INT3 if (in_progress) > + * > + * Idem for bp_int3_handler. > + */ Looks correct. > smp_rmb(); > > if (likely(!bp_patching_in_progress)) > @@ -788,9 +797,8 @@ void *text_poke_bp(void *addr, const void *opcode, size_t len, void *handler) > bp_int3_addr = (u8 *)addr + sizeof(int3); > bp_patching_in_progress = true; > /* > - * Corresponding read barrier in int3 notifier for > - * making sure the in_progress flags is correctly ordered wrt. > - * patching > + * Corresponding read barrier in int3 notifier for making sure the > + * in_progress and handler are correctly ordered wrt. patching. > */ This looks correct as well. > smp_wmb(); > > @@ -815,9 +823,11 @@ void *text_poke_bp(void *addr, const void *opcode, size_t len, void *handler) > text_poke(addr, opcode, sizeof(int3)); > > on_each_cpu(do_sync_core, NULL, 1); > - > + /* > + * sync_core() implies an smp_mb() and orders this store against > + * the writing of the new instruction. > + */ Yep. > bp_patching_in_progress = false; > - smp_wmb(); Heh, I think this was a "lets not leak bp_patching_in_progress" out of this function. But I don't see any harm if it happens. As this function was a *very* slow path, that smp_wmb() was a "it's not really needed, but it wont hurt anything to slap it in there just in case". Reviewed-by: Steven Rostedt (VMware) -- Steve > > return addr; > }