tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Uninitialized var in altq_subr.c



hf%spg.tu-darmstadt.de@localhost wrote:

> compiling an altq enabled -current kernel gives me
> 
> #   compile  PIZZA_PF/altq_wfq.o
> /u/netbsd-builds/developer/sparc/tools/bin/sparc--netbsdelf-gcc -mno-fpu 
> -ffreestanding -fno-zero-initialized-in-bss -mcpu
> --- altq_subr.o ---
> cc1: warnings being treated as errors
> /public/netbsd-developer/sys/altq/altq_subr.c: In function 'read_machclk':
> /public/netbsd-developer/sys/altq/altq_subr.c:826: warning: 'val' may be 
> used uninitialized in this function
> 
> Looking at the source, gcc is right:
> 
> 
> 1.16         (peter    12-Oct-06): u_int64_t
> 1.16         (peter    12-Oct-06): read_machclk(void)
> 1.1          (thorpej  14-Dec-00): {
> 1.16         (peter    12-Oct-06):      u_int64_t val;
> 1.16         (peter    12-Oct-06):
> 1.16         (peter    12-Oct-06):      if (machclk_usepcc) {
> 1.25         (ad       10-May-08): #ifdef __HAVE_CPU_COUNTER
> 1.25         (ad       10-May-08):              return cpu_counter();
> 1.16         (peter    12-Oct-06): #endif
> 1.1          (thorpej  14-Dec-00):      } else {
> 1.16         (peter    12-Oct-06):              struct timeval tv;
> 1.16         (peter    12-Oct-06):
> 1.16         (peter    12-Oct-06):              microtime(&tv);
> 1.16         (peter    12-Oct-06):              val = 
> (((u_int64_t)(tv.tv_sec - boottime.tv_sec) * 1000000
> 1.16         (peter    12-Oct-06):                  + tv.tv_usec) << 
> MACHCLK_SHIFT);
> 1.1          (thorpej  14-Dec-00):      }
> 1.16         (peter    12-Oct-06):      return (val);
> 
> 
> I am tempted to slip in a 'val = 0', but am not sure that this is the 
> intended return value for 'if (machclk_usepcc)' and undefined 
> __HAVE_CPU_COUNTER?

I think the following diff is more straightforward:

---
Index: altq_subr.c
===================================================================
RCS file: /cvsroot/src/sys/altq/altq_subr.c,v
retrieving revision 1.25
diff -u -r1.25 altq_subr.c
--- altq_subr.c 10 May 2008 15:11:10 -0000      1.25
+++ altq_subr.c 21 Nov 2008 10:49:37 -0000
@@ -755,21 +755,18 @@
  *  - 64-bit-long monotonically-increasing counter
  *  - frequency range is 100M-4GHz (CPU speed)
  */
-/* if pcc is not available or disabled, emulate 256MHz using microtime() */
+/*
+ * if an MD cpu counter is not available or disabled,
+ * emulate 256MHz using microtime()
+ */
 #define        MACHCLK_SHIFT   8
 
-int machclk_usepcc;
+#ifdef __HAVE_CPU_COUNTER
+static int machclk_usecc;
+#endif
 u_int32_t machclk_freq = 0;
 u_int32_t machclk_per_tick = 0;
 
-#ifdef __alpha__
-#ifdef __FreeBSD__
-extern u_int32_t cycles_per_sec;       /* alpha cpu clock frequency */
-#elif defined(__NetBSD__) || defined(__OpenBSD__)
-extern u_int64_t cycles_per_usec;      /* alpha cpu clock frequency */
-#endif
-#endif /* __alpha__ */
-
 void
 init_machclk(void)
 {
@@ -778,11 +775,12 @@
 
 #ifdef __HAVE_CPU_COUNTER
        /* check if TSC is available */
-       machclk_usepcc = cpu_hascounter();
+       machclk_usecc = cpu_hascounter();
        machclk_freq = cpu_frequency(curcpu());
-#endif
 
-       if (machclk_usepcc == 0) {
+       if (machclk_usecc == 0)
+#endif
+       {
                /* emulate 256MHz using microtime() */
                machclk_freq = 1000000 << MACHCLK_SHIFT;
                machclk_per_tick = machclk_freq / hz;
@@ -792,6 +790,7 @@
                return;
        }
 
+#ifdef __HAVE_CPU_COUNTER
        /*
         * if we don't know the clock frequency, measure it.
         */
@@ -818,6 +817,7 @@
 #ifdef ALTQ_DEBUG
        printf("altq: CPU clock: %uHz\n", machclk_freq);
 #endif
+#endif
 }
 
 u_int64_t
@@ -825,11 +825,12 @@
 {
        u_int64_t val;
 
-       if (machclk_usepcc) {
 #ifdef __HAVE_CPU_COUNTER
+       if (machclk_usecc) {
                return cpu_counter();
+       } else
 #endif
-       } else {
+       {
                struct timeval tv;
 
                microtime(&tv);
Index: altq_var.h
===================================================================
RCS file: /cvsroot/src/sys/altq/altq_var.h,v
retrieving revision 1.11
diff -u -r1.11 altq_var.h
--- altq_var.h  9 Jul 2007 21:10:46 -0000       1.11
+++ altq_var.h  21 Nov 2008 10:49:37 -0000
@@ -118,7 +118,6 @@
  * machine dependent clock
  * a 64bit high resolution time counter.
  */
-extern int machclk_usepcc;
 extern u_int32_t machclk_freq;
 extern u_int32_t machclk_per_tick;
 extern void init_machclk(void);
---

On the other hand, current cpu_counter() code in the ALTQ doesn't
look MP safe because it uses cpu_counter() directly which is
independent among processors.

Maybe we should always use nanotime(9) (or nanouptime(9))
(as OpenBSD always uses microtime(9)) since all our ports have
had timecounter(9) support which provides accurate nanotime(9)
with a reasonable counter source.

---
Index: altq_subr.c
===================================================================
RCS file: /cvsroot/src/sys/altq/altq_subr.c,v
retrieving revision 1.25
diff -u -r1.25 altq_subr.c
--- altq_subr.c 10 May 2008 15:11:10 -0000      1.25
+++ altq_subr.c 21 Nov 2008 11:22:25 -0000
@@ -70,11 +70,6 @@
 #include <altq/altq_conf.h>
 #endif
 
-/* machine dependent clock related includes */
-#ifdef __HAVE_CPU_COUNTER
-#include <machine/cpu_counter.h>               /* for pentium tsc */
-#endif
-
 /*
  * internal function prototypes
  */
@@ -373,20 +368,6 @@
                CALLOUT_RESET(&tbr_callout, 1, tbr_timeout, (void *)0);
        else
                tbr_timer = 0;  /* don't need tbr_timer anymore */
-#if defined(__alpha__) && !defined(ALTQ_NOPCC)
-       {
-               /*
-                * XXX read out the machine dependent clock once a second
-                * to detect counter wrap-around.
-                */
-               static u_int cnt;
-
-               if (++cnt >= hz) {
-                       (void)read_machclk();
-                       cnt = 0;
-               }
-       }
-#endif /* __alpha__ && !ALTQ_NOPCC */
 }
 
 /*
@@ -747,95 +728,31 @@
        return;
 }
 
-
-/*
- * high resolution clock support taking advantage of a machine dependent
- * high resolution time counter (e.g., timestamp counter of intel pentium).
- * we assume
- *  - 64-bit-long monotonically-increasing counter
- *  - frequency range is 100M-4GHz (CPU speed)
- */
-/* if pcc is not available or disabled, emulate 256MHz using microtime() */
-#define        MACHCLK_SHIFT   8
-
-int machclk_usepcc;
 u_int32_t machclk_freq = 0;
 u_int32_t machclk_per_tick = 0;
 
-#ifdef __alpha__
-#ifdef __FreeBSD__
-extern u_int32_t cycles_per_sec;       /* alpha cpu clock frequency */
-#elif defined(__NetBSD__) || defined(__OpenBSD__)
-extern u_int64_t cycles_per_usec;      /* alpha cpu clock frequency */
-#endif
-#endif /* __alpha__ */
-
 void
 init_machclk(void)
 {
 
        callout_init(&tbr_callout, 0);
 
-#ifdef __HAVE_CPU_COUNTER
-       /* check if TSC is available */
-       machclk_usepcc = cpu_hascounter();
-       machclk_freq = cpu_frequency(curcpu());
-#endif
-
-       if (machclk_usepcc == 0) {
-               /* emulate 256MHz using microtime() */
-               machclk_freq = 1000000 << MACHCLK_SHIFT;
-               machclk_per_tick = machclk_freq / hz;
-#ifdef ALTQ_DEBUG
-               printf("altq: emulate %uHz CPU clock\n", machclk_freq);
-#endif
-               return;
-       }
-
-       /*
-        * if we don't know the clock frequency, measure it.
-        */
-       if (machclk_freq == 0) {
-               static int      wait;
-               struct timeval  tv_start, tv_end;
-               u_int64_t       start, end, diff;
-               int             timo;
-
-               microtime(&tv_start);
-               start = read_machclk();
-               timo = hz;      /* 1 sec */
-               (void)tsleep(&wait, PWAIT | PCATCH, "init_machclk", timo);
-               microtime(&tv_end);
-               end = read_machclk();
-               diff = (u_int64_t)(tv_end.tv_sec - tv_start.tv_sec) * 1000000
-                   + tv_end.tv_usec - tv_start.tv_usec;
-               if (diff != 0)
-                       machclk_freq = (u_int)((end - start) * 1000000 / diff);
-       }
-
+       /* use 1GHz provided by nanotime() */
+       machclk_freq = 1000000000;
        machclk_per_tick = machclk_freq / hz;
-
 #ifdef ALTQ_DEBUG
-       printf("altq: CPU clock: %uHz\n", machclk_freq);
+       printf("altq: emulate %uHz CPU clock\n", machclk_freq);
 #endif
 }
 
 u_int64_t
 read_machclk(void)
 {
+       struct timespec tsp;
        u_int64_t val;
 
-       if (machclk_usepcc) {
-#ifdef __HAVE_CPU_COUNTER
-               return cpu_counter();
-#endif
-       } else {
-               struct timeval tv;
-
-               microtime(&tv);
-               val = (((u_int64_t)(tv.tv_sec - boottime.tv_sec) * 1000000
-                   + tv.tv_usec) << MACHCLK_SHIFT);
-       }
+       getnanouptime(&tsp);
+       val = ((u_int64_t)tsp.tv_sec * 1000000000 + tsp.tv_nsec);
        return (val);
 }
 
Index: altq_var.h
===================================================================
RCS file: /cvsroot/src/sys/altq/altq_var.h,v
retrieving revision 1.11
diff -u -r1.11 altq_var.h
--- altq_var.h  9 Jul 2007 21:10:46 -0000       1.11
+++ altq_var.h  21 Nov 2008 11:22:25 -0000
@@ -118,7 +118,6 @@
  * machine dependent clock
  * a 64bit high resolution time counter.
  */
-extern int machclk_usepcc;
 extern u_int32_t machclk_freq;
 extern u_int32_t machclk_per_tick;
 extern void init_machclk(void);
---
Izumi Tsutsui


Home | Main Index | Thread Index | Old Index