From owner-freebsd-pf@FreeBSD.ORG Thu Jul 12 00:58:44 2007 Return-Path: X-Original-To: freebsd-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 8C7B316A41F for ; Thu, 12 Jul 2007 00:58:44 +0000 (UTC) (envelope-from nate@root.org) Received: from root.org (root.org [67.118.192.226]) by mx1.freebsd.org (Postfix) with ESMTP id 6B1B613C45A for ; Thu, 12 Jul 2007 00:58:44 +0000 (UTC) (envelope-from nate@root.org) Received: (qmail 88182 invoked from network); 12 Jul 2007 00:58:45 -0000 Received: from ppp-71-139-42-13.dsl.snfc21.pacbell.net (HELO ?10.0.5.18?) (nate-mail@71.139.42.13) by root.org with ESMTPA; 12 Jul 2007 00:58:45 -0000 Message-ID: <46957CB9.1020801@root.org> Date: Wed, 11 Jul 2007 17:58:33 -0700 From: Nate Lawson User-Agent: Thunderbird 2.0.0.4 (X11/20070617) MIME-Version: 1.0 To: Eygene Ryabinkin References: <200706160347.33331.max@love2party.net> <20070617094126.GT3779@void.codelabs.ru> <200706171717.21585.max@love2party.net> <20070619074150.GC26920@void.codelabs.ru> <4677FF00.4060506@root.org> <20070620152609.GD26920@void.codelabs.ru> <20070620190423.GH26920@void.codelabs.ru> <468ACC91.9010806@root.org> <20070710061950.GB38151@void.codelabs.ru> In-Reply-To: <20070710061950.GB38151@void.codelabs.ru> X-Enigmail-Version: 0.95.0 Content-Type: multipart/mixed; boundary="------------050403040607040200090901" Cc: freebsd-pf@freebsd.org Subject: Re: pf 4.1 Update available for testing 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: Thu, 12 Jul 2007 00:58:44 -0000 This is a multi-part message in MIME format. --------------050403040607040200090901 Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Eygene Ryabinkin wrote: > Nate, good day. > > Tue, Jul 03, 2007 at 03:24:17PM -0700, Nate Lawson wrote: >> I have tried to achieve the same goal with a simpler patch. Here are >> the changes: >> >> Be sure to initialize the callout struct and other setup tasks before >> proceeding. Previously, machclk_freq could be set to a non-zero value >> by tsc_freq_changed(), preventing the callout from being initialized. >> To fix this, call init_machclk() from all paths. init_machclk() is >> split into two functions, one that only runs the first time it is >> called. The second half runs each time the frequency changes and >> calibrates various items. Also, static variables are zero so no need to >> initialize them. >> >> If you can test this, that would be great. > > Yes, it seems to work. I am a little concerned with the dependency > your patch introduces: it assumes that init_machclk() will always > use tsc_freq as the frequency source. So one day when another > counter will appear one will need to locate all respective references > to the tsc_freq and change them accordingly. That was the reason > for my lengthy changes: the explicit API. May be the simple comment > around init_machclk() will be enough, but explicit parameter will > be better. I will try to think how it can be done with your patch, > but if any of you have some thoughts why it shouldn't be done this > way or have some other ideas -- I am all ears. > > Thank you! Thanks for your testing. I've submitted the attached patch to re@ to go into 7-current. It no longer explicitly references tsc_freq in the eventhandler. It allows the existing eventhandler (sys/i386/i386/tsc.c and amd64) to set tsc_freq. Its priority is _PRI_LAST so tsc_freq is already modified. That way all we have to do for future time sources is make sure they update themselves and then ALTQ only has to be changed in init_machclk(). Once I commit it, please cvsup and test. I'll send you notice. -Nate --------------050403040607040200090901 Content-Type: text/x-patch; name="altq-fix-4.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="altq-fix-4.diff" Index: altq_subr.c =================================================================== RCS file: /home/ncvs/src/sys/contrib/altq/altq/altq_subr.c,v retrieving revision 1.9 diff -u -r1.9 altq_subr.c --- altq_subr.c 26 Mar 2007 18:03:29 -0000 1.9 +++ altq_subr.c 12 Jul 2007 00:52:19 -0000 @@ -887,8 +887,8 @@ #define MACHCLK_SHIFT 8 int machclk_usepcc; -u_int32_t machclk_freq = 0; -u_int32_t machclk_per_tick = 0; +u_int32_t machclk_freq; +u_int32_t machclk_per_tick; #ifdef __alpha__ #ifdef __FreeBSD__ @@ -911,14 +911,14 @@ return; /* Total setting for this level gives the new frequency in MHz. */ - machclk_freq = level->total_set.freq * 1000000; + init_machclk(); } EVENTHANDLER_DEFINE(cpufreq_post_change, tsc_freq_changed, NULL, - EVENTHANDLER_PRI_ANY); + EVENTHANDLER_PRI_LAST); #endif /* __FreeBSD_version >= 700035 */ -void -init_machclk(void) +static void +init_machclk_setup(void) { #if (__FreeBSD_version >= 600000) callout_init(&tbr_callout, 0); @@ -941,6 +941,18 @@ tsc_is_broken)) machclk_usepcc = 0; #endif +} + +void +init_machclk(void) +{ + static int called; + + /* Call one-time initialization function. */ + if (!called) { + init_machclk_setup(); + called = 1; + } if (machclk_usepcc == 0) { /* emulate 256MHz using microtime() */ --------------050403040607040200090901--