From owner-freebsd-pf@FreeBSD.ORG Wed May 2 11:42:07 2007 Return-Path: X-Original-To: pf@freebsd.org Delivered-To: freebsd-pf@FreeBSD.ORG Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 99ED116A52C; Wed, 2 May 2007 11:42:07 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) Received: from pobox.codelabs.ru (pobox.codelabs.ru [144.206.177.45]) by mx1.freebsd.org (Postfix) with ESMTP id 4158613C458; Wed, 2 May 2007 11:42:07 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) DomainKey-Signature: a=rsa-sha1; q=dns; c=simple; s=one; d=codelabs.ru; h=Received:Resent-From:Resent-Date:Resent-Message-ID:Resent-To:Date:From:To:Cc:Message-ID:References:MIME-Version:Content-Type:Content-Disposition:In-Reply-To:Resent-Sender:Resent-Date:X-Spam-Status:Subject; b=mN+8Ahyatys3DT+g+mqT3KXjp/cyq+A5OFiQpWhcNnX+STEHHObLavbkJVGvaEsZOJunShhac1mZP8GeB0ZTKKZ4zZZh2br4qafhbaOhhY2+R+6EuIku4wIKAOziTcu44y/5DO/7XVm3abIyO1GAUJ0Bb1GTjs/EGp83kwyza4M=; Received: from codelabs.ru (pobox.codelabs.ru [144.206.177.45]) by pobox.codelabs.ru with esmtpsa (TLSv1:AES256-SHA:256) id 1HjDDh-00049T-6h; Wed, 02 May 2007 15:42:05 +0400 Resent-From: rea-fbsd@codelabs.ru Resent-Date: Wed, 2 May 2007 15:42:00 +0400 Resent-Message-ID: <20070502114200.GO7358@codelabs.ru> Resent-To: freebsd-current@freebsd.org, max@love2party.net, pf@freebsd.org Date: Mon, 30 Apr 2007 15:27:26 +0400 From: Eygene Ryabinkin To: Nate Lawson Message-ID: <20070430112726.GA71812@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> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="zYM0uCDKw75PZbzx" Content-Disposition: inline In-Reply-To: <20070413212742.GH49158@codelabs.ru> Resent-Sender: rea-fbsd@codelabs.ru Resent-Date: Wed, 02 May 2007 15:42:05 +0400 X-Spam-Status: No, score=-3.4 required=4.0 tests=ALL_TRUSTED,AWL,BAYES_00 Cc: freebsd-current@freebsd.org, rwatson@freebsd.org, jhb@freebsd.org, pf@freebsd.org Subject: Re: call for testers: altq in current X-BeenThere: freebsd-pf@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Technical discussion and general questions about packet filter \(pf\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 May 2007 11:42:07 -0000 --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--