[PATCH] powerpc: Fix race in the pasemi timebase calibration

[PATCH] powerpc: Fix race in the pasemi timebase calibration

[Olof Johansson]

Make sure the new timebase value is available by the time take_timebase
completes. Otherwise take_timebase might race with give_timebase,
causing severe badness when the value later is modified (think looong
hang trying to catch up with a very large number of lost ticks).

This has shown up lately, possibly because of other code paths in the
startup of secondary cpus being slimmed down enough that the race happened
more often.

Signed-off-by: Olof Johansson <olof@lixom.net>


---

Paul,

This started showing up in the 2.6.22 timeframe, and noone else seems
to have hit it yet, so there's no urge to get it into 2.6.23. Please
queue it for .24 though.


Thanks,

-Olof

Index: mainline/arch/powerpc/platforms/pasemi/setup.c
===================================================================
--- mainline.orig/arch/powerpc/platforms/pasemi/setup.c
+++ mainline/arch/powerpc/platforms/pasemi/setup.c
@@ -50,6 +50,7 @@ static void pas_restart(char *cmd)
 
 #ifdef CONFIG_SMP
 static DEFINE_SPINLOCK(timebase_lock);
+static unsigned long timebase_avail;
 
 static void __devinit pas_give_timebase(void)
 {
@@ -61,6 +62,7 @@ static void __devinit pas_give_timebase(
 	mtspr(SPRN_TBCTL, TBCTL_UPDATE_LOWER | (tb & 0xffffffff));
 	mtspr(SPRN_TBCTL, TBCTL_UPDATE_UPPER | (tb >> 32));
 	mtspr(SPRN_TBCTL, TBCTL_RESTART);
+	timebase_avail = 1;
 	spin_unlock(&timebase_lock);
 	pr_debug("pas_give_timebase: cpu %d gave tb %lx\n",
 		 smp_processor_id(), tb);
@@ -68,6 +70,8 @@ static void __devinit pas_give_timebase(
 
 static void __devinit pas_take_timebase(void)
 {
+	while (!timebase_avail)
+		smp_rmb();
 	pr_debug("pas_take_timebase: cpu %d has tb %lx\n",
 		 smp_processor_id(), mftb());
 }

Re: [PATCH] powerpc: Fix race in the pasemi timebase calibration

[Paul Mackerras]

Olof Johansson writes:

> Make sure the new timebase value is available by the time take_timebase
> completes. Otherwise take_timebase might race with give_timebase,
> causing severe badness when the value later is modified (think looong
> hang trying to catch up with a very large number of lost ticks).

OK.

> @@ -61,6 +62,7 @@ static void __devinit pas_give_timebase(
>  	mtspr(SPRN_TBCTL, TBCTL_UPDATE_LOWER | (tb & 0xffffffff));
>  	mtspr(SPRN_TBCTL, TBCTL_UPDATE_UPPER | (tb >> 32));
>  	mtspr(SPRN_TBCTL, TBCTL_RESTART);
> +	timebase_avail = 1;

No memory barrier before setting timebase_avail?  Shouldn't there be
one?

Actually I don't understand that code at all.  Your give_timebase
seems to freeze the timebase, read it, set it to the same value and
restart, all without synchronizing with the other cpu, and your
take_timebase does nothing except print the timebase.  How does that
work?

Regards,
Paul.

Re: [PATCH] powerpc: Fix race in the pasemi timebase calibration

[Olof Johansson]

On Wed, Aug 22, 2007 at 11:27:33AM +1000, Paul Mackerras wrote:
> Olof Johansson writes:
> 
> > Make sure the new timebase value is available by the time take_timebase
> > completes. Otherwise take_timebase might race with give_timebase,
> > causing severe badness when the value later is modified (think looong
> > hang trying to catch up with a very large number of lost ticks).
> 
> OK.
> 
> > @@ -61,6 +62,7 @@ static void __devinit pas_give_timebase(
> >  	mtspr(SPRN_TBCTL, TBCTL_UPDATE_LOWER | (tb & 0xffffffff));
> >  	mtspr(SPRN_TBCTL, TBCTL_UPDATE_UPPER | (tb >> 32));
> >  	mtspr(SPRN_TBCTL, TBCTL_RESTART);
> > +	timebase_avail = 1;
> 
> No memory barrier before setting timebase_avail?  Shouldn't there be
> one?

Technically there's no previous memory access to put that barrier up
against since they're all SPR ops, but an isync after the mtspr would
be warranted.

> Actually I don't understand that code at all.  Your give_timebase
> seems to freeze the timebase, read it, set it to the same value and
> restart, all without synchronizing with the other cpu, and your
> take_timebase does nothing except print the timebase.  How does that
> work?

The TBCTL functions control the TBs of all cores. I.e. current
give_timebase will push out the current TB of the booting core to all
others in the system.

And yes, I had misunderstood the timebase calibration back when I
implemented it, not realizing we do a give+take for each cpu coming
up. It should really look more like the pseries implementation, only
using TBCTL to freeze/thaw and do the handover manually. That'll be CPU
hotplug-proof as well.

New patch reworking all of this coming. Thanks for the reality check.

-Olof

[PATCH] powerpc: Rework SMP timebase handoff for pasemi

[Olof Johansson]

Rework timebase handoff to play nice with configurations with more than
2 cores, as well as with CPU hotplug.

Previous scheme just pushed out the current timebase from the giving
core to all cores without caring if they wanted it or not, nor checking
if they'd taken it. The taking side didn't make sure the giving side
had provided a value yet either. In other words, it was completely broken.


Signed-off-by: Olof Johansson <olof@lixom.net>

Index: mainline/arch/powerpc/platforms/pasemi/setup.c
===================================================================
--- mainline.orig/arch/powerpc/platforms/pasemi/setup.c
+++ mainline/arch/powerpc/platforms/pasemi/setup.c
@@ -50,26 +50,30 @@ static void pas_restart(char *cmd)
 
 #ifdef CONFIG_SMP
 static DEFINE_SPINLOCK(timebase_lock);
+static unsigned long timebase;
 
 static void __devinit pas_give_timebase(void)
 {
-	unsigned long tb;
-
 	spin_lock(&timebase_lock);
 	mtspr(SPRN_TBCTL, TBCTL_FREEZE);
-	tb = mftb();
-	mtspr(SPRN_TBCTL, TBCTL_UPDATE_LOWER | (tb & 0xffffffff));
-	mtspr(SPRN_TBCTL, TBCTL_UPDATE_UPPER | (tb >> 32));
-	mtspr(SPRN_TBCTL, TBCTL_RESTART);
+	isync();
+	timebase = get_tb();
 	spin_unlock(&timebase_lock);
-	pr_debug("pas_give_timebase: cpu %d gave tb %lx\n",
-		 smp_processor_id(), tb);
+
+	while (timebase)
+		barrier();
+	mtspr(SPRN_TBCTL, TBCTL_RESTART);
 }
 
 static void __devinit pas_take_timebase(void)
 {
-	pr_debug("pas_take_timebase: cpu %d has tb %lx\n",
-		 smp_processor_id(), mftb());
+	while (!timebase)
+		smp_rmb();
+
+	spin_lock(&timebase_lock);
+	set_tb(timebase >> 32, timebase & 0xffffffff);
+	timebase = 0;
+	spin_unlock(&timebase_lock);
 }
 
 struct smp_ops_t pas_smp_ops = {