Skip to contentSkip to conversation

powerpc: Fix race in the pasemi timebase calibration

PATCH In review Olof Johansson 2 participants 4 messages Aug 21, 2007 – Aug 22, 2007 PowerPC

lore
Share & tools
lore

Olof Johansson

author
#0 lore raw

PATCH 1/1 powerpc: Fix race in the pasemi timebase calibration

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 <[email protected]>

---

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

arch/powerpc/platforms/pasemi/setup.c +4 -0
Index: 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());
 }
#1 lore raw
Quoted 4 lines · 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).

OK.

arch/powerpc/platforms/pasemi/setup.c static void __devinit pas_give_timebase( · line 65

Quoted 5 lines
arch/powerpc/platforms/pasemi/setup.c +1 -0
@@ -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.

author
#2 lore raw
Quoted 9 lines · Paul Mackerras
Quoted 4 lines · 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).

OK.

Quoted 8 lines

arch/powerpc/platforms/pasemi/setup.c static void __devinit pas_give_timebase( · line 65

Quoted 5 lines
arch/powerpc/platforms/pasemi/setup.c +1 -0
@@ -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.

Quoted 5 lines

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

author
#3 lore raw

PATCH 1/1 powerpc: Rework SMP timebase handoff for pasemi

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 <[email protected]>
arch/powerpc/platforms/pasemi/setup.c +14 -10
Index: 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 = {