Date: Thu, 13 Jul 2017 01:42:25 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ian Lepore <ian@freebsd.org> 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 Message-ID: <20170712224803.G1991@besplex.bde.org> In-Reply-To: <201707120242.v6C2gvDB026199@repo.freebsd.org> References: <201707120242.v6C2gvDB026199@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <machine/intr_machdep.h>
> #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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170712224803.G1991>