call for testers: altq in current

[Eygene Ryabinkin]

Nate, good day.

Sat, Apr 14, 2007 at 01:27:42AM +0400, Eygene Ryabinkin wrote:
> I am just using the defaults for the -CURRENT. Can not verify
> them now -- my -CURRENT is crashing with the modem link, so
> I am either writing mails or doing the tests, sorry.

OK, I had cured the modem crash and coincidently it was related to
your changes: the problem was in the altq_subr.c. The behaviour of
the calls to machclk_init() was "check if we have machclk_freq ==
0 and invoke the machclk_init()". And the first action of machclk_init()
was to initialise the tbr_callout and then the machclk_freq was set
(or not, but the tbr_callout was initialized in any way).

But your change introduced another way for the 'machclk_freq' to
be set. And the bad thing was that the tbr_callout was not initialized
and will never be: your hook set the machclk_freq to some value and
machclk_init() was never called. So it gave me the uninitialized
callout with the wrong (NULL) c_mtx and bad flags. And when the
NULL mutex was freed in the softclock() the kernel panic'ed. There
were message in -current from me about this (subject started with
'mtx_unlock(NULL)' posted at 21st Apr 2007). I just did the very
rough and incorrect patch and John Baldwin kindly pointed me that
it was incorrect.

The patch that fixes the root of the problem is attached: it just
decouples the callout initialization from machclk_freq initialization.

I am CC'ing this to John Baldwin and Robert Watson, because they
were involved into the discuission about my previous wrong fix.  I
still have a question: maybe the initialization of the tbr_callout
in my patch should be protected with some mutex? I don't feel that
it is the case, because for the current code is seems to be unrelevant:
the worst thing will be the double initialization of the callout,
but maybe the mutex will be good for the future. Any ideas?

> > On the new code but without loading cpufreq and leaving the freq at 2200
> > Mhz, do you get the right numbers?  Are they constant?
> 
> Monday will reveal the things. Will post an update.

Was not able to test the things on Monday. But will try to do it
on this week. Sorry for it: many other tasks waited my attention
:(( Maybe the weird speeds were related to the uninitialized
tbr_callout(), though I am not sure.
-- 
Eygene
-------------- next part --------------
diff --git a/sys/contrib/altq/altq/altq_hfsc.c b/sys/contrib/altq/altq/altq_hfsc.c
diff --git a/sys/contrib/altq/altq/altq_subr.c b/sys/contrib/altq/altq/altq_subr.c
index 7426e75..0c6e485 100644
--- a/sys/contrib/altq/altq/altq_subr.c
+++ b/sys/contrib/altq/altq/altq_subr.c
@@ -383,6 +383,7 @@ tbr_set(ifq, profile)
 	if (tbr_dequeue_ptr == NULL)
 		tbr_dequeue_ptr = tbr_dequeue;
 
+	tbr_callout_init();
 	if (machclk_freq == 0)
 		init_machclk();
 	if (machclk_freq == 0) {
@@ -917,13 +918,26 @@ EVENTHANDLER_DEFINE(cpufreq_post_change, tsc_freq_changed, NULL,
     EVENTHANDLER_PRI_ANY);
 #endif /* __FreeBSD_version >= 700035 */
 
+#if (__FreeBSD_version >= 600000)
+/*
+ * Initializes the callout. OBVIOUS: should be called before the
+ * first use of the tbr_callout.
+ */
 void
-init_machclk(void)
+tbr_callout_init(void)
 {
-#if (__FreeBSD_version >= 600000)
-	callout_init(&tbr_callout, 0);
-#endif
+	static int called = 0;
 
+	if (!called) {
+		callout_init(&tbr_callout, 0);
+		called = 1;
+	}
+}
+#endif /* __FreeBSD_version >= 600000 */
+
+void
+init_machclk(void)
+{
 	machclk_usepcc = 1;
 
 #if (!defined(__i386__) && !defined(__alpha__)) || defined(ALTQ_NOPCC)
diff --git a/sys/contrib/altq/altq/altq_var.h b/sys/contrib/altq/altq/altq_var.h
index 99407fb..3879a28 100644
--- a/sys/contrib/altq/altq/altq_var.h
+++ b/sys/contrib/altq/altq/altq_var.h
@@ -125,6 +125,11 @@ extern void init_machclk(void);
 extern u_int64_t read_machclk(void);
 
 /*
+ * Callout initializer.
+ */
+extern void tbr_callout_init(void);
+
+/*
  * debug support
  */
 #ifdef ALTQ_DEBUG