x86: add workaround monitor bug

From: Peter Zijlstra <[email protected]>

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 <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

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

arch/x86/include/asm/cpufeatures.h +1 -0

index 78dbd28..197a3f4 100644
@@ -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

arch/x86/include/asm/mwait.h +0 -0

index 0deeb2d..f37f2d8 100644

arch/x86/include/asm/mwait.h static inline void __sti_mwait(unsigned long eax, unsigned long ecx) · line 100

Quoted 4 lines

static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx) {

arch/x86/include/asm/mwait.h +1 -1

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

Quoted 9 lines · Ingo Molnar

arch/x86/include/asm/mwait.h static inline void __sti_mwait(unsigned long eax, unsigned long ecx) · line 100

Quoted 4 lines

static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx) {

arch/x86/include/asm/mwait.h +1 -1

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

Confused.. POLLING_NRFLAGS is not used to wake up ever. It is only used to determine if we want to send IPIs or not.

And since we _must_ send an IPI in this case, because the monitor is busted, we cannot set this.

Quoted 3 lines

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

This is exactly what is done. See resched_curr()'s use of set_nr_and_not_polling(). That does:

	if (!(fetch_or(&flags, NEED_RESCHED) & POLLING_NRFLAG))
		smp_send_reschedule(cpu);

So we unconditionally set NEED_RESCHED, if, when we set that, POLLING was set, we skip the IPI.

So again, since monitor is busted, simply setting NEED_RESCHED will not wake us, we must send the IPI, this is achieved by not setting POLLING_NRFLAG.

Quoted 15 lines · Peter Zijlstra

Quoted 4 lines · Ingo Molnar

Quoted 4 lines

static inline void mwait_idle_with_hints(unsigned long eax,
unsigned long ecx) {

(quoted diff) +1 -1

-	if (!current_set_polling_and_test()) {
+	if (static_cpu_has_bug(X86_BUG_MONITOR)

Quoted 8 lines

Quoted 1 line

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

Quoted 2 lines

Confused.. POLLING_NRFLAGS is not used to wake up ever. It is only used to determine if we want to send IPIs or not.

I called the IPI the 'wakeup' - it's the 'CPU wakeup' :-)

Quoted 8 lines

And since we _must_ send an IPI in this case, because the monitor is busted, we cannot set this.

Quoted 4 lines

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

Quoted 8 lines

This is exactly what is done. See resched_curr()'s use of set_nr_and_not_polling(). That does:

	if (!(fetch_or(&flags, NEED_RESCHED) & POLLING_NRFLAG))
		smp_send_reschedule(cpu);

So we unconditionally set NEED_RESCHED, if, when we set that, POLLING was set, we skip the IPI.

Ah, indeed, we set NEED_RESCHED in the same memory address that __monitor() is watching so all is good.

Quoted 3 lines

So again, since monitor is busted, simply setting NEED_RESCHED will not wake us, we must send the IPI, this is achieved by not setting POLLING_NRFLAG.

Yeah, so I got the impression that it might be broken in only certain circumstances, or is it completely busted?

Thanks,

Ingo

Quoted 43 lines · On Fri, 8 Jul 2016 14:07:12 +0200 Ingo Molnar <[email protected]> wrote:

Quoted 15 lines · Peter Zijlstra

Quoted 4 lines · Ingo Molnar

Quoted 4 lines

static inline void mwait_idle_with_hints(unsigned long eax,
unsigned long ecx) {

(quoted diff) +1 -1

-	if (!current_set_polling_and_test()) {
+	if (static_cpu_has_bug(X86_BUG_MONITOR)

Quoted 8 lines

Quoted 1 line

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

Quoted 2 lines

Confused.. POLLING_NRFLAGS is not used to wake up ever. It is only used to determine if we want to send IPIs or not.

I called the IPI the 'wakeup' - it's the 'CPU wakeup' :-)

Quoted 8 lines

And since we _must_ send an IPI in this case, because the monitor is busted, we cannot set this.

Quoted 4 lines

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

Quoted 8 lines

This is exactly what is done. See resched_curr()'s use of set_nr_and_not_polling(). That does:

	if (!(fetch_or(&flags, NEED_RESCHED) & POLLING_NRFLAG))
		smp_send_reschedule(cpu);

So we unconditionally set NEED_RESCHED, if, when we set that, POLLING was set, we skip the IPI.

Ah, indeed, we set NEED_RESCHED in the same memory address that __monitor() is watching so all is good.

Quoted 7 lines

Quoted 3 lines

So again, since monitor is busted, simply setting NEED_RESCHED will not wake us, we must send the IPI, this is achieved by not setting POLLING_NRFLAG.

Yeah, so I got the impression that it might be broken in only certain circumstances, or is it completely busted?

That is right, monitor is only partially broken not completely busted. I don't have the statistics but it is not rare to miss wakeup. The typical symptom is random slowness and fails to boot, without this patch.

So doing both can speed up wake up in some cases.

Jacob

Quoted 3 lines

Thanks,

Ingo