jiffies_64 vs. jiffies

jiffies_64 vs. jiffies

[Atsushi Nemoto]

Hi.  I noticed that the 'jiffies' variable has 'wall_jiffies + 1'
value in most of time.  I'm using MIPS platform but I think this is
same for other platforms.

I suppose this is due to gcc does not know that jiffies_64 and jiffies
share same place.

In kernel/timer.c:

static inline void update_times(void)
{
	unsigned long ticks;

	ticks = jiffies - wall_jiffies;
	if (ticks) {
		wall_jiffies += ticks;
		update_wall_time(ticks);
	}
	calc_load(ticks);
}
  
void do_timer(struct pt_regs *regs)
{
	jiffies_64++;
	update_times();
	softlockup_tick(regs);
}

This is compiled MIPS code (gcc 3.4.5):

80056db4 <do_timer>:
...
80056de0:       lui     a3,0x8033
80056de4:       lw      v0,21400(a3)	# load jiffies_64(lo)
80056de8:       lui     a1,0x8033
80056dec:       lui     t1,0x8033
80056df0:       lw      a2,21400(a1)	# load jiffies
80056df4:       lw      v1,21404(a3)	# load jiffies_64(hi)
80056df8:       addiu   v0,v0,1		# inc jiffies_64(lo)
80056dfc:       lw      t0,21360(t1)	# load wall_jiffies
80056e00:       sltiu   a1,v0,1		# calc carry
80056e04:       addu    v1,v1,a1	# add carry to jiffies_64(hi)
80056e08:       subu    s4,a2,t0	# calc ticks (jiffies - wall_jiffies)
80056e0c:       sw      v0,21400(a3)	# store jiffies_64(lo)
80056e10:       sw      v1,21404(a3)	# store jiffies_64(hi)
80056e14:       beqz    s4,80057060 <do_timer+0x2ac>
80056e18:       move    s8,a0

The 'tick' variable is calculated using 'jiffies' value before
incrementing 'jiffies_64'.  As a result, wall_jiffies will always one
smaller then jiffies on elsewhere.

I also checked x86 code (3.4.4).

c012696a <do_timer>:
...
c012696e:       mov    0xc0482400,%eax	# load jiffies
c0126973:       addl   $0x1,0xc0482400	# inc jiffies_64(lo)
c012697a:       mov    0xc041a230,%edx	# load wall_jiffies
c0126980:       mov    %eax,%ebx
c0126982:       adcl   $0x0,0xc0482404	# add carry to jiffies_64(hi)
c0126989:       sub    %edx,%ebx	# calc ticks (jiffies - wall_jiffies)
c012698b:       jne    c01269a0 <do_timer+0x36>

Though I'm not familiar with x86, it looks same.

Is this really expected code?  If no, how it can be fixed?  Insert
"barrier()" right after "jiffies_64++" ?

---
Atsushi Nemoto

Re: jiffies_64 vs. jiffies

[Atsushi Nemoto]

>>>>> On Wed, 01 Mar 2006 14:44:42 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> said:
anemo> Hi.  I noticed that the 'jiffies' variable has 'wall_jiffies +
anemo> 1' value in most of time.  I'm using MIPS platform but I think
anemo> this is same for other platforms.

anemo> I suppose this is due to gcc does not know that jiffies_64 and
anemo> jiffies share same place.
...
anemo> Is this really expected code?  If no, how it can be fixed?
anemo> Insert "barrier()" right after "jiffies_64++" ?

I suppose passing updated jiffies to update_times() would be more
efficient than barrier().  Here is a patch.


Pass updated jiffies to update_times() to avoid jiffies/jiffies_64
aliasing.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

diff --git a/kernel/timer.c b/kernel/timer.c
index fe3a9a9..7734788 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -904,11 +904,11 @@ void run_local_timers(void)
  * Called by the timer interrupt. xtime_lock must already be taken
  * by the timer IRQ!
  */
