From owner-svn-src-all@freebsd.org Wed Jul 12 15:42:34 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 9929CD9B4EA; Wed, 12 Jul 2017 15:42:34 +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 4698867975; Wed, 12 Jul 2017 15:42:33 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id 73B79D69A54; Thu, 13 Jul 2017 01:42:26 +1000 (AEST) Date: Thu, 13 Jul 2017 01:42:25 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ian Lepore cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r320901 - in head/sys: amd64/amd64 isa x86/isa In-Reply-To: <201707120242.v6C2gvDB026199@repo.freebsd.org> Message-ID: <20170712224803.G1991@besplex.bde.org> References: <201707120242.v6C2gvDB026199@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=KeqiiUQD c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=MLXom4X1Ctjlc9KFhS4A: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: Wed, 12 Jul 2017 15:42:34 -0000 On Wed, 12 Jul 2017, Ian Lepore wrote: > Log: > Protect access to the AT realtime clock with its own mutex. > > The mutex protecting access to the registered realtime clock should not be > overloaded to protect access to the atrtc hardware, which might not even be > the registered rtc. More importantly, the resettodr mutex needs to be > eliminated to remove locking/sleeping restrictions on clock drivers, and > that can't happen if MD code for amd64 depends on it. This change moves the > protection into what's really being protected: access to the atrtc date and > time registers. The spin mutex provided some protection against timing bugs caused by preemption, but it is impossible to hold the mutex around the user code that is setting the time, so both the kernel and user code should check if the operation took too long and fail/retry if it did. With correct code like that, spin mutexes are still good for limiting retries. I think it is best to try to hold one around the entire kernel part of the operation, and release it when sleeping at lower levels. > Modified: head/sys/x86/isa/atrtc.c > ============================================================================== > --- head/sys/x86/isa/atrtc.c Tue Jul 11 21:55:20 2017 (r320900) > +++ head/sys/x86/isa/atrtc.c Wed Jul 12 02:42:57 2017 (r320901) > @@ -53,9 +53,17 @@ __FBSDID("$FreeBSD$"); > #include > #include "clock_if.h" > > +/* > + * clock_lock protects low-level access to individual hardware registers. > + * atrtc_time_lock protects the entire sequence of accessing multiple registers > + * to read or write the date and time. > + */ > #define RTC_LOCK do { if (!kdb_active) mtx_lock_spin(&clock_lock); } while (0) > #define RTC_UNLOCK do { if (!kdb_active) mtx_unlock_spin(&clock_lock); } while (0) This has a lot of old design and implementation bugs: - it abuses the i8254 mutex for the rtc - it obfuscates this using macros - locking individual register accesses is buggy. Register accesses need to be locked in groups like your new code does - the kdb_active hack is buggy (it can't use mutexes and is unnecessarily non-reentrant) The locks should be separate and statically allocated. > > +struct mtx atrtc_time_lock; > +MTX_SYSINIT(atrtc_lock_init, &atrtc_time_lock, "atrtc", MTX_DEF); > + I think the old mutex should be used here, after fixing it. Locking individual register access is a special case of locking groups of register acceses. It must be a spin mutex for low-level use, and that is good for high-level use too, to complete the high-level operations as soon as possible. You used this mutex for efirt.c, but I think that is another error like sharing the i8254 mutex (or the resettodr mutex). Different clock drivers should just use different mutexes. Perhaps some drivers actually need sleep mutexes. > ... > @@ -352,6 +364,7 @@ atrtc_gettime(device_t dev, struct timespec *ts) > * to make sure that no more than 240us pass after we start reading, > * and try again if so. > */ > + mtx_lock(&atrtc_time_lock); > while (rtcin(RTC_STATUSA) & RTCSA_TUP) > continue; > critical_enter(); Note the comment about the 240us window. On i386, this was broken by SMPng, and almost fixed by starting the locking outside of the loop (a spin mutex would fix it). On i386, before SMPng, there was an splhigh() starting before the loop. rtcin() also used splhigh(). This was only shortly before SMPng -- FreeBSD-4 had both splhigh()'s, but FreeBSD-3 had neither. With no locking, an interrupt after the read of the status register can occur, and interrupt handling can easily take longer than the 240us window even if it doesn't cause preemption. critical_enter() in the above gives poor man's locking which blocks preemption but not-so-fast interrupt handlers. The race is not very important since this code only runs at boot time. The old splhigh() locking and a [spin] mutex around the loop lock out interrupts and/or preemption for too long (0-1 seconds), but again this is not very important since this code only runs at boot time. This is fixed in my version, but only for spl locking. The lock is acquired before each test in the loop and dropped after each failing test in the loop. Then operation is completed as far as necessary while the lock is still held. > @@ -369,6 +382,7 @@ atrtc_gettime(device_t dev, struct timespec *ts) > ct.year += (ct.year < 80 ? 2000 : 1900); > #endif > critical_exit(); > + mtx_unlock(&atrtc_time_lock); > /* Set dow = -1 because some clocks don't set it correctly. */ > ct.dow = -1; > return (clock_ct_to_ts(&ct, ts)); The new mtx_lock() should be placed next to the old critical_enter() too. Put these before or after the loop depending on whether races or high latency are preferred. Bruce