Re: Linux v2.6.18-rc5

Re: Linux v2.6.18-rc5

[Nathan Lynch]

Hi Olaf-

Olaf Hering wrote:
> On Sun, Aug 27, Linus Torvalds wrote:
> 
> > Pls test it out, and please remind all the appropriate people about any 
> > regressions you find (including any found earlier if they haven't been 
> > addressed yet).
> 
> > Nathan Lynch:
> >       [POWERPC] Fix gettimeofday inaccuracies
> 
> Tested on B&W G3, iBook1 and a G4/466.
> This patch causes deadlocks on ppc32, but not on ppc64. Have to verify
> it on a vanilla kernel, but I'm sure there are no funky patches in
> openSuSE.
> 
> https://bugzilla.novell.com/show_bug.cgi?id=202146

Sorry about that, does this (a partial revert of the change) fix it
for you?


diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 18e59e4..fe9b1d9 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -655,7 +655,6 @@ void timer_interrupt(struct pt_regs * re
 	int next_dec;
 	int cpu = smp_processor_id();
 	unsigned long ticks;
-	u64 tb_next_jiffy;
 
 #ifdef CONFIG_PPC32
 	if (atomic_read(&ppc_n_lost_interrupts) != 0)
@@ -697,14 +696,11 @@ void timer_interrupt(struct pt_regs * re
 			continue;
 
 		write_seqlock(&xtime_lock);
-		tb_next_jiffy = tb_last_jiffy + tb_ticks_per_jiffy;
-		if (per_cpu(last_jiffy, cpu) >= tb_next_jiffy) {
-			tb_last_jiffy = tb_next_jiffy;
-			tb_last_stamp = per_cpu(last_jiffy, cpu);
-			do_timer(regs);
-			timer_recalc_offset(tb_last_jiffy);
-			timer_check_rtc();
-		}
+		tb_last_jiffy += tb_ticks_per_jiffy;
+		tb_last_stamp = per_cpu(last_jiffy, cpu);
+		do_timer(regs);
+		timer_recalc_offset(tb_last_jiffy);
+		timer_check_rtc();
 		write_sequnlock(&xtime_lock);
 	}
 	

Re: Linux v2.6.18-rc5

[Yves-Alexis Perez]

On Tue, 2006-08-29 at 08:06 -0500, Nathan Lynch wrote:
> Sorry about that, does this (a partial revert of the change) fix it
> for you? 

I tried 2.6.18-rc5 on my powerbook g4 (5,6) and indeed I had lots of
deadlocks, it was quite unusable. I've tried this patch and it seems to
fix the problem. (up for half an our without problems, I guess)
-- 
Yves-Alexis

Re: Linux v2.6.18-rc5

[Olaf Hering]

On Tue, Aug 29, Nathan Lynch wrote:

> Hi Olaf-
> 
> Olaf Hering wrote:
> > On Sun, Aug 27, Linus Torvalds wrote:
> > 
> > > Pls test it out, and please remind all the appropriate people about any 
> > > regressions you find (including any found earlier if they haven't been 
> > > addressed yet).
> > 
> > > Nathan Lynch:
> > >       [POWERPC] Fix gettimeofday inaccuracies
> > 
> > Tested on B&W G3, iBook1 and a G4/466.
> > This patch causes deadlocks on ppc32, but not on ppc64. Have to verify
> > it on a vanilla kernel, but I'm sure there are no funky patches in
> > openSuSE.
> > 
> > https://bugzilla.novell.com/show_bug.cgi?id=202146
> 
> Sorry about that, does this (a partial revert of the change) fix it
> for you?

Yes, it works ok with this change.

Re: Linux v2.6.18-rc5

[Paul Mackerras]

Olaf,

This patch should fix it.  The problem was that I was comparing a
32-bit quantity with a 64-bit quantity, and consequently time wasn't
advancing.  This makes us use a 64-bit quantity on all platforms,
which ends up simplifying the code since we can now get rid of the
tb_last_stamp variable (which actually fixes another bug that Ben H
and I noticed while going carefully through the code).

This works fine on my G4 tibook.  Let me know how it goes on your
machines.

Paul.

diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 18e59e4..a124499 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -125,15 +125,8 @@ static long timezone_offset;
 unsigned long ppc_proc_freq;
 unsigned long ppc_tb_freq;
 
-u64 tb_last_jiffy __cacheline_aligned_in_smp;
-unsigned long tb_last_stamp;
-
-/*
- * Note that on ppc32 this only stores the bottom 32 bits of
- * the timebase value, but that's enough to tell when a jiffy
- * has passed.
- */
-DEFINE_PER_CPU(unsigned long, last_jiffy);
+static u64 tb_last_jiffy __cacheline_aligned_in_smp;
+static DEFINE_PER_CPU(u64, last_jiffy);
 
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING
 /*
@@ -458,7 +451,7 @@ void do_gettimeofday(struct timeval *tv)
 		do {
 			seq = read_seqbegin_irqsave(&xtime_lock, flags);
 			sec = xtime.tv_sec;
-			nsec = xtime.tv_nsec + tb_ticks_since(tb_last_stamp);
+			nsec = xtime.tv_nsec + tb_ticks_since(tb_last_jiffy);
 		} while (read_seqretry_irqrestore(&xtime_lock, seq, flags));
 		usec = nsec / 1000;
 		while (usec >= 1000000) {
@@ -700,7 +693,6 @@ #endif
 		tb_next_jiffy = tb_last_jiffy + tb_ticks_per_jiffy;
 		if (per_cpu(last_jiffy, cpu) >= tb_next_jiffy) {
 			tb_last_jiffy = tb_next_jiffy;
-			tb_last_stamp = per_cpu(last_jiffy, cpu);
 			do_timer(regs);
 			timer_recalc_offset(tb_last_jiffy);
 			timer_check_rtc();
@@ -749,7 +741,7 @@ void __init smp_space_timers(unsigned in
 	int i;
 	unsigned long half = tb_ticks_per_jiffy / 2;
 	unsigned long offset = tb_ticks_per_jiffy / max_cpus;
-	unsigned long previous_tb = per_cpu(last_jiffy, boot_cpuid);
+	u64 previous_tb = per_cpu(last_jiffy, boot_cpuid);
 
 	/* make sure tb > per_cpu(last_jiffy, cpu) for all cpus always */
 	previous_tb -= tb_ticks_per_jiffy;
@@ -830,7 +822,7 @@ #endif
 	 * and therefore the (jiffies - wall_jiffies) computation
 	 * has been removed.
 	 */
-	tb_delta = tb_ticks_since(tb_last_stamp);
+	tb_delta = tb_ticks_since(tb_last_jiffy);
 	tb_delta = mulhdu(tb_delta, do_gtod.varp->tb_to_xs); /* in xsec */
 	new_nsec -= SCALE_XSEC(tb_delta, 1000000000);
 
@@ -950,8 +942,7 @@ void __init time_init(void)
 	if (__USE_RTC()) {
 		/* 601 processor: dec counts down by 128 every 128ns */
 		ppc_tb_freq = 1000000000;
-		tb_last_stamp = get_rtcl();
-		tb_last_jiffy = tb_last_stamp;
+		tb_last_jiffy = get_rtcl();
 	} else {
 		/* Normal PowerPC with timebase register */
 		ppc_md.calibrate_decr();
@@ -959,7 +950,7 @@ void __init time_init(void)
 		       ppc_tb_freq / 1000000, ppc_tb_freq % 1000000);
 		printk(KERN_DEBUG "time_init: processor frequency   = %lu.%.6lu MHz\n",
 		       ppc_proc_freq / 1000000, ppc_proc_freq % 1000000);
-		tb_last_stamp = tb_last_jiffy = get_tb();
+		tb_last_jiffy = get_tb();
 	}
 
 	tb_ticks_per_jiffy = ppc_tb_freq / HZ;
