Re: Eventtimers b0rking w/ math/atlas

On Fri, Sep 10, 2010 at 04:31:40PM +0200, Dimitry Andric wrote:
> On 2010-09-10 15:50, Thomas E. Spanjaard wrote:
> >While trying to build math/atlas on a FreeBSD/amd64 9.0-CURRENT r212411,
> >the kernel hangs at some point when math/atlas tries to run some tests
> >(presumably the ones to profile the code and optimise). The kernel
> >spouts messages about Starting event timers: LAPIC _at_ 1000Hz, HPET _at_
> >127Hz; then LAPIC _at_ 1000Hz, HPET _at_ 8128Hz (iirc), back, then back again.
> 
> It's probably running profiled executables (e.g. compiled with -pg)?
> This can lead to the above messages, and apparently also to deadlocks.
> 
> I am not sure what to do about it though, except confirm that this is
> very likely your problem...

Try this.

diff --git a/sys/kern/kern_clocksource.c b/sys/kern/kern_clocksource.c
index 6b005de..7f0c781 100644
--- a/sys/kern/kern_clocksource.c
+++ b/sys/kern/kern_clocksource.c
_at__at_ -65,6 +65,7 _at__at_ inline static int	doconfigtimer(int i);
 static void		configtimer(int i);
 
 static struct eventtimer *timer[2] = { NULL, NULL };
+static int		blocked_timer[2];
 static int		timertest = 0;
 static int		timerticks[2] = { 0, 0 };
 static int		profiling_on = 0;
_at__at_ -91,6 +92,22 _at__at_ static DPCPU_DEFINE(tc, configtimer);
 	(((uint64_t)0x8000000000000000 + ((bt)->frac >> 2)) /		\
 	    ((bt)->frac >> 1))
 
+static void
+block_timer(int i)
+{
+
+	critical_enter();
+	atomic_add_acq_int(&blocked_timer[i], 1);
+}
+
+static void
+unblock_timer(int i)
+{
+
+	atomic_add_rel_int(&blocked_timer[i], -1);
+	critical_exit();
+}
+
 /* Per-CPU timer1 handler. */
 static int
 hardclockhandler(struct trapframe *frame)
_at__at_ -246,6 +263,8 _at__at_ doconfigtimer(int i)
 		atomic_store_rel_int(*conf + i, 0);
 		return (1);
 	}
+	if (atomic_load_acq_int(&blocked_timer[i]) > 0)
+		return (1);
 	return (0);
 }
 
_at__at_ -366,9 +385,7 _at__at_ cpu_initclocks_bsp(void)
 		profhz = round_freq(timer[1], stathz * 64);
 	}
 	tick = 1000000 / hz;
-	ET_LOCK();
 	cpu_restartclocks();
-	ET_UNLOCK();
 }
 
 /* Start per-CPU event timers on APs. */
_at__at_ -376,12 +393,10 _at__at_ void
 cpu_initclocks_ap(void)
 {
 
-	ET_LOCK();
 	if (timer[0]->et_flags & ET_FLAGS_PERCPU)
 		et_start(timer[0], NULL, &timerperiod[0]);
 	if (timer[1] && timer[1]->et_flags & ET_FLAGS_PERCPU)
 		et_start(timer[1], NULL, &timerperiod[1]);
-	ET_UNLOCK();
 }
 
 /* Reconfigure and restart event timers after configuration changes. */
_at__at_ -411,9 +426,11 _at__at_ cpu_restartclocks(void)
 		timer2hz = round_freq(timer[1], timer2hz);
 	}
 	timer1hz = round_freq(timer[0], timer1hz);
+#ifdef notyet
 	printf("Starting kernel event timers: %s _at_ %dHz, %s _at_ %dHz\n",
 	    timer[0]->et_name, timer1hz,
 	    timer[1] ? timer[1]->et_name : "NONE", timer2hz);
+#endif
 	/* Restart event timers. */
 	FREQ2BT(timer1hz, &timerperiod[0]);
 	configtimer(0);
_at__at_ -422,7 +439,9 _at__at_ cpu_restartclocks(void)
 		timerticks[1] = 0;
 		FREQ2BT(timer2hz, &timerperiod[1]);
 		configtimer(1);
+#ifdef notyet
 		timertest = 1;
+#endif
 	}
 }
 
_at__at_ -433,7 +452,11 _at__at_ cpu_startprofclock(void)
 
 	ET_LOCK();
 	profiling_on = 1;
+	block_timer(0);
+	block_timer(1);
 	cpu_restartclocks();
+	unblock_timer(1);
+	unblock_timer(0);
 	ET_UNLOCK();
 }
 
_at__at_ -444,7 +467,11 _at__at_ cpu_stopprofclock(void)
 
 	ET_LOCK();
 	profiling_on = 0;
+	block_timer(0);
+	block_timer(1);
 	cpu_restartclocks();
+	unblock_timer(1);
+	unblock_timer(0);
 	ET_UNLOCK();
 }
 
