Messages in this thread Patch in this message | | | Date | Fri, 20 Oct 2006 17:05:38 +0100 | From | Ralf Baechle <> | Subject | Re: [PATCH 1/3] Fix COW D-cache aliasing on fork |
| |
On Sat, Oct 21, 2006 at 12:39:40AM +1000, Nick Piggin wrote:
> >>That would require changing the order of cache flush and tlb flush. > >>To keep certain architectures that require a valid translation in > >>the TLB the cacheflush has to be done first. Not sure if those > >>architectures need a writeable mapping for dirty cachelines - I > >>think hypersparc was one of them. > > > > > >There just has to be "a mapping" in the TLB so that the L2 cache can > >translate the virtual address to a physical one for the writeback to > >main memory. > > So moving the flush_cache_mm below the copy_page_range, to just > before the flush_tlb_mm, would work then? This would make the > race much smaller than with this patchset.
90% of this changeset are MIPS-specific code. Of that in turn much is just infrastructure which is already being used anyway.
> But doesn't that still leave a race?
Both calls would have to be done under the mmap_sem to close any races.
> What if another thread writes to cache after we have flushed it > but before flushing the TLBs? Although we've marked the the ptes > readonly, the CPU won't trap if the TLB is valid? There must be > some special way for the arch to handle this, but I can't see it.
There isn't really. Reordering with a patch like:
Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
diff --git a/kernel/fork.c b/kernel/fork.c index 29ebb30..28e51e0 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -202,7 +202,6 @@ static inline int dup_mmap(struct mm_str struct mempolicy *pol; down_write(&oldmm->mmap_sem); - flush_cache_mm(oldmm); /* * Not linked in yet - no deadlock potential: */ @@ -287,8 +286,9 @@ static inline int dup_mmap(struct mm_str } retval = 0; out: - up_write(&mm->mmap_sem); flush_tlb_mm(oldmm); + flush_cache_mm(oldmm); + up_write(&mm->mmap_sem); up_write(&oldmm->mmap_sem); return retval; fail_nomem_policy: should close the hole for all effected architectures. I say should because this patch would need another round of linux-arch reviewing and I haven't tested it this patch yet myself.
But even so that doesn't change that I would really like to make copy_user_highpage() an arch interface replacing copy_to_user_page.
The current way of doing things enforces a cacheflush on MIPS which itself is pricy - 1,000 cycles when it's cheap but could be several times as expensive. And as a side effect of the cacheflush the process breaking a COW page will start with a cold page.
Or if an architecture wants to be clever about aliasing and uses the vto argument of copy_user_page to create a non-conflicting mapping it means the mapping setup by copy_user_highpage will be unused ...
Ralf - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |