On Mon, 2010-12-20 at 16:50 +0000, Russell King - ARM Linux wrote:
> On Mon, Dec 20, 2010 at 04:39:07PM +0000, Catalin Marinas wrote:
> > On 18 December 2010 11:04, Russell King - ARM Linux
> > <linux@xxxxxxxxxxxxxxxx> wrote:
> > > There is a subtle race in the CPU hotplug code, where a CPU which has
> > > been offlined can online itself before being requested, which results
> > > in things going astray on the next online/offline cycle.
> > [...]
> > > --- a/arch/arm/mach-realview/platsmp.c
> > > +++ b/arch/arm/mach-realview/platsmp.c
> > > @@ -36,6 +36,19 @@ extern void realview_secondary_startup(void);
> > > */
> > > volatile int __cpuinitdata pen_release = -1;
> > >
> > > +/*
> > > + * Write pen_release in a way that is guaranteed to be visible to all
> > > + * observers, irrespective of whether they're taking part in coherency
> > > + * or not. This is necessary for the hotplug code to work reliably.
> > > + */
> > > +static void write_pen_release(int val)
> > > +{
> > > + pen_release = val;
> > > + smp_wmb();
> > > + __cpuc_flush_dcache_area((void *)&pen_release, sizeof(pen_release));
> > > + outer_clean_range(__pa(&pen_release), __pa(&pen_release + 1));
> > > +}
> >
> > Just a minor thing - I don't think we need any barrier here. According
> > to the ARM ARM B2.2.7:
> >
> > "Any data cache or unified cache maintenance operation by MVA must be
> > executed in program order
> > relative to any explicit load or store on the same processor to an
> > address covered by the MVA of the
> > cache operation."
>
> Right, I had been thinking about that. I think the barrier came from the
> original ARM implementation, and I added the cache flushes.
I think the original smp_wmb() was there to prevent the pen_release
update not being visible to the primary CPU before the spin_lock() loop
on the secondary, leading to a deadlock.
The smp_wmb() is no longer needed with your patch as we do explicit
cache flushing which contains a DSB. But for clarity, you may still want
to keep it as a separate call in platform_secondary_init() rather than
write_pen_release():
* let the primary processor know we're out of the
* pen, then head off into the C entry point
*/
- pen_release = -1;
+ write_pen_release(-1);
smp_wmb();
/*
* Synchronise with the boot thread.
> > We also have a corresponding smp_rmb() in boot_secondary(), I don't
> > think it has any use either.
>
> I think you're right, but it looks a little unsafe without:
>
> timeout = jiffies + (1 * HZ);
> while (time_before(jiffies, timeout)) {
> if (pen_release == -1)
> break;
>
> udelay(10);
> }
>
> I don't think it makes much odds if the pen_release check gets
> re-ordered, especially as we do a final pen_release check after we
> unlock. However, could the loop end up waiting additional 10us
> without the smp_rmb() ?
Looking through A3.8.2, there is only a "control dependency" between
reading the jiffies and reading the pen_release. My understanding is
that these reads could happen in any order, so to avoid an additional
10us wait the barrier is still useful.
This smp_rmb() was supposed to work in pair with the smp_wmb() above but
that's only by following the memory-barriers.txt document. From an ARM
perspective, we could simply have an smp_mb() in boot_secondary().
If we keep the smp_wmb() in platform_secondary_init() for clarity, we
could keep the smp_rmb() here as well. I think I prefer this option.
--
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
