The SERIALIZE instruction gives software a way to force the processor to
complete all modifications to flags, registers and memory from previous
instructions and drain all buffered writes to memory before the next
instruction is fetched and executed. Thus, it serves the purpose of
sync_core(). Use it when available.
Use boot_cpu_has() and not static_cpu_has(); the most critical paths
(returning to user mode and from interrupt and NMI) will not reach
sync_core().
Cc: Andy Lutomirski <[email protected]>
Cc: Cathy Zhang <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Fenghua Yu <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Kyung Min Park <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: "Ravi V. Shankar" <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: [email protected]
Cc: [email protected]
Reviwed-by: Tony Luck <[email protected]>
Suggested-by: Andy Lutomirski <[email protected]>
Signed-off-by: Ricardo Neri <[email protected]>
---
---
arch/x86/include/asm/special_insns.h | 5 +++++
arch/x86/include/asm/sync_core.h | 10 +++++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 59a3e13204c3..0a2a60bba282 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -234,6 +234,11 @@ static inline void clwb(volatile void *__p)
#define nop() asm volatile ("nop")
+static inline void serialize(void)
+{
+ asm volatile(".byte 0xf, 0x1, 0xe8");
+}
+
#endif /* __KERNEL__ */
#endif /* _ASM_X86_SPECIAL_INSNS_H */
diff --git a/arch/x86/include/asm/sync_core.h b/arch/x86/include/asm/sync_core.h
index fdb5b356e59b..bf132c09d61b 100644
--- a/arch/x86/include/asm/sync_core.h
+++ b/arch/x86/include/asm/sync_core.h
@@ -5,6 +5,7 @@
#include <linux/preempt.h>
#include <asm/processor.h>
#include <asm/cpufeature.h>
+#include <asm/special_insns.h>
#ifdef CONFIG_X86_32
static inline void iret_to_self(void)
@@ -54,7 +55,8 @@ static inline void iret_to_self(void)
static inline void sync_core(void)
{
/*
- * There are quite a few ways to do this. IRET-to-self is nice
+ * Hardware can do this for us if SERIALIZE is available. Otherwise,
+ * there are quite a few ways to do this. IRET-to-self is nice
* because it works on every CPU, at any CPL (so it's compatible
* with paravirtualization), and it never exits to a hypervisor.
* The only down sides are that it's a bit slow (it seems to be
@@ -75,6 +77,12 @@ static inline void sync_core(void)
* Like all of Linux's memory ordering operations, this is a
* compiler barrier as well.
*/
+
+ if (boot_cpu_has(X86_FEATURE_SERIALIZE)) {
+ serialize();
+ return;
+ }
+
iret_to_self();
}
--
2.17.1
On July 26, 2020 9:31:32 PM PDT, Ricardo Neri <[email protected]> wrote:
>The SERIALIZE instruction gives software a way to force the processor
>to
>complete all modifications to flags, registers and memory from previous
>instructions and drain all buffered writes to memory before the next
>instruction is fetched and executed. Thus, it serves the purpose of
>sync_core(). Use it when available.
>
>Use boot_cpu_has() and not static_cpu_has(); the most critical paths
>(returning to user mode and from interrupt and NMI) will not reach
>sync_core().
>
>Cc: Andy Lutomirski <[email protected]>
>Cc: Cathy Zhang <[email protected]>
>Cc: Dave Hansen <[email protected]>
>Cc: Fenghua Yu <[email protected]>
>Cc: "H. Peter Anvin" <[email protected]>
>Cc: Kyung Min Park <[email protected]>
>Cc: Peter Zijlstra <[email protected]>
>Cc: "Ravi V. Shankar" <[email protected]>
>Cc: Sean Christopherson <[email protected]>
>Cc: [email protected]
>Cc: [email protected]
>Reviwed-by: Tony Luck <[email protected]>
>Suggested-by: Andy Lutomirski <[email protected]>
>Signed-off-by: Ricardo Neri <[email protected]>
>---
>---
> arch/x86/include/asm/special_insns.h | 5 +++++
> arch/x86/include/asm/sync_core.h | 10 +++++++++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/include/asm/special_insns.h
>b/arch/x86/include/asm/special_insns.h
>index 59a3e13204c3..0a2a60bba282 100644
>--- a/arch/x86/include/asm/special_insns.h
>+++ b/arch/x86/include/asm/special_insns.h
>@@ -234,6 +234,11 @@ static inline void clwb(volatile void *__p)
>
> #define nop() asm volatile ("nop")
>
>+static inline void serialize(void)
>+{
>+ asm volatile(".byte 0xf, 0x1, 0xe8");
>+}
>+
> #endif /* __KERNEL__ */
>
> #endif /* _ASM_X86_SPECIAL_INSNS_H */
>diff --git a/arch/x86/include/asm/sync_core.h
>b/arch/x86/include/asm/sync_core.h
>index fdb5b356e59b..bf132c09d61b 100644
>--- a/arch/x86/include/asm/sync_core.h
>+++ b/arch/x86/include/asm/sync_core.h
>@@ -5,6 +5,7 @@
> #include <linux/preempt.h>
> #include <asm/processor.h>
> #include <asm/cpufeature.h>
>+#include <asm/special_insns.h>
>
> #ifdef CONFIG_X86_32
> static inline void iret_to_self(void)
>@@ -54,7 +55,8 @@ static inline void iret_to_self(void)
> static inline void sync_core(void)
> {
> /*
>- * There are quite a few ways to do this. IRET-to-self is nice
>+ * Hardware can do this for us if SERIALIZE is available. Otherwise,
>+ * there are quite a few ways to do this. IRET-to-self is nice
> * because it works on every CPU, at any CPL (so it's compatible
> * with paravirtualization), and it never exits to a hypervisor.
> * The only down sides are that it's a bit slow (it seems to be
>@@ -75,6 +77,12 @@ static inline void sync_core(void)
> * Like all of Linux's memory ordering operations, this is a
> * compiler barrier as well.
> */
>+
>+ if (boot_cpu_has(X86_FEATURE_SERIALIZE)) {
>+ serialize();
>+ return;
>+ }
>+
> iret_to_self();
> }
>
Any reason to not make sync_core() an inline with alternatives?
For a really overenginered solution, but which might perform unnecessary poorly on existing hardware:
asm volatile("1: .byte 0xf, 0x1, 0xe8; 2:"
_ASM_EXTABLE(1b,2b));
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On July 26, 2020 10:55:15 PM PDT, [email protected] wrote:
>On July 26, 2020 9:31:32 PM PDT, Ricardo Neri
><[email protected]> wrote:
>>The SERIALIZE instruction gives software a way to force the processor
>>to
>>complete all modifications to flags, registers and memory from
>previous
>>instructions and drain all buffered writes to memory before the next
>>instruction is fetched and executed. Thus, it serves the purpose of
>>sync_core(). Use it when available.
>>
>>Use boot_cpu_has() and not static_cpu_has(); the most critical paths
>>(returning to user mode and from interrupt and NMI) will not reach
>>sync_core().
>>
>>Cc: Andy Lutomirski <[email protected]>
>>Cc: Cathy Zhang <[email protected]>
>>Cc: Dave Hansen <[email protected]>
>>Cc: Fenghua Yu <[email protected]>
>>Cc: "H. Peter Anvin" <[email protected]>
>>Cc: Kyung Min Park <[email protected]>
>>Cc: Peter Zijlstra <[email protected]>
>>Cc: "Ravi V. Shankar" <[email protected]>
>>Cc: Sean Christopherson <[email protected]>
>>Cc: [email protected]
>>Cc: [email protected]
>>Reviwed-by: Tony Luck <[email protected]>
>>Suggested-by: Andy Lutomirski <[email protected]>
>>Signed-off-by: Ricardo Neri <[email protected]>
>>---
>>---
>> arch/x86/include/asm/special_insns.h | 5 +++++
>> arch/x86/include/asm/sync_core.h | 10 +++++++++-
>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>
>>diff --git a/arch/x86/include/asm/special_insns.h
>>b/arch/x86/include/asm/special_insns.h
>>index 59a3e13204c3..0a2a60bba282 100644
>>--- a/arch/x86/include/asm/special_insns.h
>>+++ b/arch/x86/include/asm/special_insns.h
>>@@ -234,6 +234,11 @@ static inline void clwb(volatile void *__p)
>>
>> #define nop() asm volatile ("nop")
>>
>>+static inline void serialize(void)
>>+{
>>+ asm volatile(".byte 0xf, 0x1, 0xe8");
>>+}
>>+
>> #endif /* __KERNEL__ */
>>
>> #endif /* _ASM_X86_SPECIAL_INSNS_H */
>>diff --git a/arch/x86/include/asm/sync_core.h
>>b/arch/x86/include/asm/sync_core.h
>>index fdb5b356e59b..bf132c09d61b 100644
>>--- a/arch/x86/include/asm/sync_core.h
>>+++ b/arch/x86/include/asm/sync_core.h
>>@@ -5,6 +5,7 @@
>> #include <linux/preempt.h>
>> #include <asm/processor.h>
>> #include <asm/cpufeature.h>
>>+#include <asm/special_insns.h>
>>
>> #ifdef CONFIG_X86_32
>> static inline void iret_to_self(void)
>>@@ -54,7 +55,8 @@ static inline void iret_to_self(void)
>> static inline void sync_core(void)
>> {
>> /*
>>- * There are quite a few ways to do this. IRET-to-self is nice
>>+ * Hardware can do this for us if SERIALIZE is available. Otherwise,
>>+ * there are quite a few ways to do this. IRET-to-self is nice
>> * because it works on every CPU, at any CPL (so it's compatible
>> * with paravirtualization), and it never exits to a hypervisor.
>> * The only down sides are that it's a bit slow (it seems to be
>>@@ -75,6 +77,12 @@ static inline void sync_core(void)
>> * Like all of Linux's memory ordering operations, this is a
>> * compiler barrier as well.
>> */
>>+
>>+ if (boot_cpu_has(X86_FEATURE_SERIALIZE)) {
>>+ serialize();
>>+ return;
>>+ }
>>+
>> iret_to_self();
>> }
>>
>
>Any reason to not make sync_core() an inline with alternatives?
>
>For a really overenginered solution, but which might perform
>unnecessary poorly on existing hardware:
>
>asm volatile("1: .byte 0xf, 0x1, 0xe8; 2:"
> _ASM_EXTABLE(1b,2b));
(and : : : "memory" of course.)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Sun, Jul 26, 2020 at 09:31:32PM -0700, Ricardo Neri wrote:
> +static inline void serialize(void)
> +{
> + asm volatile(".byte 0xf, 0x1, 0xe8");
> +}
Can we pretty please have a comment with the binutils version that has
the mnomic? Such that when we increase the required binutils version we
can grep for this.
On Sun, Jul 26, 2020 at 09:31:32PM -0700, Ricardo Neri wrote:
> @@ -75,6 +77,12 @@ static inline void sync_core(void)
> * Like all of Linux's memory ordering operations, this is a
> * compiler barrier as well.
> */
> +
> + if (boot_cpu_has(X86_FEATURE_SERIALIZE)) {
> + serialize();
> + return;
> + }
> +
> iret_to_self();
I was sorta expecting something like:
alternative(IRET_TO_SELF, SERIALIZE, X86_FEATURE_SERIALIZE);
But instead you used boot_cpu_has() which is a dynamic test.
On Sun, Jul 26, 2020 at 10:55:15PM -0700, [email protected] wrote:
> For a really overenginered solution, but which might perform
> unnecessary poorly on existing hardware:
>
> asm volatile("1: .byte 0xf, 0x1, 0xe8; 2:"
> _ASM_EXTABLE(1b,2b));
Ha! cute, you take an #UD ?
We could optimize the #UD exception handler for this I suppose, but that
makes it an even worse hack. The simple alternative() seems like a much
simpler approach.
On July 27, 2020 1:20:03 AM PDT, [email protected] wrote:
>On Sun, Jul 26, 2020 at 09:31:32PM -0700, Ricardo Neri wrote:
>> +static inline void serialize(void)
>> +{
>> + asm volatile(".byte 0xf, 0x1, 0xe8");
>> +}
>
>Can we pretty please have a comment with the binutils version that has
>the mnomic? Such that when we increase the required binutils version we
>can grep for this.
It also needs : : : "memory" to be a barrier.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On July 27, 2020 1:36:19 AM PDT, [email protected] wrote:
>On Sun, Jul 26, 2020 at 10:55:15PM -0700, [email protected] wrote:
>> For a really overenginered solution, but which might perform
>> unnecessary poorly on existing hardware:
>>
>> asm volatile("1: .byte 0xf, 0x1, 0xe8; 2:"
>> _ASM_EXTABLE(1b,2b));
>
>Ha! cute, you take an #UD ?
>
>We could optimize the #UD exception handler for this I suppose, but
>that
>makes it an even worse hack. The simple alternative() seems like a much
>simpler approach.
If this is in any way performance critical, then no :) Taking the #UD has the cute property that we end up IRET on the way back, so we don't even need a fix-up path.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Mon, Jul 27, 2020 at 05:49:28AM -0700, [email protected] wrote:
> On July 27, 2020 1:36:19 AM PDT, [email protected] wrote:
> >On Sun, Jul 26, 2020 at 10:55:15PM -0700, [email protected] wrote:
> >> For a really overenginered solution, but which might perform
> >> unnecessary poorly on existing hardware:
> >>
> >> asm volatile("1: .byte 0xf, 0x1, 0xe8; 2:"
> >> _ASM_EXTABLE(1b,2b));
> >
> >Ha! cute, you take an #UD ?
> >
> >We could optimize the #UD exception handler for this I suppose, but
> >that makes it an even worse hack. The simple alternative() seems like
> >a much simpler approach.
>
> If this is in any way performance critical, then no :)
Yeah, I'm not sure.. the 'funny' thing is that typically call
sync_core() from an IPI anyway. And the synchronous broadcast IPI is by
far the most expensive part of that.
Something like this...
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 20e07feb4064..528e049ee1d9 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -989,12 +989,13 @@ void *text_poke_kgdb(void *addr, const void *opcode, size_t len)
static void do_sync_core(void *info)
{
- sync_core();
+ /* IRET implies sync_core() */
}
void text_poke_sync(void)
{
on_each_cpu(do_sync_core, NULL, 1);
+ sync_core();
}
struct text_poke_loc {
> Taking the #UD
> has the cute property that we end up IRET on the way back, so we don't
> even need a fix-up path.
I got that, what I had in mind was making sure #UD avoids the overhead
of doing exception entry/exit by adding an early exit.
Something like so:
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 8493f55e1167..a3f41d645944 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -96,6 +96,16 @@ __always_inline int is_valid_bugaddr(unsigned long addr)
return *(unsigned short *)addr == INSN_UD2;
}
+__always_inline int handle_serialize(struct pt_regs *regs)
+{
+ const char serialize[3] = { 0x0f, 0xe8, 0x02 };
+
+ if (regs->ip < TASK_SIZE_MAX)
+ return 0;
+
+ return !memcmp((const void *)regs->ip, serialize, 3);
+}
+
static nokprobe_inline int
do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str,
struct pt_regs *regs, long error_code)
@@ -252,8 +262,13 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op)
* handle it before exception entry to avoid recursive WARN
* in case exception entry is the one triggering WARNs.
*/
- if (!user_mode(regs) && handle_bug(regs))
- return;
+ if (!user_mode(regs)) {
+ if (handle_bug(regs))
+ return;
+
+ if (handle_serialize(regs))
+ return;
+ }
state = idtentry_enter(regs);
instrumentation_begin();
On Mon, Jul 27, 2020 at 03:05:36PM +0200, [email protected] wrote:
> Yeah, I'm not sure.. the 'funny' thing is that typically call
> sync_core() from an IPI anyway. And the synchronous broadcast IPI is by
> far the most expensive part of that.
>
> Something like this...
>
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 20e07feb4064..528e049ee1d9 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -989,12 +989,13 @@ void *text_poke_kgdb(void *addr, const void *opcode, size_t len)
>
> static void do_sync_core(void *info)
> {
> - sync_core();
> + /* IRET implies sync_core() */
> }
>
> void text_poke_sync(void)
> {
> on_each_cpu(do_sync_core, NULL, 1);
> + sync_core();
> }
>
> struct text_poke_loc {
So 'people' have wanted to optimize this for NOHZ_FULL and I suppose
virt as well.
IFF VMENTER is serializing, I suppose we can simply do something like:
bool text_poke_cond(int cpu, void *info)
{
/*
* If we observe the vCPU is preempted, it will do VMENTER
* no point in sending an IPI to SERIALIZE.
*/
return !vcpu_is_preempted(cpu);
}
void text_poke_sync(void)
{
smp_call_function_many_cond(cpu_possible_mask,
do_sync_core, NULL, 1, text_poke_cond);
sync_core();
}
The 'same' for NOHZ_FULL, except we need to cmpxchg a value such that
if the cmpxchg() succeeds we know the CPU is in userspace and will
SERIALIZE on the next entry. Much like kvm_flush_tlb_others().
Anyway, that's all hand-wavey.. I'll let someone that cares about those
things write actual patches :-)
> For a really overenginered solution, but which might perform unnecessary poorly on existing hardware:
>
> asm volatile("1: .byte 0xf, 0x1, 0xe8; 2:"
> _ASM_EXTABLE(1b,2b));
You win the prize for the smallest code. Might need (the already large) comment to double
in size to explain the subtleties!
-Tony
On Mon, Jul 27, 2020 at 10:20:03AM +0200, [email protected] wrote:
> On Sun, Jul 26, 2020 at 09:31:32PM -0700, Ricardo Neri wrote:
> > +static inline void serialize(void)
> > +{
> > + asm volatile(".byte 0xf, 0x1, 0xe8");
> > +}
>
> Can we pretty please have a comment with the binutils version that has
> the mnomic? Such that when we increase the required binutils version we
> can grep for this.
This is supported since binutils 2.35[1]. I'll add a comment with the
clarification.
Thanks and BR,
Ricardo
[1]. https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob_plain;f=gas/NEWS;;hb=refs/tags/binutils-2_35
On Mon, Jul 27, 2020 at 05:47:32AM -0700, [email protected] wrote:
> On July 27, 2020 1:20:03 AM PDT, [email protected] wrote:
> >On Sun, Jul 26, 2020 at 09:31:32PM -0700, Ricardo Neri wrote:
> >> +static inline void serialize(void)
> >> +{
> >> + asm volatile(".byte 0xf, 0x1, 0xe8");
> >> +}
> >
> >Can we pretty please have a comment with the binutils version that has
> >the mnomic? Such that when we increase the required binutils version we
> >can grep for this.
>
> It also needs : : : "memory" to be a barrier.
Sure Peter, I will make this change.
Thanks and BR,
Ricardo
On Mon, Jul 27, 2020 at 03:30:20PM +0200, [email protected] wrote:
> On Mon, Jul 27, 2020 at 03:05:36PM +0200, [email protected] wrote:
> > Yeah, I'm not sure.. the 'funny' thing is that typically call
> > sync_core() from an IPI anyway. And the synchronous broadcast IPI is by
> > far the most expensive part of that.
> >
> > Something like this...
> >
> > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > index 20e07feb4064..528e049ee1d9 100644
> > --- a/arch/x86/kernel/alternative.c
> > +++ b/arch/x86/kernel/alternative.c
> > @@ -989,12 +989,13 @@ void *text_poke_kgdb(void *addr, const void *opcode, size_t len)
> >
> > static void do_sync_core(void *info)
> > {
> > - sync_core();
> > + /* IRET implies sync_core() */
> > }
> >
> > void text_poke_sync(void)
> > {
> > on_each_cpu(do_sync_core, NULL, 1);
> > + sync_core();
> > }
> >
> > struct text_poke_loc {
>
> So 'people' have wanted to optimize this for NOHZ_FULL and I suppose
> virt as well.
>
> IFF VMENTER is serializing, I suppose we can simply do something like:
>
> bool text_poke_cond(int cpu, void *info)
> {
> /*
> * If we observe the vCPU is preempted, it will do VMENTER
> * no point in sending an IPI to SERIALIZE.
> */
> return !vcpu_is_preempted(cpu);
> }
>
> void text_poke_sync(void)
> {
> smp_call_function_many_cond(cpu_possible_mask,
> do_sync_core, NULL, 1, text_poke_cond);
> sync_core();
> }
>
> The 'same' for NOHZ_FULL, except we need to cmpxchg a value such that
> if the cmpxchg() succeeds we know the CPU is in userspace and will
> SERIALIZE on the next entry. Much like kvm_flush_tlb_others().
>
>
> Anyway, that's all hand-wavey.. I'll let someone that cares about those
> things write actual patches :-)
I think I got a little lost here. If I understand correctly, there are
two alternatives to implement support for serialize better:
a) alternative(IRET_TO_SELF, SERIALIZE, X86_FEATURE_SERIALIZE); or
b) asm volatile("1:.byte 0xf, 0x1, 0xe8;2:" _ASM_EXTABLE(1b:2b)
a) would be the traditional and simpler solution. b) would rely on
causing an #UD and getting an IRET on existing hardware b) would need some
more optimization work when handling the exception and a few reworks on
the poke patching code.
Which option should I focus on? Which option would be more desirable/better?
Thanks and BR,
Ricardo
On Mon, Jul 27, 2020 at 09:41:14PM -0700, Ricardo Neri wrote:
> I think I got a little lost here.
Hehe, sorry. I got carried away, it's just that recently people
expressed interest in 'fixing' some of the text_poke_sync() issues
again.
> If I understand correctly, there are
> two alternatives to implement support for serialize better:
>
> a) alternative(IRET_TO_SELF, SERIALIZE, X86_FEATURE_SERIALIZE); or
> b) asm volatile("1:.byte 0xf, 0x1, 0xe8;2:" _ASM_EXTABLE(1b:2b)
>
> a) would be the traditional and simpler solution. b) would rely on
> causing an #UD and getting an IRET on existing hardware b) would need some
> more optimization work when handling the exception and a few reworks on
> the poke patching code.
>
> Which option should I focus on? Which option would be more desirable/better?
I'd say go with (a) for now. We can always go overboard later ;-)