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