lkml.org 
[lkml]   [2001]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [Patch] Re: Nasty suprise with uptime
Date
On Wednesday 31 October 2001 20:47, Tim Schmielau wrote:
> btw.: can someone please explain to me why do_timer uses
> (*(unsigned long *)&jiffies)++;
> instead of just doing jiffies++ ?

This casts away volatile -> gcc generates potentially faster code


> -unsigned long volatile jiffies;
> +unsigned long volatile jiffies = INITIAL_JIFFIES;
> +unsigned long volatile jiffies_hi, jiffies_hi_shadow;

Grepped your patch for uses of jiffies_hi_shadow, found none
except for stores. Seems to be unused?


> void do_timer(struct pt_regs *regs)
> {
> - (*(unsigned long *)&jiffies)++;
> + /* we assume that two calls to do_timer can never overlap
> + * since they are one jiffie apart in time */
> + if (jiffies != (unsigned long)(-1)) {
> + jiffies++;
> + } else {
> + /* We still need to care about the race with readers of
> + * jiffies_hi. Readers have to discard the values if
> + * jiffies_hi != jiffies_hi_shadow when read with
> + * proper barriers in between. */
> + jiffies_hi++;
> + barrier();
> + jiffies++;
> + barrier();
> + jiffies_hi_shadow = jiffies_hi;
> + barrier();
> + }

Looks like gross overkill for 64bit i++. I see no flaw in much simpler:
+ if(++jiffies == 0) {
+ /* barrier(); --vda: needed? I have some doubts... */
+ jiffies_hi++;
+ }

To avoid races 64bit readers must read in reverse order: hi,lo,check hi;
your patch already does this just right:

> +static inline u64 get_jiffies64() {
> + unsigned long hi,lo;
> + /* We need to make sure jiffies_hi does not change while
> + * reading jiffies and jiffies_hi */
> + do {
> + hi = jiffies_hi;
> + barrier();
> + lo = jiffies;
> + barrier();
> + } while (hi != jiffies_hi);
> + return lo + (((u64)hi) << BITS_PER_LONG);
> +}
> +


> +#define INITIAL_JIFFIES 0xFFFFD000ul
> +extern unsigned long volatile jiffies, jiffies_hi, jiffies_hi_shadow;

This belongs to some .h file, not .c (most likely include/linux/sched.h)

Also consider

+#if defined(CONFIG_DEBUG_KERNEL)
+/* Rollover in 1000 secs */
+#define INITIAL_JIFFIES ((unsigned long) -1000*HZ)
+#else
+#define INITIAL_JIFFIES 0
+#endif


Hope your patch will go into mainline soon!
--
vda
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:12    [W:0.072 / U:2.388 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site