Date: Wed, 14 Mar 2018 03:09:48 +0000 (UTC) From: Eitan Adler <eadler@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: svn commit: r330896 - in stable/11/sys: dev/syscons kern Message-ID: <201803140309.w2E39mMD031368@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: eadler Date: Wed Mar 14 03:09:47 2018 New Revision: 330896 URL: https://svnweb.freebsd.org/changeset/base/330896 Log: MFC r304804: Less-quick fix for locking fixes in r172250. r172250 added a second syscons spinlock for the output routine alone. It is better to extend the coverage of the first syscons spinlock added in r162285. 2 locks might work with complicated juggling, but no juggling was done. What the 2 locks actually did was to cover some of the missing locking in each other and deadlock less often against each other than a single lock with larger coverage would against itself. Races are preferable to deadlocks here, but 2 locks are still worse since they are harder to understand and fix. Prefer deadlocks to races and merge the second lock into the first one. Extend the scope of the spinlocking to all of sc_cnputc() instead of just the sc_puts() part. This further prefers deadlocks to races. Extend the kdb_active hack from sc_puts() internals for the second lock to all spinlocking. This reduces deadlocks much more than the other changes increases them. The s/p,10* test in ddb gets much further now. Hide this detail in the SC_VIDEO_LOCK() macro. Add namespace pollution in 1 nested #include and reduce namespace pollution in other nested #includes to pay for this. Move the first lock higher in the witness order. The second lock was unnaturally low and the first lock was unnaturally high. The second lock had to be above "sleepq chain" and/or "callout" to avoid spurious LORs for visual bells in sc_puts(). Other console driver locks are already even higher (but not adjacent like they should be) except when they are missing from the table. Audio bells also benefit from the syscons lock being high so that audio mutexes have chance of being lower. Otherwise, console drviver locks should be as low as possible. Non-spurious LORs now occur if the bell code calls printf() or is interrupted (perhaps by an NMI) and the interrupt handler calls printf(). Previous commits turned off many bells in console i/o but missed ones done by the teken layer. Modified: stable/11/sys/dev/syscons/syscons.c stable/11/sys/dev/syscons/syscons.h stable/11/sys/kern/subr_witness.c Directory Properties: stable/11/ (props changed) Modified: stable/11/sys/dev/syscons/syscons.c ============================================================================== --- stable/11/sys/dev/syscons/syscons.c Wed Mar 14 03:04:24 2018 (r330895) +++ stable/11/sys/dev/syscons/syscons.c Wed Mar 14 03:09:47 2018 (r330896) @@ -343,7 +343,9 @@ sctty_outwakeup(struct tty *tp) len = ttydisc_getc(tp, buf, sizeof buf); if (len == 0) break; + SC_VIDEO_LOCK(scp->sc); sc_puts(scp, buf, len, 0); + SC_VIDEO_UNLOCK(scp->sc); } } @@ -1760,6 +1762,8 @@ sc_cnputc(struct consdev *cd, int c) /* assert(sc_console != NULL) */ + SC_VIDEO_LOCK(scp->sc); + #ifndef SC_NO_HISTORY if (scp == scp->sc->cur_scp && scp->status & SLKED) { scp->status &= ~SLKED; @@ -1793,6 +1797,7 @@ sc_cnputc(struct consdev *cd, int c) s = spltty(); /* block sckbdevent and scrn_timer */ sccnupdate(scp); splx(s); + SC_VIDEO_UNLOCK(scp->sc); } static int @@ -2726,24 +2731,14 @@ exchange_scr(sc_softc_t *sc) static void sc_puts(scr_stat *scp, u_char *buf, int len, int kernel) { - int need_unlock = 0; - #ifdef DEV_SPLASH /* make screensaver happy */ if (!sticky_splash && scp == scp->sc->cur_scp && !sc_saver_keyb_only) run_scrn_saver = FALSE; #endif - if (scp->tsw) { - if (!kdb_active && !mtx_owned(&scp->sc->scr_lock)) { - need_unlock = 1; - mtx_lock_spin(&scp->sc->scr_lock); - } + if (scp->tsw) (*scp->tsw->te_puts)(scp, buf, len, kernel); - if (need_unlock) - mtx_unlock_spin(&scp->sc->scr_lock); - } - if (scp->sc->delayed_next_scr) sc_switch_scr(scp->sc, scp->sc->delayed_next_scr - 1); } @@ -2906,10 +2901,8 @@ scinit(int unit, int flags) * disappeared... */ sc = sc_get_softc(unit, flags & SC_KERNEL_CONSOLE); - if ((sc->flags & SC_INIT_DONE) == 0) { - mtx_init(&sc->scr_lock, "scrlock", NULL, MTX_SPIN); + if ((sc->flags & SC_INIT_DONE) == 0) SC_VIDEO_LOCKINIT(sc); - } adp = NULL; if (sc->adapter >= 0) { @@ -3126,7 +3119,6 @@ scterm(int unit, int flags) (*scp->tsw->te_term)(scp, &scp->ts); if (scp->ts != NULL) free(scp->ts, M_DEVBUF); - mtx_destroy(&sc->scr_lock); mtx_destroy(&sc->video_mtx); /* clear the structure */ Modified: stable/11/sys/dev/syscons/syscons.h ============================================================================== --- stable/11/sys/dev/syscons/syscons.h Wed Mar 14 03:04:24 2018 (r330895) +++ stable/11/sys/dev/syscons/syscons.h Wed Mar 14 03:09:47 2018 (r330896) @@ -34,8 +34,9 @@ #ifndef _DEV_SYSCONS_SYSCONS_H_ #define _DEV_SYSCONS_SYSCONS_H_ -#include <sys/lock.h> -#include <sys/mutex.h> +#include <sys/kdb.h> /* XXX */ +#include <sys/_lock.h> +#include <sys/_mutex.h> /* machine-dependent part of the header */ @@ -239,7 +240,6 @@ typedef struct sc_softc { /* 2 is just enough for kdb to grab for stepping normal grabbing: */ struct sc_cnstate grab_state[2]; int kbd_open_level; - struct mtx scr_lock; /* mutex for sc_puts() */ struct mtx video_mtx; long scrn_time_stamp; @@ -547,12 +547,12 @@ typedef struct { MTX_SPIN | MTX_RECURSE); #define SC_VIDEO_LOCK(sc) \ do { \ - if (!cold) \ + if (!kdb_active) \ mtx_lock_spin(&(sc)->video_mtx); \ } while(0) #define SC_VIDEO_UNLOCK(sc) \ do { \ - if (!cold) \ + if (!kdb_active) \ mtx_unlock_spin(&(sc)->video_mtx); \ } while(0) Modified: stable/11/sys/kern/subr_witness.c ============================================================================== --- stable/11/sys/kern/subr_witness.c Wed Mar 14 03:04:24 2018 (r330895) +++ stable/11/sys/kern/subr_witness.c Wed Mar 14 03:09:47 2018 (r330896) @@ -645,7 +645,6 @@ static struct witness_order_list_entry order_lists[] = #endif { "rm.mutex_mtx", &lock_class_mtx_spin }, { "sio", &lock_class_mtx_spin }, - { "scrlock", &lock_class_mtx_spin }, #ifdef __i386__ { "cy", &lock_class_mtx_spin }, #endif @@ -661,6 +660,7 @@ static struct witness_order_list_entry order_lists[] = { "pmc-per-proc", &lock_class_mtx_spin }, #endif { "process slock", &lock_class_mtx_spin }, + { "syscons video lock", &lock_class_mtx_spin }, { "sleepq chain", &lock_class_mtx_spin }, { "rm_spinlock", &lock_class_mtx_spin }, { "turnstile chain", &lock_class_mtx_spin }, @@ -669,7 +669,6 @@ static struct witness_order_list_entry order_lists[] = { "td_contested", &lock_class_mtx_spin }, { "callout", &lock_class_mtx_spin }, { "entropy harvest mutex", &lock_class_mtx_spin }, - { "syscons video lock", &lock_class_mtx_spin }, #ifdef SMP { "smp rendezvous", &lock_class_mtx_spin }, #endif
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201803140309.w2E39mMD031368>