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

[Ricardo Neri]

On Tue, Aug 18, 2020 at 07:31:30AM +0200, Ingo Molnar wrote:
> 
> * 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.

Indeed, I missed this grammar error.

>  	 */
>  	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

I didn't realize that the double spaces after the period were part of the
style.

FWIW,

Reviewed-by: Ricardo Neri <ricardo.neri-calderon@xxxxxxxxxxxxxxx>