From: Mark Rutland <mark.rutland@arm.com>
To: Yury Norov <ynorov@caviumnetworks.com>
Cc: Will Deacon <will.deacon@arm.com>,
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
Chris Metcalf <cmetcalf@mellanox.com>,
Christopher Lameter <cl@linux.com>,
Russell King - ARM Linux <linux@armlinux.org.uk>,
Steven Rostedt <rostedt@goodmis.org>,
Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
Catalin Marinas <catalin.marinas@arm.com>,
Pekka Enberg <penberg@kernel.org>,
David Rientjes <rientjes@google.com>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>,
Andrew Morton <akpm@linux-foundation.org>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
Michael Ellerman <mpe@ellerman.id.au>,
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()
Date: Tue, 3 Apr 2018 14:48:32 +0100 [thread overview]
Message-ID: <20180403134832.2cdae64uwuot6ryz@lakrids.cambridge.arm.com> (raw)
In-Reply-To: <20180401111108.mudkiewzn33sifvk@yury-thinkpad>
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.
WARNING: multiple messages have this Message-ID (diff)
From: mark.rutland@arm.com (Mark Rutland)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 2/2] smp: introduce kick_active_cpus_sync()
Date: Tue, 3 Apr 2018 14:48:32 +0100 [thread overview]
Message-ID: <20180403134832.2cdae64uwuot6ryz@lakrids.cambridge.arm.com> (raw)
In-Reply-To: <20180401111108.mudkiewzn33sifvk@yury-thinkpad>
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.
WARNING: multiple messages have this Message-ID (diff)
From: Mark Rutland <mark.rutland@arm.com>
To: Yury Norov <ynorov@caviumnetworks.com>
Cc: Will Deacon <will.deacon@arm.com>,
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
Chris Metcalf <cmetcalf@mellanox.com>,
Christopher Lameter <cl@linux.com>,
Russell King - ARM Linux <linux@armlinux.org.uk>,
Steven Rostedt <rostedt@goodmis.org>,
Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
Catalin Marinas <catalin.marinas@arm.com>,
Pekka Enberg <penberg@kernel.org>,
David Rientjes <rientjes@google.com>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>,
Andrew Morton <akpm@linux-foundation.org>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
Michael Ellerman <mpe@ellerman.id.au>,
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()
Date: Tue, 03 Apr 2018 13:48:32 +0000 [thread overview]
Message-ID: <20180403134832.2cdae64uwuot6ryz@lakrids.cambridge.arm.com> (raw)
In-Reply-To: <20180401111108.mudkiewzn33sifvk@yury-thinkpad>
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.
next prev parent reply other threads:[~2018-04-03 13:48 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-25 17:50 [PATCH 0/2] smp: don't kick CPUs running idle or nohz_full tasks Yury Norov
2018-03-25 17:50 ` Yury Norov
2018-03-25 17:50 ` Yury Norov
2018-03-25 17:50 ` [PATCH 1/2] rcu: declare rcu_eqs_special_set() in public header Yury Norov
2018-03-25 17:50 ` Yury Norov
2018-03-25 17:50 ` Yury Norov
2018-03-25 19:12 ` Paul E. McKenney
2018-03-25 19:12 ` Paul E. McKenney
2018-03-25 19:12 ` Paul E. McKenney
2018-03-25 19:18 ` Yury Norov
2018-03-25 19:18 ` Yury Norov
2018-03-25 19:18 ` Yury Norov
2018-03-25 17:50 ` [PATCH 2/2] smp: introduce kick_active_cpus_sync() Yury Norov
2018-03-25 17:50 ` Yury Norov
2018-03-25 17:50 ` Yury Norov
2018-03-25 19:23 ` Paul E. McKenney
2018-03-25 19:23 ` Paul E. McKenney
2018-03-25 19:23 ` Paul E. McKenney
2018-03-25 20:11 ` Yury Norov
2018-03-25 20:11 ` Yury Norov
2018-03-25 20:11 ` Yury Norov
2018-03-26 12:45 ` Paul E. McKenney
2018-03-26 12:45 ` Paul E. McKenney
2018-03-26 12:45 ` Paul E. McKenney
2018-03-28 13:36 ` Yury Norov
2018-03-28 13:36 ` Yury Norov
2018-03-28 13:36 ` Yury Norov
2018-03-28 13:56 ` Paul E. McKenney
2018-03-28 13:56 ` Paul E. McKenney
2018-03-28 13:56 ` Paul E. McKenney
2018-03-28 14:41 ` Yury Norov
2018-03-28 14:41 ` Yury Norov
2018-03-28 14:41 ` Yury Norov
2018-03-28 14:45 ` Paul E. McKenney
2018-03-28 14:45 ` Paul E. McKenney
2018-03-28 14:45 ` Paul E. McKenney
2018-03-26 8:53 ` Andrea Parri
2018-03-26 8:53 ` Andrea Parri
2018-03-26 8:53 ` Andrea Parri
2018-03-26 18:57 ` Steven Rostedt
2018-03-26 18:57 ` Steven Rostedt
2018-03-26 18:57 ` Steven Rostedt
2018-03-28 12:59 ` Yury Norov
2018-03-28 12:59 ` Yury Norov
2018-03-28 12:59 ` Yury Norov
2018-03-27 10:21 ` Will Deacon
2018-03-27 10:21 ` Will Deacon
2018-03-27 10:21 ` Will Deacon
2018-03-28 10:58 ` Yury Norov
2018-03-28 10:58 ` Yury Norov
2018-03-28 10:58 ` Yury Norov
2018-04-01 11:11 ` Yury Norov
2018-04-01 11:11 ` Yury Norov
2018-04-01 11:11 ` Yury Norov
2018-04-01 14:10 ` Paul E. McKenney
2018-04-01 14:10 ` Paul E. McKenney
2018-04-01 14:10 ` Paul E. McKenney
2018-04-03 13:48 ` Mark Rutland [this message]
2018-04-03 13:48 ` Mark Rutland
2018-04-03 13:48 ` Mark Rutland
2018-04-04 3:36 ` Yury Norov
2018-04-04 3:36 ` Yury Norov
2018-04-04 3:36 ` Yury Norov
2018-04-04 9:08 ` Mark Rutland
2018-04-04 9:08 ` Mark Rutland
2018-04-04 9:08 ` Mark Rutland
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180403134832.2cdae64uwuot6ryz@lakrids.cambridge.arm.com \
--to=mark.rutland@arm.com \
--cc=akpm@linux-foundation.org \
--cc=benh@kernel.crashing.org \
--cc=catalin.marinas@arm.com \
--cc=cl@linux.com \
--cc=cmetcalf@mellanox.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=kvm-ppc@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux@armlinux.org.uk \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=mpe@ellerman.id.au \
--cc=paulmck@linux.vnet.ibm.com \
--cc=paulus@samba.org \
--cc=penberg@kernel.org \
--cc=rientjes@google.com \
--cc=rostedt@goodmis.org \
--cc=will.deacon@arm.com \
--cc=ynorov@caviumnetworks.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.