pf 4.1 Update available for testing

[Eygene Ryabinkin]

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
-------------- next part --------------
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);