From owner-freebsd-pf@FreeBSD.ORG Wed Jun 20 19:04:30 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 423CC16A469 for ; Wed, 20 Jun 2007 19:04:30 +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 C72E113C447 for ; Wed, 20 Jun 2007 19:04:29 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) DomainKey-Signature: a=rsa-sha1; q=dns; c=simple; s=one; d=codelabs.ru; h=Received:Date:From:To:Cc:Message-ID:References:MIME-Version:Content-Type:Content-Disposition:In-Reply-To:Sender:X-Spam-Status:Subject; b=kCm9zH5j6nsyyfM1zsuul84H7aFfQIv0LOxCMo8yEnV2B60SRX5SIaQj7dE2o5knENoL2tkM+1sMzi+asRVgUqPPAUZdDctI9Wh5S3KtgWA2Sp3L9655JFloeuevsNEzAd6njkTHSvhqU4naihyJResbeotSGRHFvxBdfZirZ2E=; Received: from void.codelabs.ru (void.codelabs.ru [144.206.177.25]) by pobox.codelabs.ru with esmtpsa (TLSv1:AES256-SHA:256) id 1I15Tg-0007hV-9f; Wed, 20 Jun 2007 23:04:29 +0400 Date: Wed, 20 Jun 2007 23:04:23 +0400 From: Eygene Ryabinkin To: Nate Lawson , Max Laier Message-ID: <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> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="ew6BAiZeqk4r7MaW" Content-Disposition: inline In-Reply-To: <20070620152609.GD26920@void.codelabs.ru> Sender: rea-fbsd@codelabs.ru X-Spam-Status: No, score=-2.9 required=4.0 tests=ALL_TRUSTED,AWL,BAYES_00 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: Wed, 20 Jun 2007 19:04:30 -0000 --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--