On Wed, Nov 19, 2008 at 08:17:15PM +0000, Russell King wrote: > On Wed, Nov 19, 2008 at 05:53:38AM +0100, Nick Piggin wrote: > > We determined that the problem was most likely due to a cache aliasing > > issue. flush_cache_vunmap was only being called at the moment the page > > tables were to be taken down, however with lazy unmapping, this can > > happen after the page has subsequently been freed and allocated for > > something else. The dangling alias may still have dirty data attached > > to it. > > This would certainly cause problems - writes which hit the cache for the > vmapped pages will remain in the cache, and would be evicted some time > later. > > With VIVT ARMs, that time is bounded by the context switch, which does a > full cache flush, but between that a page may well have been reallocated > for some other purpose. > > So, ensuring that we have no dirty lines corresponding to stale mappings > on pages before they're freed is an absolute must for VIVT caches. > > > @@ -734,7 +743,7 @@ static void free_vmap_block(struct vmap_ > > spin_unlock(&vmap_block_tree_lock); > > BUG_ON(tmp != vb); > > > > - free_unmap_vmap_area(vb->va); > > + free_unmap_vmap_area_noflush(vb->va); > > call_rcu(&vb->rcu_head, rcu_free_vb); > > } > > > > @@ -820,6 +829,8 @@ static void vb_free(const void *addr, un > > free_vmap_block(vb); > > } else > > spin_unlock(&vb->lock); > > + > > + flush_cache_vunmap((unsigned long)addr, (unsigned long)addr + size); > > One minor nit - this looks like we're flushing the cache after the > range has been marked as needing the mapping torn down. Can the > mapping be torn down before the cache is flushed? > > The reason I ask is that with VIPT caches, the mapping has to be > present so the hardware knows the physical tag to be flushed, so if > the mapping has been torn down, we'll get an exception. _If_ and > only if we convert flush_cache_vunmap() to be less heavy weight > than its current flush_cache_all(). > > IOW, shouldn't flush_cache_vunmap() be towards the start of vb_free() ? You are correct. Thanks. Andrew, can you queue this incremental fix please? -- Russell King wrote: > One minor nit - this looks like we're flushing the cache after the > range has been marked as needing the mapping torn down. Can the > mapping be torn down before the cache is flushed? > > The reason I ask is that with VIPT caches, the mapping has to be > present so the hardware knows the physical tag to be flushed, so if > the mapping has been torn down, we'll get an exception. _If_ and > only if we convert flush_cache_vunmap() to be less heavy weight > than its current flush_cache_all(). > > IOW, shouldn't flush_cache_vunmap() be towards the start of vb_free() ? Signed-off-by: Nick Piggin <npiggin@xxxxxxx> --- Index: linux-2.6/mm/vmalloc.c =================================================================== --- linux-2.6.orig/mm/vmalloc.c +++ linux-2.6/mm/vmalloc.c @@ -805,6 +805,9 @@ static void vb_free(const void *addr, un BUG_ON(size & ~PAGE_MASK); BUG_ON(size > PAGE_SIZE*VMAP_MAX_ALLOC); + + flush_cache_vunmap((unsigned long)addr, (unsigned long)addr + size); + order = get_order(size); offset = (unsigned long)addr & (VMAP_BLOCK_SIZE - 1); @@ -829,8 +832,6 @@ static void vb_free(const void *addr, un free_vmap_block(vb); } else spin_unlock(&vb->lock); - - flush_cache_vunmap((unsigned long)addr, (unsigned long)addr + size); } /** -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
