Re: [PATCH] x86: add workaround monitor bug

From: Ingo Molnar
Date: Fri Jul 08 2016 - 04:55:26 EST



* Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx> wrote:

> From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>
> Monitored cached line may not wake up from mwait on certain
> Goldmont based CPUs. This patch will avoid calling
> current_set_polling_and_test() and thereby not set the TIF_ flag.
> The result is that we'll always send IPIs for wakeups.
>
> Signed-off-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> ---
> arch/x86/include/asm/cpufeatures.h | 1 +
> arch/x86/include/asm/mwait.h | 2 +-
> arch/x86/kernel/cpu/intel.c | 5 +++++
> arch/x86/kernel/process.c | 2 +-
> 4 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 78dbd28..197a3f4 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -304,6 +304,7 @@
> #define X86_BUG_SYSRET_SS_ATTRS X86_BUG(8) /* SYSRET doesn't fix up SS attrs */
> #define X86_BUG_NULL_SEG X86_BUG(9) /* Nulling a selector preserves the base */
> #define X86_BUG_SWAPGS_FENCE X86_BUG(10) /* SWAPGS without input dep on GS */
> +#define X86_BUG_MONITOR X86_BUG(11) /* IPI required to wake up remote cpu */
>
>
> #ifdef CONFIG_X86_32
> diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
> index 0deeb2d..f37f2d8 100644
> --- a/arch/x86/include/asm/mwait.h
> +++ b/arch/x86/include/asm/mwait.h
> @@ -97,7 +97,7 @@ static inline void __sti_mwait(unsigned long eax, unsigned long ecx)
> */
> static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
> {
> - if (!current_set_polling_and_test()) {
> + if (static_cpu_has_bug(X86_BUG_MONITOR) || !current_set_polling_and_test()) {

Hm, this might be suboptimal: if MONITOR/MWAIT is implemented by setting the
exclusive flag for the monitored memory address and then snooping for cache
invalidation requests for that cache line, then not modifying the ->flags value
with TIF_POLLING_NRFLAG makes MWAIT not wake up - only the IPI would wake it up.

I think a better approach would be to still optimistically modify the ->flags
value _AND_ to also send an IPI, to make sure the wakeup is not lost. This means
that the woken CPU will wake up much faster (no IPI latency).

(The system will still bear the ovehread of sending and receiving the IPI, but
that cost is unavoidable if there's no other workaround for this erratum.)

Thanks,

Ingo