Messages in this thread |  | | From | vda <> | Subject | Re: [Patch] Re: Nasty suprise with uptime | Date | Thu, 1 Nov 2001 16:09:08 +0000 |
| |
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/
|  |