d/freebsd-nq
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Less-quick fix for locking fixes in r172250. r172250 added a second

Browse Source 
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.
This commit is contained in:
Bruce Evans 2016-08-25 13:46:52 +00:00
parent 4b7b743c16
commit d350ce61cf
3 changed files with 13 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
sys/dev/syscons/syscons.c
View File

@ -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 */

10
sys/dev/syscons/syscons.h
View File

@ -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)

3
sys/kern/subr_witness.c
View File

@ -637,7 +637,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
@ -653,6 +652,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 },
@ -661,7 +661,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