@@ -1036,7 +1027,7 @@ void __init time_init(void)
 	do_gtod.varp = &do_gtod.vars[0];
 	do_gtod.var_idx = 0;
 	do_gtod.varp->tb_orig_stamp = tb_last_jiffy;
-	__get_cpu_var(last_jiffy) = tb_last_stamp;
+	__get_cpu_var(last_jiffy) = tb_last_jiffy;
 	do_gtod.varp->stamp_xsec = (u64) xtime.tv_sec * XSEC_PER_SEC;
 	do_gtod.tb_ticks_per_sec = tb_ticks_per_sec;
 	do_gtod.varp->tb_to_xs = tb_to_xs;
diff --git a/include/asm-powerpc/time.h b/include/asm-powerpc/time.h
index dcde441..5785ac4 100644
--- a/include/asm-powerpc/time.h
+++ b/include/asm-powerpc/time.h
@@ -30,10 +30,6 @@ extern unsigned long tb_ticks_per_usec;
 extern unsigned long tb_ticks_per_sec;
 extern u64 tb_to_xs;
 extern unsigned      tb_to_us;
-extern unsigned long tb_last_stamp;
-extern u64 tb_last_jiffy;
-
-DECLARE_PER_CPU(unsigned long, last_jiffy);
 
 struct rtc_time;
 extern void to_tm(int tim, struct rtc_time * tm);

Re: Linux v2.6.18-rc5

[Olaf Hering]

On Wed, Aug 30, Paul Mackerras wrote:

> This works fine on my G4 tibook.  Let me know how it goes on your
> machines.

Works ok on an iBook1.

Re: Linux v2.6.18-rc5

[Mikael Pettersson]

Paul Mackerras writes:
 > Olaf,
 > 
 > This patch should fix it.  The problem was that I was comparing a
 > 32-bit quantity with a 64-bit quantity, and consequently time wasn't
 > advancing.  This makes us use a 64-bit quantity on all platforms,
 > which ends up simplifying the code since we can now get rid of the
 > tb_last_stamp variable (which actually fixes another bug that Ben H
 > and I noticed while going carefully through the code).
 > 
 > This works fine on my G4 tibook.  Let me know how it goes on your
 > machines.

Thanks. This fixed a kernel hang bug on my G4 eMac with 2.6.18-rc5.

The vanilla kernel ran fine until I tar xvf'd a file from an NFS-mount,
then everything ground to a halt.

/Mikael