-static inline void update_times(void)
+static inline void update_times(unsigned long jif)
 {
 	unsigned long ticks;
 
-	ticks = jiffies - wall_jiffies;
+	ticks = jif - wall_jiffies;
 	if (ticks) {
 		wall_jiffies += ticks;
 		update_wall_time(ticks);
@@ -924,8 +924,7 @@ static inline void update_times(void)
 
 void do_timer(struct pt_regs *regs)
 {
-	jiffies_64++;
-	update_times();
+	update_times(++jiffies_64);
 	softlockup_tick(regs);
 }
 
---
Atsushi Nemoto

Re: jiffies_64 vs. jiffies

[Nick Piggin]

Atsushi Nemoto wrote:

> @@ -924,8 +924,7 @@ static inline void update_times(void)
>  
>  void do_timer(struct pt_regs *regs)
>  {
> -	jiffies_64++;
> -	update_times();
> +	update_times(++jiffies_64);
>  	softlockup_tick(regs);
>  }
>  

jiffies_64 is not volatile so you should not have to obfuscate
the code like this.

-- 
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: jiffies_64 vs. jiffies

[Atsushi Nemoto]

>>>>> On Wed, 01 Mar 2006 23:52:37 +1100, Nick Piggin <nickpiggin@yahoo.com.au> said:

>> void do_timer(struct pt_regs *regs)
>> {
>> -	jiffies_64++;
>> -	update_times();
>> +	update_times(++jiffies_64);
>>  	softlockup_tick(regs);
>> }

nick> jiffies_64 is not volatile so you should not have to obfuscate
nick> the code like this.

Well, do you mean it should be like this ?

	jiffies_64++;
	update_times(jiffies_64);

Thanks for your comments.
---
Atsushi Nemoto

Re: jiffies_64 vs. jiffies

[Nick Piggin]

Atsushi Nemoto wrote:
>>>>>>On Wed, 01 Mar 2006 23:52:37 +1100, Nick Piggin <nickpiggin@yahoo.com.au> said:
> 
> 
>>>void do_timer(struct pt_regs *regs)
>>>{
>>>-	jiffies_64++;
>>>-	update_times();
>>>+	update_times(++jiffies_64);
>>> 	softlockup_tick(regs);
>>>}
> 
> 
> nick> jiffies_64 is not volatile so you should not have to obfuscate
> nick> the code like this.
> 
> Well, do you mean it should be like this ?
> 
> 	jiffies_64++;
> 	update_times(jiffies_64);
> 

Yeah. It makes your patch a line smaller too!

> Thanks for your comments.

Oh it was nothing really ;)

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: jiffies_64 vs. jiffies

[Atsushi Nemoto]

>>>>> On Thu, 02 Mar 2006 02:00:16 +1100, Nick Piggin <nickpiggin@yahoo.com.au> said:

>> Well, do you mean it should be like this ?
>> 
>> jiffies_64++;
>> update_times(jiffies_64);

nick> Yeah. It makes your patch a line smaller too!

Another solution might be simplifying update_times() like this.  It
looks there is no point to calculate ticks in update_times().

diff --git a/kernel/timer.c b/kernel/timer.c
index fe3a9a9..6188c99 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -906,14 +906,9 @@ void run_local_timers(void)
  */
 static inline void update_times(void)
 {
-	unsigned long ticks;
-
-	ticks = jiffies - wall_jiffies;
-	if (ticks) {
-		wall_jiffies += ticks;
-		update_wall_time(ticks);
-	}
-	calc_load(ticks);
+	wall_jiffies++;
+	update_wall_time(1);
+	calc_load(1);
 }
   
 /*


As for long term solution, using an union for jiffies and jiffies_64
would be robust.  But it affects so many codes ...

---
Atsushi Nemoto

Re: jiffies_64 vs. jiffies

[Paul Mackerras]

Atsushi Nemoto writes:

> Hi.  I noticed that the 'jiffies' variable has 'wall_jiffies + 1'
> value in most of time.  I'm using MIPS platform but I think this is
> same for other platforms.
> 
> I suppose this is due to gcc does not know that jiffies_64 and jiffies
> share same place.

I can confirm that the same thing happens on powerpc, both 32-bit and
64-bit.  The compiler loads up jiffies, jiffies_64 and wall_jiffies
into registers before storing back the incremented value into
jiffies_64 and then updating wall_jiffies.

Thanks for finding that, it explains some other strange things that I
have seen happen.

Paul.