[PATCH] x86/cpu: Fix typos and improve the comments in sync_core()

[Ingo Molnar]

* tip-bot2 for Ricardo Neri <tip-bot2@xxxxxxxxxxxxx> wrote:

> --- 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,14 +55,23 @@ static inline void iret_to_self(void)
>  static inline void sync_core(void)
>  {
>  	/*
> +	 * The SERIALIZE instruction is the most straightforward way to
> +	 * do this but it not universally available.
> +	 */
> +	if (static_cpu_has(X86_FEATURE_SERIALIZE)) {
> +		serialize();
> +		return;
> +	}
> +
> +	/*
> +	 * For all other processors, 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 a bit more than 2x slower than the fastest
> +	 * options) and that it unmasks NMIs.  The "push %cs" is needed
> +	 * because, in paravirtual environments, __KERNEL_CS may not be a
> +	 * valid CS value when we do IRET directly.

So there's two typos in the new comments, there are at least two 
misapplied commas, it departs from existing style, and there's a typo 
in the existing comments as well.

Also, before this patch the 'compiler barrier' comment was valid for 
the whole function (there was no branching), but after this patch it 
reads of it was only valid for the legacy IRET-to-self branch.

Which together broke my detector and triggered a bit of compulsive 
bike-shed painting. ;-) See the resulting patch below.

Thanks,

	Ingo

================>
From: Ingo Molnar <mingo@xxxxxxxxxx>
Date: Tue, 18 Aug 2020 07:24:05 +0200
Subject: [PATCH] x86/cpu: Fix typos and improve the comments in sync_core()

- Fix typos.

- Move the compiler barrier comment to the top, because it's valid for the
  whole function, not just the legacy branch.

Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>
---
 arch/x86/include/asm/sync_core.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/sync_core.h b/arch/x86/include/asm/sync_core.h
index 4631c0f969d4..0fd4a9dfb29c 100644
--- a/arch/x86/include/asm/sync_core.h
+++ b/arch/x86/include/asm/sync_core.h
@@ -47,16 +47,19 @@ static inline void iret_to_self(void)
  *
  *  b) Text was modified on a different CPU, may subsequently be
  *     executed on this CPU, and you want to make sure the new version
- *     gets executed.  This generally means you're calling this in a IPI.
+ *     gets executed.  This generally means you're calling this in an IPI.
  *
  * If you're calling this for a different reason, you're probably doing
  * it wrong.
+ *
+ * Like all of Linux's memory ordering operations, this is a
+ * compiler barrier as well.
  */
 static inline void sync_core(void)
 {
 	/*
 	 * The SERIALIZE instruction is the most straightforward way to
-	 * do this but it not universally available.
+	 * do this, but it is not universally available.
 	 */
 	if (static_cpu_has(X86_FEATURE_SERIALIZE)) {
 		serialize();
@@ -67,10 +70,10 @@ static inline void sync_core(void)
 	 * For all other processors, 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
+	 * to a hypervisor.  The only downsides are that it's a bit slow
 	 * (it seems to be a bit more than 2x slower than the fastest
-	 * options) and that it unmasks NMIs.  The "push %cs" is needed
-	 * because, in paravirtual environments, __KERNEL_CS may not be a
+	 * options) and that it unmasks NMIs.  The "push %cs" is needed,
+	 * because in paravirtual environments __KERNEL_CS may not be a
 	 * valid CS value when we do IRET directly.
 	 *
 	 * In case NMI unmasking or performance ever becomes a problem,
@@ -81,9 +84,6 @@ static inline void sync_core(void)
 	 * CPUID is the conventional way, but it's nasty: it doesn't
 	 * exist on some 486-like CPUs, and it usually exits to a
 	 * hypervisor.
-	 *
-	 * Like all of Linux's memory ordering operations, this is a
-	 * compiler barrier as well.
 	 */
 	iret_to_self();
 }