svn commit: r312199 - head/sys/kern

[Bruce Evans]

On Sat, 14 Jan 2017, Mark Johnston wrote:

> Log:
>  Stop the scheduler upon panic even in non-SMP kernels.
>
>  This is needed for kernel dumps to work, as the panicking thread will call
>  into code that makes use of kernel locks.
>
>  Reported and tested by:	Eugene Grosbein
>  MFC after:	1 week
>
> Modified:
>  head/sys/kern/kern_shutdown.c
>
> Modified: head/sys/kern/kern_shutdown.c
> ==============================================================================
> --- head/sys/kern/kern_shutdown.c	Sat Jan 14 22:16:01 2017	(r312198)
> +++ head/sys/kern/kern_shutdown.c	Sat Jan 14 22:16:03 2017	(r312199)
> @@ -733,13 +733,13 @@ vpanic(const char *fmt, va_list ap)
> 		CPU_CLR(PCPU_GET(cpuid), &other_cpus);
> 		stop_cpus_hard(other_cpus);
> 	}
> +#endif
>
> 	/*
> 	 * Ensure that the scheduler is stopped while panicking, even if panic
> 	 * has been entered from kdb.
> 	 */
> 	td->td_stopsched = 1;
> -#endif
>
> 	bootopt = RB_AUTOBOOT;
> 	newpanic = 0;

This variable causes various problems.  In my patches for early use of the
message buffer, I have to provide a fake td to point to it since curthread
is not initialized early and message buffer code does bogus locking.

This variable used to be a global, but was changed to per-thread because it
was claimed to cause cache contention.  I don't see how a the global can
cause significant contention (or more than other globals) provided it is
not in the same cache line as an often-written-to global.  This variable
is not even checked by inline mutex code, so this code is broken and without
INVARIANTS the variable is mainly accessed in the contended case, but the
contended case is already slow and accesses many other globals.

The accesses are well obfuscated by ifdefs.  __mtx_lock_sleep() seems to
have only the following at the beginning in the non-INVARIANTS case:
- SCHEDULER_STOPPED() accesses this variable
- then if ADAPTIVE_MUTEXES is configured, the much larger global mtx_delay
   is accessed.  Even standard compiler optimizations are likely to copy from
   static global data to initialize constants a bit like this.
These seem to be the only globals in the main, not counting per-CPU ones
like curthread.  This and other mutex functions recently gained the
dubious optimization of initializing lda early in the main path.  This is
correct if the main path is usually to do almost everything (because the
usual case is contended).

Scheduler code also checks 'cold' quite often and kdb_active sometimes.
It is not called as often as mutex code.  No care is taken to keep 'cold'
away from other globals.  On i386, it is just 'int cold = 1;' in machdep.c,
so it is placed wherever the linker decides to pack it.

WHen this variable was changed from global to thread-local, it was set for
more threads.  Now it is set for only 1 thread.  This depends on all other
CPUs being stopped so that they can't be running other threads.  If another
thread somehow runs, then the thread variable is much more broken than the
global since it doesn't stop scheduling on all threads.  Perhaps the change
to stop other threads made the global less pessimal than before (I can't
see why -- efficiency doesn't matter while the scheduler is stopped --
we just need to make the access efficient in normal operation when the
variable is 0).

Bruce