Date: Mon, 30 Apr 2007 15:27:26 +0400 From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: Nate Lawson <nate@root.org> Cc: freebsd-current@freebsd.org, rwatson@freebsd.org, jhb@freebsd.org, pf@freebsd.org Subject: Re: call for testers: altq in current Message-ID: <20070430112726.GA71812@codelabs.ru> In-Reply-To: <20070413212742.GH49158@codelabs.ru> References: <4617D3A6.8000201@root.org> <20070409094010.GL26348@codelabs.ru> <461FDD28.6030502@root.org> <20070413204237.GG49158@codelabs.ru> <461FEE6D.4030201@root.org> <20070413212742.GH49158@codelabs.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
--zYM0uCDKw75PZbzx
Content-Type: text/plain; charset=koi8-r
Content-Disposition: inline
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
--zYM0uCDKw75PZbzx
Content-Type: text/plain; charset=koi8-r
Content-Disposition: attachment; filename="altq-fix.diff"
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
--zYM0uCDKw75PZbzx--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070430112726.GA71812>