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 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