_at__at_ -461,10 +488,11 _at__at_ sysctl_kern_eventtimer_timer1(SYSCTL_HANDLER_ARGS)
 	snprintf(buf, sizeof(buf), "%s", et->et_name);
 	ET_UNLOCK();
 	error = sysctl_handle_string(oidp, buf, sizeof(buf), req);
+	if (error != 0 || req->newptr == NULL)
+		return (error);
 	ET_LOCK();
 	et = timer[0];
-	if (error != 0 || req->newptr == NULL ||
-	    strcmp(buf, et->et_name) == 0) {
+	if (strcmp(buf, et->et_name) == 0) {
 		ET_UNLOCK();
 		return (error);
 	}
_at__at_ -473,12 +501,14 _at__at_ sysctl_kern_eventtimer_timer1(SYSCTL_HANDLER_ARGS)
 		ET_UNLOCK();
 		return (ENOENT);
 	}
+	block_timer(0);
 	timer1hz = 0;
 	configtimer(0);
 	et_free(timer[0]);
 	timer[0] = et;
 	et_init(timer[0], timer1cb, NULL, NULL);
 	cpu_restartclocks();
+	unblock_timer(0);
 	ET_UNLOCK();
 	return (error);
 }
_at__at_ -501,10 +531,11 _at__at_ sysctl_kern_eventtimer_timer2(SYSCTL_HANDLER_ARGS)
 		snprintf(buf, sizeof(buf), "%s", et->et_name);
 	ET_UNLOCK();
 	error = sysctl_handle_string(oidp, buf, sizeof(buf), req);
+	if (error != 0 || req->newptr == NULL)
+		return (error);
 	ET_LOCK();
 	et = timer[1];
-	if (error != 0 || req->newptr == NULL ||
-	    strcmp(buf, et ? et->et_name : "NONE") == 0) {
+	if (strcmp(buf, et ? et->et_name : "NONE") == 0) {
 		ET_UNLOCK();
 		return (error);
 	}
_at__at_ -513,6 +544,7 _at__at_ sysctl_kern_eventtimer_timer2(SYSCTL_HANDLER_ARGS)
 		ET_UNLOCK();
 		return (ENOENT);
 	}
+	block_timer(1);
 	if (timer[1] != NULL) {
 		timer2hz = 0;
 		configtimer(1);
_at__at_ -522,6 +554,7 _at__at_ sysctl_kern_eventtimer_timer2(SYSCTL_HANDLER_ARGS)
 	if (timer[1] != NULL)
 		et_init(timer[1], timer2cb, NULL, NULL);
 	cpu_restartclocks();
+	unblock_timer(1);
 	ET_UNLOCK();
 	return (error);
 }
diff --git a/sys/kern/kern_et.c b/sys/kern/kern_et.c
index 6a56b1d..94e1466 100644
--- a/sys/kern/kern_et.c
+++ b/sys/kern/kern_et.c
_at__at_ -38,7 +38,7 _at__at_ SLIST_HEAD(et_eventtimers_list, eventtimer);
 static struct et_eventtimers_list eventtimers = SLIST_HEAD_INITIALIZER(et_eventtimers);
 
 struct mtx	et_eventtimers_mtx;
-MTX_SYSINIT(et_eventtimers_init, &et_eventtimers_mtx, "et_mtx", MTX_SPIN);
+MTX_SYSINIT(et_eventtimers_init, &et_eventtimers_mtx, "et_mtx", MTX_DEF);
 
 SYSCTL_NODE(_kern, OID_AUTO, eventtimer, CTLFLAG_RW, 0, "Event timers");
 SYSCTL_NODE(_kern_eventtimer, OID_AUTO, et, CTLFLAG_RW, 0, "");
diff --git a/sys/sys/timeet.h b/sys/sys/timeet.h
index bc713d6..87392a2 100644
--- a/sys/sys/timeet.h
+++ b/sys/sys/timeet.h
_at__at_ -83,8 +83,8 _at__at_ struct eventtimer {
 };
 
 extern struct mtx	et_eventtimers_mtx;
-#define	ET_LOCK()	mtx_lock_spin(&et_eventtimers_mtx)
-#define	ET_UNLOCK()	mtx_unlock_spin(&et_eventtimers_mtx)
+#define	ET_LOCK()	mtx_lock(&et_eventtimers_mtx)
+#define	ET_UNLOCK()	mtx_unlock(&et_eventtimers_mtx)
 
 /* Driver API */
 int	et_register(struct eventtimer *et);
diff --git a/sys/x86/x86/local_apic.c b/sys/x86/x86/local_apic.c
index f479bbe..7811441 100644
--- a/sys/x86/x86/local_apic.c
+++ b/sys/x86/x86/local_apic.c
_at__at_ -500,9 +500,11 _at__at_ lapic_et_start(struct eventtimer *et,
 		} while (lapic_timer_divisor <= 128);
 		if (lapic_timer_divisor > 128)
 			panic("lapic: Divisor too big");
+#ifdef notyet
 		if (bootverbose)
 			printf("lapic: Divisor %lu, Frequency %lu Hz\n",
 			    lapic_timer_divisor, value);
+#endif
 		et->et_frequency = value;
 		et->et_min_period.sec = 0;
 		et->et_min_period.frac =