Re: [PATCH v2 1/3] powerpc/booke64: add sync after writing PTE

On Fri, 2013-10-11 at 10:51 +1100, Benjamin Herrenschmidt wrote:

On Thu, 2013-10-10 at 18:25 -0500, Scott Wood wrote:

quoted

Looking at some of the code in mm/, I suspect that the normal callers of
set_pte_at() already have an unlock (and thus a sync) 

Unlock is lwsync actually...

Oops, I was seeing the conditional sync from SYNC_IO in the disassembly.
BTW, it's a bug that we don't do SYNC_IO on e500mc -- the assumption
that lwsync is 64-bit-only is no longer true.

quoted

already, so we may
not even be relying on those retries.  Certainly some of them do; it
would take some effort to verify all of them.

Also, without such a sync in map_kernel_page(), even with software
tablewalk, couldn't we theoretically have a situation where a store to
pointer X that exposes a new mapping gets reordered before the PTE store
as seen by another CPU?  The other CPU could see non-NULL X and
dereference it, but get the stale PTE.  Callers of ioremap() generally
don't do a barrier of their own prior to exposing the result.

Hrm, we transition to the new PTE either restricts the access permission
in which case it flushes the TLB (and synchronizes with other CPUs) or
extends access (adds dirty, set pte from 0 -> populated, ...) in which
case the worst case is we see the old one and take a spurrious fault.

Yes, and the lwsync is good enough for software reading the PTE.  So it
becomes a question of how much spurious faults with hardware tablewalk
hurt performance, and at least for the lmbench fork test, the sync is
worse (or maybe lwsync happens to be good enough for hw tablewalk on
e6500?).

So the problem would only be with kernel mappings and in that case I
think we are fine. A driver doing an ioremap shouldn't then start using
that mapping on another CPU before having *informed* that other CPU of
the existence of the mapping and that should be ordered.

But are callers of ioremap() expected to use a barrier before exposing
the pointer (and what type)?  I don't think that's common practice.

map_kernel_page() should not be performance critical, so it shouldn't be
a big deal to put mb() in there.

-Scott