From owner-svn-src-all@freebsd.org Sun Jan 15 05:54:59 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 2CFF3CB08E3; Sun, 15 Jan 2017 05:54:59 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id B7E8110DC; Sun, 15 Jan 2017 05:54:58 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id 6D60BD63CD1; Sun, 15 Jan 2017 16:54:56 +1100 (AEDT) Date: Sun, 15 Jan 2017 16:54:55 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Mark Johnston cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r312199 - head/sys/kern In-Reply-To: <201701142216.v0EMG3rN093070@repo.freebsd.org> Message-ID: <20170115161524.A55089@besplex.bde.org> References: <201701142216.v0EMG3rN093070@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=BKLDlBYG c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=RAJ7xaerS_eBAxpZtjMA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 15 Jan 2017 05:54:59 -0000 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