Re: [PATCH] THP: mremap support and TLB optimization #2
From: Johannes Weiner <hidden>
Date: 2011-08-05 10:51:17
On Thu, Jul 28, 2011 at 04:26:31PM +0200, Andrea Arcangeli wrote:
Hello, this is the latest version of the mremap THP native implementation plus optimizations. So first question is: am I right, the "- 1" that I am removing below was buggy? It's harmless because these old_end/next are page aligned, but if PAGE_SIZE would be 1, it'd break, right? It's really confusing to read even if it happens to work. Please let me know because that "- 1" ironically it's the thing I'm less comfortable about in this patch.
quoted hunk ↗ jump to hunk
@@ -134,14 +126,17 @@ unsigned long move_page_tables(struct vm { unsigned long extent, next, old_end; pmd_t *old_pmd, *new_pmd; + bool need_flush = false; old_end = old_addr + len; flush_cache_range(vma, old_addr, old_end); + mmu_notifier_invalidate_range_start(vma->vm_mm, old_addr, old_end); + for (; old_addr < old_end; old_addr += extent, new_addr += extent) { cond_resched(); next = (old_addr + PMD_SIZE) & PMD_MASK; - if (next - 1 > old_end) + if (next > old_end) next = old_end;
If old_addr + PMD_SIZE overflows, next will be zero, thus smaller than old_end and not fixed up. This results in a bogus extent length here:
extent = next - old_addr;
which I think can overrun old_addr + len if the extent should have actually been smaller than the distance between new_addr and the next PMD as well as that LATENCY_LIMIT to which extent is capped a few lines below. I haven't checked all the possibilities, though. It could probably be if (next > old_end || next - 1 > old_end) next = old_end to catch the theoretical next == old_end + 1 case, but PAGE_SIZE > 1 looks like a sensible assumption to me. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>