From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750903AbeDCNsl (ORCPT ); Tue, 3 Apr 2018 09:48:41 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:33050 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750726AbeDCNsj (ORCPT ); Tue, 3 Apr 2018 09:48:39 -0400 Date: Tue, 3 Apr 2018 14:48:32 +0100 From: Mark Rutland To: Yury Norov Cc: Will Deacon , "Paul E. McKenney" , Chris Metcalf , Christopher Lameter , Russell King - ARM Linux , Steven Rostedt , Mathieu Desnoyers , Catalin Marinas , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , linux-arm-kernel@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, kvm-ppc@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] smp: introduce kick_active_cpus_sync() Message-ID: <20180403134832.2cdae64uwuot6ryz@lakrids.cambridge.arm.com> References: <20180325175004.28162-1-ynorov@caviumnetworks.com> <20180325175004.28162-3-ynorov@caviumnetworks.com> <20180327102116.GA2464@arm.com> <20180401111108.mudkiewzn33sifvk@yury-thinkpad> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180401111108.mudkiewzn33sifvk@yury-thinkpad> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Yury, On Sun, Apr 01, 2018 at 02:11:08PM +0300, Yury Norov wrote: > +/* > + * Flush I-cache if CPU is in extended quiescent state > + */ This comment is misleading. An ISB doesn't touch the I-cache; it forces a context synchronization event. > + .macro isb_if_eqs > +#ifndef CONFIG_TINY_RCU > + bl rcu_is_watching > + tst w0, #0xff > + b.ne 1f The TST+B.NE can be a CBNZ: bl rcu_is_watching cbnz x0, 1f isb 1: > + /* Pairs with aarch64_insn_patch_text for EQS CPUs. */ > + isb > +1: > +#endif > + .endm > + > el0_sync_invalid: > inv_entry 0, BAD_SYNC > ENDPROC(el0_sync_invalid) > @@ -840,8 +861,10 @@ el0_svc: > mov wsc_nr, #__NR_syscalls > el0_svc_naked: // compat entry point > stp x0, xscno, [sp, #S_ORIG_X0] // save the original x0 and syscall number > + isb_if_eqs > enable_dbg_and_irq > - ct_user_exit 1 > + ct_user_exit I don't think this is safe. here we issue the ISB *before* exiting a quiesecent state, so I think we can race with another CPU that calls kick_all_active_cpus_sync, e.g. CPU0 CPU1 ISB patch_some_text() kick_all_active_cpus_sync() ct_user_exit // not synchronized! use_of_patched_text() ... and therefore the ISB has no effect, which could be disasterous. I believe we need the ISB *after* we transition into a non-quiescent state, so that we can't possibly miss a context synchronization event. Thanks, Mark. From mboxrd@z Thu Jan 1 00:00:00 1970 From: mark.rutland@arm.com (Mark Rutland) Date: Tue, 3 Apr 2018 14:48:32 +0100 Subject: [PATCH 2/2] smp: introduce kick_active_cpus_sync() In-Reply-To: <20180401111108.mudkiewzn33sifvk@yury-thinkpad> References: <20180325175004.28162-1-ynorov@caviumnetworks.com> <20180325175004.28162-3-ynorov@caviumnetworks.com> <20180327102116.GA2464@arm.com> <20180401111108.mudkiewzn33sifvk@yury-thinkpad> Message-ID: <20180403134832.2cdae64uwuot6ryz@lakrids.cambridge.arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Yury, On Sun, Apr 01, 2018 at 02:11:08PM +0300, Yury Norov wrote: > +/* > + * Flush I-cache if CPU is in extended quiescent state > + */ This comment is misleading. An ISB doesn't touch the I-cache; it forces a context synchronization event. > + .macro isb_if_eqs > +#ifndef CONFIG_TINY_RCU > + bl rcu_is_watching > + tst w0, #0xff > + b.ne 1f The TST+B.NE can be a CBNZ: bl rcu_is_watching cbnz x0, 1f isb 1: > + /* Pairs with aarch64_insn_patch_text for EQS CPUs. */ > + isb > +1: > +#endif > + .endm > + > el0_sync_invalid: > inv_entry 0, BAD_SYNC > ENDPROC(el0_sync_invalid) > @@ -840,8 +861,10 @@ el0_svc: > mov wsc_nr, #__NR_syscalls > el0_svc_naked: // compat entry point > stp x0, xscno, [sp, #S_ORIG_X0] // save the original x0 and syscall number > + isb_if_eqs > enable_dbg_and_irq > - ct_user_exit 1 > + ct_user_exit I don't think this is safe. here we issue the ISB *before* exiting a quiesecent state, so I think we can race with another CPU that calls kick_all_active_cpus_sync, e.g. CPU0 CPU1 ISB patch_some_text() kick_all_active_cpus_sync() ct_user_exit // not synchronized! use_of_patched_text() ... and therefore the ISB has no effect, which could be disasterous. I believe we need the ISB *after* we transition into a non-quiescent state, so that we can't possibly miss a context synchronization event. Thanks, Mark. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Rutland Date: Tue, 03 Apr 2018 13:48:32 +0000 Subject: Re: [PATCH 2/2] smp: introduce kick_active_cpus_sync() Message-Id: <20180403134832.2cdae64uwuot6ryz@lakrids.cambridge.arm.com> List-Id: References: <20180325175004.28162-1-ynorov@caviumnetworks.com> <20180325175004.28162-3-ynorov@caviumnetworks.com> <20180327102116.GA2464@arm.com> <20180401111108.mudkiewzn33sifvk@yury-thinkpad> In-Reply-To: <20180401111108.mudkiewzn33sifvk@yury-thinkpad> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Yury Norov Cc: Will Deacon , "Paul E. McKenney" , Chris Metcalf , Christopher Lameter , Russell King - ARM Linux , Steven Rostedt , Mathieu Desnoyers , Catalin Marinas , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , linux-arm-kernel@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, kvm-ppc@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Hi Yury, On Sun, Apr 01, 2018 at 02:11:08PM +0300, Yury Norov wrote: > +/* > + * Flush I-cache if CPU is in extended quiescent state > + */ This comment is misleading. An ISB doesn't touch the I-cache; it forces a context synchronization event. > + .macro isb_if_eqs > +#ifndef CONFIG_TINY_RCU > + bl rcu_is_watching > + tst w0, #0xff > + b.ne 1f The TST+B.NE can be a CBNZ: bl rcu_is_watching cbnz x0, 1f isb 1: > + /* Pairs with aarch64_insn_patch_text for EQS CPUs. */ > + isb > +1: > +#endif > + .endm > + > el0_sync_invalid: > inv_entry 0, BAD_SYNC > ENDPROC(el0_sync_invalid) > @@ -840,8 +861,10 @@ el0_svc: > mov wsc_nr, #__NR_syscalls > el0_svc_naked: // compat entry point > stp x0, xscno, [sp, #S_ORIG_X0] // save the original x0 and syscall number > + isb_if_eqs > enable_dbg_and_irq > - ct_user_exit 1 > + ct_user_exit I don't think this is safe. here we issue the ISB *before* exiting a quiesecent state, so I think we can race with another CPU that calls kick_all_active_cpus_sync, e.g. CPU0 CPU1 ISB patch_some_text() kick_all_active_cpus_sync() ct_user_exit // not synchronized! use_of_patched_text() ... and therefore the ISB has no effect, which could be disasterous. I believe we need the ISB *after* we transition into a non-quiescent state, so that we can't possibly miss a context synchronization event. Thanks, Mark.