svn commit: r355475 - head/sys/x86/x86

[Konstantin Belousov]

Author: kib
Date: Sat Dec  7 00:28:08 2019
New Revision: 355475
URL: https://svnweb.freebsd.org/changeset/base/355475

Log:
  x86: Restore the critical section around whole ipi_bitmap_handler() if
  hardclock IPI is delivered.
  
  In the current code after r355311, critical section is taken only
  around hardclockintr() call, and sched_preempt() is called after the
  section is exited. If we reschedule after exit, as we typically would
  due to conditions that caused IPI, in ULE the runq tdq_ipipending is
  not cleared, which blocks generation of further preempt IPIs.
  
  Since all relatively modern (10 years) hardware has per-cpu event
  timers, restoring the critical section conditionally does not affect
  it.
  
  Reported and tested by: cy
  Diagnosed and reviewed by: jeff (previous version)
  Sponsored by:	The FreeBSD Foundation
  Differential revision:	https://reviews.freebsd.org/D22716

Modified:
  head/sys/x86/x86/mp_x86.c

Modified: head/sys/x86/x86/mp_x86.c
==============================================================================
--- head/sys/x86/x86/mp_x86.c	Sat Dec  7 00:23:19 2019	(r355474)
+++ head/sys/x86/x86/mp_x86.c	Sat Dec  7 00:28:08 2019	(r355475)
@@ -1262,10 +1262,28 @@ ipi_bitmap_handler(struct trapframe frame)
 	u_int ipi_bitmap;
 
 	td = curthread;
+	ipi_bitmap = atomic_readandclear_int(&cpuid_to_pcpu[cpu]->
+	    pc_ipi_bitmap);
+
+	/*
+	 * sched_preempt() must be called to clear the pending preempt
+	 * IPI to enable delivery of further preempts.  However, the
+	 * critical section will cause extra scheduler lock thrashing
+	 * when used unconditionally.  Only critical_enter() if
+	 * hardclock must also run, which requires the section entry.
+	 */
+	if (ipi_bitmap & (1 << IPI_HARDCLOCK))
+		critical_enter();
+
 	td->td_intr_nesting_level++;
 	oldframe = td->td_intr_frame;
 	td->td_intr_frame = &frame;
-	ipi_bitmap = atomic_readandclear_int(&cpuid_to_pcpu[cpu]->pc_ipi_bitmap);
+	if (ipi_bitmap & (1 << IPI_PREEMPT)) {
+#ifdef COUNT_IPIS
+		(*ipi_preempt_counts[cpu])++;
+#endif
+		sched_preempt(td);
+	}
 	if (ipi_bitmap & (1 << IPI_AST)) {
 #ifdef COUNT_IPIS
 		(*ipi_ast_counts[cpu])++;
@@ -1273,23 +1291,15 @@ ipi_bitmap_handler(struct trapframe frame)
 		/* Nothing to do for AST */
 	}
 	if (ipi_bitmap & (1 << IPI_HARDCLOCK)) {
-		critical_enter();
 #ifdef COUNT_IPIS
 		(*ipi_hardclock_counts[cpu])++;
 #endif
 		hardclockintr();
-		critical_exit();
 	}
-
-	/* Run preempt after clock handlers since it may switch. */
-	if (ipi_bitmap & (1 << IPI_PREEMPT)) {
-#ifdef COUNT_IPIS
-		(*ipi_preempt_counts[cpu])++;
-#endif
-		sched_preempt(td);
-	}
 	td->td_intr_frame = oldframe;
 	td->td_intr_nesting_level--;
+	if (ipi_bitmap & (1 << IPI_HARDCLOCK))
+		critical_exit();
 }
 
 /*