Date: Wed, 20 Jun 2007 23:04:23 +0400 From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: Nate Lawson <nate@root.org>, Max Laier <max@love2party.net> Cc: freebsd-pf@freebsd.org Subject: Re: pf 4.1 Update available for testing Message-ID: <20070620190423.GH26920@void.codelabs.ru> In-Reply-To: <20070620152609.GD26920@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>
next in thread | previous in thread | raw e-mail | index | archive | help
--ew6BAiZeqk4r7MaW Content-Type: text/plain; charset=koi8-r Content-Disposition: inline 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! -- Eygene --ew6BAiZeqk4r7MaW Content-Type: text/plain; charset=koi8-r Content-Disposition: attachment; filename="altq-fix-2.diff" diff --git a/sys/contrib/altq/altq/altq_subr.c b/sys/contrib/altq/altq/altq_subr.c index 0c6e485..0ca01db 100644 --- a/sys/contrib/altq/altq/altq_subr.c +++ b/sys/contrib/altq/altq/altq_subr.c @@ -129,6 +129,28 @@ static struct ip4_frag *ip4f_alloc(void); static void ip4f_free(struct ip4_frag *); #endif /* ALTQ3_CLFIER_COMPAT */ +static inline void +machclk_set_freq(u_int32_t _newfreq); +static inline void +machclk_init_pcc(void); +/* + * We do machclk_init_pcc() here because machclk_freq can be + * already initialized by tsc_freq_changed() or simular, but + * machclk_usepcc will not be properly initialized then. + * We could call machclk_init_pcc() from the tsc_freq_changed(), + * but the then calls to the machclk_init_pcc() will be more + * frequent. It is microoptimisation and it can be dropped + * in favor of the more clean code. + */ +#define INIT_MACHCLK \ + do { \ + if (machclk_freq == 0) \ + init_machclk(); \ + else \ + machclk_init_pcc(); \ + } while(0) + + /* * alternate queueing support routines */ @@ -384,8 +406,7 @@ tbr_set(ifq, profile) tbr_dequeue_ptr = tbr_dequeue; tbr_callout_init(); - if (machclk_freq == 0) - init_machclk(); + INIT_MACHCLK; if (machclk_freq == 0) { printf("tbr_set: no cpu clock available!\n"); return (ENXIO); @@ -599,8 +620,7 @@ altq_add(struct pf_altq *a) if (a->qname[0] != 0) return (altq_add_queue(a)); - if (machclk_freq == 0) - init_machclk(); + INIT_MACHCLK; if (machclk_freq == 0) panic("altq_add: no cpu clock"); @@ -912,7 +932,7 @@ tsc_freq_changed(void *arg, const struct cf_level *level, int status) return; /* Total setting for this level gives the new frequency in MHz. */ - machclk_freq = level->total_set.freq * 1000000; + machclk_set_freq(level->total_set.freq * 1000000); } EVENTHANDLER_DEFINE(cpufreq_post_change, tsc_freq_changed, NULL, EVENTHANDLER_PRI_ANY); @@ -935,9 +955,14 @@ tbr_callout_init(void) } #endif /* __FreeBSD_version >= 600000 */ -void -init_machclk(void) +static inline void +machclk_init_pcc(void) { + static int called = 0; + + if (called) + return; + machclk_usepcc = 1; #if (!defined(__i386__) && !defined(__alpha__)) || defined(ALTQ_NOPCC) @@ -955,11 +980,24 @@ init_machclk(void) tsc_is_broken)) machclk_usepcc = 0; #endif + called = 1; +} + +static inline void +machclk_set_freq(u_int32_t newfreq) +{ + machclk_freq = newfreq; + machclk_per_tick = machclk_freq / hz; +} + +void +init_machclk(void) +{ + machclk_init_pcc(); if (machclk_usepcc == 0) { /* emulate 256MHz using microtime() */ - machclk_freq = 1000000 << MACHCLK_SHIFT; - machclk_per_tick = machclk_freq / hz; + machclk_set_freq(1000000 << MACHCLK_SHIFT); #ifdef ALTQ_DEBUG printf("altq: emulate %uHz cpu clock\n", machclk_freq); #endif @@ -1011,7 +1049,7 @@ init_machclk(void) machclk_freq = (u_int)((end - start) * 1000000 / diff); } - machclk_per_tick = machclk_freq / hz; + machclk_set_freq(machclk_freq); #ifdef ALTQ_DEBUG printf("altq: CPU clock: %uHz\n", machclk_freq); --ew6BAiZeqk4r7MaW--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070620190423.GH26920>