Date: Tue, 03 Jul 2007 15:24:17 -0700 From: Nate Lawson <nate@root.org> To: Eygene Ryabinkin <rea-fbsd@codelabs.ru> Cc: freebsd-pf@freebsd.org Subject: Re: pf 4.1 Update available for testing Message-ID: <468ACC91.9010806@root.org> In-Reply-To: <20070620190423.GH26920@void.codelabs.ru> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format. --------------060406020103030708050005 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Eygene Ryabinkin wrote: > Nate, Max, good day. > > Wed, Jun 20, 2007 at 07:26:09PM +0400, Eygene Ryabinkin wrote: >> Fine, thanks! So, you're happy with the way the problem was fixed? >> I see that another function that uses tbr_callout is tbr_timeout, >> but it will not be called before tbr_set. So it seems to me that >> callout initialisation only in tbr_set is enough. But maybe I am >> missing something? > > After some thinking I came to the idea that one more patch must be > applied. The variables machclk_usepcc and machclk_per_tick can be > left uninitialised following the same codepath as for tbr_callout: > tsc_freq_changed() touches only machclk_freq, but init_machclk > touches all three variables. > > This error can potentially be responsible to the weird bandwidth > values I am having with the altq on my notebook. The issue is > described on the thread > http://lists.freebsd.org/pipermail/freebsd-current/2007-April/070730.html > Basically, I am setting one BW limit in pf.conf and seeing another > one (much lower) via the ifstat utility. > > I was able only to test the compilation of the new patched kernel. > No bandwidth tests were done: I have no access to the fast LAN link > up to the Monday, 24th, sorry. May be I will be able to setup > ng_eiface and test with it, but I am not fluent with the netgraph. > Will post an update if tests will be carried. > > But I am pretty sure that the altq_subr.c should be patched to > properly handle the initialization of these two variables. The > only question is how to do it: via my patch or using some different > strategy. > > No more words, the patch is attached. Comments are welcome! > 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. Thanks, -- Nate --------------060406020103030708050005 Content-Type: text/x-patch; name="altq-fix-3.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="altq-fix-3.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 3 Jul 2007 22:15:05 -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,15 @@ return; /* Total setting for this level gives the new frequency in MHz. */ - machclk_freq = level->total_set.freq * 1000000; + tsc_freq = level->total_set.freq * 1000000; + init_machclk(); } EVENTHANDLER_DEFINE(cpufreq_post_change, tsc_freq_changed, NULL, EVENTHANDLER_PRI_ANY); #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 +942,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() */ --------------060406020103030708050005--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?468ACC91.9010806>