Re: [PATCH] x86: add workaround monitor bug

[Ingo Molnar]

* 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