THP: mremap support and TLB optimization #2

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.

mm/mremap.c +1 -0

@@ -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;

Quoted 20 lines · Andrea Arcangeli

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 diff) +1 -0

@@ -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);
 

Review unsigned long move_page_tables(struct vm · line 140

Quoted 8 lines

(quoted diff) +3 -1

+	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:

Review unsigned long move_page_tables(struct vm · line 141

Quoted 1 line

(quoted diff) +0 -0

 		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.

=1g vs 2m.

Here are also some benchmarks with a proggy like this:

===
#define _GNU_SOURCE
#include <sys/mman.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <sys/time.h>

#define SIZE (5UL*1024*1024*1024)

int main()

{
        static struct timeval oldstamp, newstamp;
	long diffsec;
	char *p, *p2, *p3, *p4;
	if (posix_memalign((void **)&p, 2*1024*1024, SIZE))
		perror("memalign"), exit(1);
	if (posix_memalign((void **)&p2, 2*1024*1024, SIZE))
		perror("memalign"), exit(1);
	if (posix_memalign((void **)&p3, 2*1024*1024, 4096))
		perror("memalign"), exit(1);

	memset(p, 0xff, SIZE);
	memset(p2, 0xff, SIZE);
	memset(p3, 0x77, 4096);
	gettimeofday(&oldstamp, NULL);
	p4 = mremap(p, SIZE, SIZE, MREMAP_FIXED|MREMAP_MAYMOVE, p3);
	gettimeofday(&newstamp, NULL);
	diffsec = newstamp.tv_sec - oldstamp.tv_sec;
	diffsec = newstamp.tv_usec - oldstamp.tv_usec + 1000000 * diffsec;
	printf("usec %ld\n", diffsec);
	if (p == MAP_FAILED || p4 != p3)
	//if (p == MAP_FAILED)
		perror("mremap"), exit(1);
	if (memcmp(p4, p2, SIZE))
		printf("mremap bug\n"), exit(1);
	printf("ok\n");

	return 0;
}

===

THP on

Performance counter stats for './largepage13' (3 runs):

          69195836 dTLB-loads                 ( +-   3.546% )  (scaled from 50.30%)
             60708 dTLB-load-misses           ( +-  11.776% )  (scaled from 52.62%)
         676266476 dTLB-stores                ( +-   5.654% )  (scaled from 69.54%)
             29856 dTLB-store-misses          ( +-   4.081% )  (scaled from 89.22%)
        1055848782 iTLB-loads                 ( +-   4.526% )  (scaled from 80.18%)
              8689 iTLB-load-misses           ( +-   2.987% )  (scaled from 58.20%)

        7.314454164  seconds time elapsed   ( +-   0.023% )

THP off

Performance counter stats for './largepage13' (3 runs):

        1967379311 dTLB-loads                 ( +-   0.506% )  (scaled from 60.59%)
           9238687 dTLB-load-misses           ( +-  22.547% )  (scaled from 61.87%)
        2014239444 dTLB-stores                ( +-   0.692% )  (scaled from 60.40%)
           3312335 dTLB-store-misses          ( +-   7.304% )  (scaled from 67.60%)
        6764372065 iTLB-loads                 ( +-   0.925% )  (scaled from 79.00%)
              8202 iTLB-load-misses           ( +-   0.475% )  (scaled from 70.55%)

        9.693655243  seconds time elapsed   ( +-   0.069% )

grep thp /proc/vmstat
thp_fault_alloc 35849
thp_fault_fallback 0
thp_collapse_alloc 3
thp_collapse_alloc_failed 0
thp_split 0

thp_split 0 confirms no thp split despite plenty of hugepages allocated.

The measurement of only the mremap time (so excluding the 3 long memset and final long 10GB memory accessing memcmp):

THP on

usec 14824
usec 14862
usec 14859

THP off

usec 256416
usec 255981
usec 255847

With an older kernel without the mremap optimizations (the below patch optimizes the non THP version too).

THP on

usec 392107
usec 390237
usec 404124

THP off

usec 444294
usec 445237
usec 445820

I guess with a threaded program that sends more IPI on large SMP it'd create an even larger difference.

All debug options are off except DEBUG_VM to avoid skewing the results.

The only problem for native 2M mremap like it happens above both the source and destination address must be 2M aligned or the hugepmd can't be moved without a split.

Patch follows:

===
Subject: thp: mremap support and TLB optimization

From: Andrea Arcangeli <[email protected]>

This adds THP support to mremap (decreases the number of split_huge_page called).

This also replaces ptep_clear_flush with ptep_get_and_clear and replaces it with a final flush_tlb_range to send a single tlb flush IPI instead of one IPI for each page.

It also removes a bogus (even if harmless) "- 1" subtraction in the "next" calculation in move_page_tables().

Signed-off-by Andrea Arcangeli <[email protected]>

---

include/linux/huge_mm.h +5 -0

@@ -22,6 +22,11 @@ extern int zap_huge_pmd(struct mmu_gathe
 extern int mincore_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 			unsigned long addr, unsigned long end,
 			unsigned char *vec);
+extern int move_huge_pmd(struct vm_area_struct *vma,
+			 struct vm_area_struct *new_vma,
+			 unsigned long old_addr,
+			 unsigned long new_addr, unsigned long old_end,
+			 pmd_t *old_pmd, pmd_t *new_pmd);
 extern int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 			unsigned long addr, pgprot_t newprot);
 

mm/huge_memory.c +2 -0

@@ -1054,6 +1054,52 @@ int mincore_huge_pmd(struct vm_area_stru
 	return ret;
 }
 
+int move_huge_pmd(struct vm_area_struct *vma, struct vm_area_struct *new_vma,
+		  unsigned long old_addr,

mm/huge_memory.c +6 -0

+	spin_lock(&mm->page_table_lock);
+	if (likely(pmd_trans_huge(*old_pmd))) {
+		if (pmd_trans_splitting(*old_pmd)) {
+			spin_unlock(&mm->page_table_lock);
+			wait_split_huge_page(vma->anon_vma, old_pmd);
+			ret = -1;

mm/huge_memory.c +4 -0

+out:
+	return ret;
+}
+
 int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 		unsigned long addr, pgprot_t newprot)
 {

mm/mremap.c +0 -2

 	return pmd;

@@ -65,8 +64,6 @@ static pmd_t *alloc_new_pmd(struct mm_st
 		return NULL;
 
 	VM_BUG_ON(pmd_trans_huge(*pmd));
-	if (pmd_none(*pmd) && __pte_alloc(mm, vma, pmd, addr))
-		return NULL;
 
 	return pmd;
 }

mm/mremap.c +1 -1

 		pte = move_pte(pte, new_vma->vm_page_prot, old_addr, new_addr);
 		set_pte_at(mm, new_addr, new_pte, pte);
 	}

@@ -123,7 +116,6 @@ static void move_ptes(struct vm_area_str
 	pte_unmap_unlock(old_pte - 1, old_ptl);
 	if (mapping)
 		mutex_unlock(&mapping->i_mmap_mutex);
-	mmu_notifier_invalidate_range_end(vma->vm_mm, old_start, old_end);
 }
 
 #define LATENCY_LIMIT	(64 * PAGE_SIZE)

@@ -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);
 

mm/mremap.c +2 -0

@@ -150,6 +145,23 @@ unsigned long move_page_tables(struct vm
 		new_pmd = alloc_new_pmd(vma->vm_mm, vma, new_addr);
 		if (!new_pmd)
 			break;
+		if (pmd_trans_huge(*old_pmd)) {
+			int err = 0;

mm/mremap.c +5 -0

+		}
+		if (pmd_none(*new_pmd) && __pte_alloc(new_vma->vm_mm, new_vma,
+						      new_pmd,
+						      new_addr))
+			break;
 		next = (new_addr + PMD_SIZE) & PMD_MASK;
 		if (extent > next - new_addr)
 			extent = next - new_addr;

@@ -157,7 +169,12 @@ unsigned long move_page_tables(struct vm
 			extent = LATENCY_LIMIT;
 		move_ptes(vma, old_pmd, old_addr, old_addr + extent,

Quoted 8 lines

(quoted diff) +8 -0

+int move_huge_pmd(struct vm_area_struct *vma, struct vm_area_struct *new_vma,
+		  unsigned long old_addr,
+		  unsigned long new_addr, unsigned long old_end,
+		  pmd_t *old_pmd, pmd_t *new_pmd)
+{
+	int ret = 0;
+	pmd_t pmd;
+

Review

Quoted 4 lines

(quoted diff) +4 -0

+	struct mm_struct *mm = vma->vm_mm;
+
+	if ((old_addr & ~HPAGE_PMD_MASK) ||
+	    (new_addr & ~HPAGE_PMD_MASK) ||

How could these conditions ever be true? We are here because it was pmd_trans_huge. There should be no way this can be aligned. If this is paranoia, make it a BUG_ON.

Review

Quoted 2 lines

(quoted diff) +1 -0

+	    (old_addr + HPAGE_PMD_SIZE) > old_end ||

Again, is this possible? The old addr was already huge.

Quoted 6 lines

(quoted diff) +6 -0

+	/*
+	 * The destination pmd shouldn't be established, free_pgtables()
+	 * should have release it.
+	 */
+	if (!pmd_none(*new_pmd)) {
+		WARN_ON(1);

Review

Quoted 4 lines

(quoted diff) +4 -0

+		VM_BUG_ON(pmd_trans_huge(*new_pmd));
+		goto out;
+	}
+

Agreed that this should never happen. The mmap_sem is held for writing and we are remapping to what should be empty space. It should not be possible for a huge PMD to be established underneath us.

Quoted 6 lines

(quoted diff) +6 -0

+	spin_lock(&mm->page_table_lock);
+	if (likely(pmd_trans_huge(*old_pmd))) {
+		if (pmd_trans_splitting(*old_pmd)) {
+			spin_unlock(&mm->page_table_lock);
+			wait_split_huge_page(vma->anon_vma, old_pmd);
+			ret = -1;

Review

Quoted 10 lines

(quoted diff) +10 -0

+		} else {
+			pmd = pmdp_get_and_clear(mm, old_addr, old_pmd);
+			VM_BUG_ON(!pmd_none(*new_pmd));
+			set_pmd_at(mm, new_addr, new_pmd, pmd);
+			spin_unlock(&mm->page_table_lock);
+			ret = 1;
+		}
+	} else
+		spin_unlock(&mm->page_table_lock);
+

The meaning of the return values of -1, 0, 1 with the caller doing

if (err)
...
else if (!err)

	...

is tricky to work out. split_huge_page only needs to be called if returning 0. Would it be possible to have the split_huge_page called in this function? The end of the function would then look like

return ret;

out_split:
split_huge_page_pmd()
return ret;

with either success or failure being returned instead of a tristate which is easier to understand.

Quoted 6 lines

mm/mremap.c +0 -0

@@ -41,8 +41,7 @@ static pmd_t *get_old_pmd(struct mm_stru
 		return NULL;
 

mm/mremap.c static pmd_t *get_old_pmd(struct mm_stru · line 46

Quoted 6 lines

mm/mremap.c +1 -2

 	pmd = pmd_offset(pud, addr);
-	split_huge_page_pmd(mm, pmd);
-	if (pmd_none_or_clear_bad(pmd))
+	if (pmd_none(*pmd))
 		return NULL;
 

Ok, this is changing to pmd_none because it could be a huge PMD and pmd_none_or_clear_bad triggers on a huge PMD. Right?

Quoted 15 lines

mm/mremap.c +0 -3

@@ -65,8 +64,6 @@ static pmd_t *alloc_new_pmd(struct mm_st
 		return NULL;
 
 	VM_BUG_ON(pmd_trans_huge(*pmd));
-	if (pmd_none(*pmd) && __pte_alloc(mm, vma, pmd, addr))
-		return NULL;
 
 	return pmd;
 }

@@ -80,11 +77,7 @@ static void move_ptes(struct vm_area_str
 	struct mm_struct *mm = vma->vm_mm;
 	pte_t *old_pte, *new_pte, pte;
 	spinlock_t *old_ptl, *new_ptl;
-	unsigned long old_start;
 

mm/mremap.c static void move_ptes(struct vm_area_str · line 87

Quoted 3 lines

mm/mremap.c +0 -3

-	old_start = old_addr;
-	mmu_notifier_invalidate_range_start(vma->vm_mm,
-					    old_start, old_end);

The MMU notifier is now being called for a larger range. Previously it would usually be ranges of 64 pages and now it looks like it happens once for the entire range being remapped. This is not mentioned in the leader. What are the consequences of having a large gap between invalidate_start and invalidate_end? Would it be a big deal to call the MMU notifier within move_huge_pmd()?

If it's safe to use larger ranges, it would be preferable to see it in a separate patch or at the very least explained in the changelog.

mm/mremap.c static void move_ptes(struct vm_area_str · line 107

Quoted 9 lines

mm/mremap.c +1 -1

 	if (vma->vm_file) {
 		/*
 		 * Subtle point from Rajesh Venkatasubramanian: before

@@ -111,7 +104,7 @@ static void move_ptes(struct vm_area_str
 				   new_pte++, new_addr += PAGE_SIZE) {
 		if (pte_none(*old_pte))
 			continue;
-		pte = ptep_clear_flush(vma, old_addr, old_pte);
+		pte = ptep_get_and_clear(mm, old_addr, old_pte);

This looks like an unrelated optimisation. You hint at this in the patch subject but it needs a separate patch or a better explanation in the leader. If I'm reading this right, it looks like you are deferring a TLB flush on a single page and calling one call later at the end of move_page_tables. At a glance, that seems ok and would reduce IPIs but I'm not thinking about it properly because I'm trying to think about THP shenanigans :)

mm/mremap.c static void move_ptes(struct vm_area_str

Quoted 10 lines

mm/mremap.c +3 -1

+	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;
 		extent = next - old_addr;
 		old_pmd = get_old_pmd(vma->vm_mm, old_addr);

You asked if removing this "- 1" is correct. It's an overflow check for a situation where old_addr + PMD_SIZE overflows. On what architecture is it possible to call mremap() at the very top of the address space or am I missing the point?

Otherwise I think the existing check is harmless if obscure. It's reasonable to assume PAGE_SIZE will be > 1 and I'm not seeing why it is required by the rest of the patch.

Quoted 10 lines

mm/mremap.c +6 -0

@@ -150,6 +145,23 @@ unsigned long move_page_tables(struct vm
 		new_pmd = alloc_new_pmd(vma->vm_mm, vma, new_addr);
 		if (!new_pmd)
 			break;
+		if (pmd_trans_huge(*old_pmd)) {
+			int err = 0;
+			if (extent == HPAGE_PMD_SIZE)
+				err = move_huge_pmd(vma, new_vma, old_addr,
+						    new_addr, old_end,
+						    old_pmd, new_pmd);

mm/mremap.c unsigned long move_page_tables(struct vm · line 159

Quoted 6 lines

mm/mremap.c +6 -0

+			if (err > 0) {
+				need_flush = true;
+				continue;
+			} else if (!err)
+				split_huge_page_pmd(vma->vm_mm, old_pmd);
+			VM_BUG_ON(pmd_trans_huge(*old_pmd));

This tristate is hard to parse but I mentioned this already.

Functionally, I can't see a major problem with the patch. The minor problems are that I'd like to see that tristate replaced for readability, the optimisation better explained or in a separate patch and an explanation why the larger ranges for mmu_notifiers is not a problem.

Quoted 8 lines · Mel Gorman

Quoted 8 lines

(quoted diff) +8 -0

+int move_huge_pmd(struct vm_area_struct *vma, struct vm_area_struct *new_vma,
+		  unsigned long old_addr,
+		  unsigned long new_addr, unsigned long old_end,
+		  pmd_t *old_pmd, pmd_t *new_pmd)
+{
+	int ret = 0;
+	pmd_t pmd;
+

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	struct mm_struct *mm = vma->vm_mm;
+
+	if ((old_addr & ~HPAGE_PMD_MASK) ||
+	    (new_addr & ~HPAGE_PMD_MASK) ||

How could these conditions ever be true? We are here because it was pmd_trans_huge. There should be no way this can be aligned. If this is paranoia, make it a BUG_ON.

It actually happens. old_addr/new_addr aren't aligned to hpage beforehand, and they just from the mremap syscall parameters. Returning 0 is needed here.

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +1 -0

+	    (old_addr + HPAGE_PMD_SIZE) > old_end ||

Again, is this possible? The old addr was already huge.

This can happen too, old_end is also passed as parameter from syscall and it's not mangled to fit an hpage, just calculated as old_addr+len (len passed as parameter). The length of the destination vma shouldn't be necessary to check, there's no new_end in the first place as parameter of move_page_tables so the caller must ensure there's enough space to copy in the destination. And if the old_end-old_addr <= HPAGE_PMD_SIZE and the old_addr and new_addr are aligned, we're sure the hugepmd is safe to create on the destination new_addr (if it's aligned).

So I think all checks are needed here.

In theory the caller could
check this stuff before calling move_huge_pmd but I try to be as less invasive as possible in the common code that may be built with TRANSPARENT_HUGEPAGE=n (these checks would still be computed in the pmd_trans_huge(*old_pmd) case only but it's kind of nicer to hide them in huge_memory.c).

Quoted 6 lines

Quoted 6 lines

(quoted diff) +6 -0

+	/*
+	 * The destination pmd shouldn't be established, free_pgtables()
+	 * should have release it.
+	 */
+	if (!pmd_none(*new_pmd)) {
+		WARN_ON(1);

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+		VM_BUG_ON(pmd_trans_huge(*new_pmd));
+		goto out;
+	}
+

Agreed that this should never happen. The mmap_sem is held for writing and we are remapping to what should be empty space. It should not be possible for a huge PMD to be established underneath us.

Yes. The code will work fine also if the new_pmd points to a pte that wasn't freed by free_pgtables

but I think it's kind of safer to have a
WARN_ON because if that really ever happens we would prefer to release the pte here and proceed mapping the hugepmd instead of splitting the source hugepmd.

If there's a hugepmd mapped instead it means the memory wasn't munmapped. I could have run a mem compare of the pte against zero page too but I didn't.. kind of overkill. But checking that there is no hugepmd is fast.

Quoted 34 lines

Quoted 6 lines

(quoted diff) +6 -0

+	spin_lock(&mm->page_table_lock);
+	if (likely(pmd_trans_huge(*old_pmd))) {
+		if (pmd_trans_splitting(*old_pmd)) {
+			spin_unlock(&mm->page_table_lock);
+			wait_split_huge_page(vma->anon_vma, old_pmd);
+			ret = -1;

Review

Quoted 10 lines

(quoted diff) +10 -0

+		} else {
+			pmd = pmdp_get_and_clear(mm, old_addr, old_pmd);
+			VM_BUG_ON(!pmd_none(*new_pmd));
+			set_pmd_at(mm, new_addr, new_pmd, pmd);
+			spin_unlock(&mm->page_table_lock);
+			ret = 1;
+		}
+	} else
+		spin_unlock(&mm->page_table_lock);
+

The meaning of the return values of -1, 0, 1 with the caller doing

if (err)
...
else if (!err)

	...

is tricky to work out. split_huge_page only needs to be called if returning 0. Would it be possible to have the split_huge_page called in this function? The end of the function would then look like

return ret;

out_split:
split_huge_page_pmd()
return ret;

Quoted 2 lines

with either success or failure being returned instead of a tristate which is easier to understand.

So basically always return 0 regardless if it was a pmd_trans_splitting or if we splitted it ourself. And only return 1 in case the move_huge_pmd was successful.

I'm afraid we've other trestates returned for the other mm methods... if we cleanup this one later we may also cleanup the others.

I'll try to clean up this one for now ok.

Quoted 6 lines

Quoted 6 lines

mm/mremap.c +0 -0

@@ -41,8 +41,7 @@ static pmd_t *get_old_pmd(struct mm_stru
 		return NULL;
 

Quoted 9 lines

mm/mremap.c static pmd_t *get_old_pmd(struct mm_stru · line 46

Quoted 6 lines

mm/mremap.c +1 -2

 	pmd = pmd_offset(pud, addr);
-	split_huge_page_pmd(mm, pmd);
-	if (pmd_none_or_clear_bad(pmd))
+	if (pmd_none(*pmd))
 		return NULL;
 

Ok, this is changing to pmd_none because it could be a huge PMD and pmd_none_or_clear_bad triggers on a huge PMD. Right?

Right. It'd bug on with a hugepmd. Maybe we could someday change pmd_none_or_clear_bad not to choke on the PSE bit, but it was kind of safer to keep assuming the PSE bit was "bad" in case we forgotten some split_huge_page somewhere, better to bug in that case than to ignore.

But as time passes and THP is rock solid, I've to admit it's becoming more a lack of reliability to consider the PSE bit "bad" and to remove some pmd_none_clear_bad than an increase of reliability to validate the THP case. Maybe we should change that. It's a common problem not specific to mremap.

Quoted 15 lines

Quoted 15 lines

mm/mremap.c +0 -3

@@ -65,8 +64,6 @@ static pmd_t *alloc_new_pmd(struct mm_st
 		return NULL;
 
 	VM_BUG_ON(pmd_trans_huge(*pmd));
-	if (pmd_none(*pmd) && __pte_alloc(mm, vma, pmd, addr))
-		return NULL;
 
 	return pmd;
 }

@@ -80,11 +77,7 @@ static void move_ptes(struct vm_area_str
 	struct mm_struct *mm = vma->vm_mm;
 	pte_t *old_pte, *new_pte, pte;
 	spinlock_t *old_ptl, *new_ptl;
-	unsigned long old_start;
 

Quoted 10 lines

mm/mremap.c static void move_ptes(struct vm_area_str · line 87

Quoted 3 lines

mm/mremap.c +0 -3

-	old_start = old_addr;
-	mmu_notifier_invalidate_range_start(vma->vm_mm,
-					    old_start, old_end);

The MMU notifier is now being called for a larger range. Previously it would usually be ranges of 64 pages and now it looks like it happens once for the entire range being remapped. This is not mentioned in the leader. What are the consequences of having a large gap between invalidate_start and invalidate_end? Would it be a big deal to call the MMU notifier within move_huge_pmd()?

Well it should improve performance. The only downside is that it will stall secondary page faults for a longer time, but because the mremap can now run faster with fewer IPIs, I think overall it should improve performance.

Also it's probably not too common to do much mremap on apps with a secondary MMU attached so in this place the mmu notifier is more about correctness I think and correctness remains. Userland can always do mremap in smaller chunks if it needs and then it'll really run faster with no downside. After all no app is supposed to

touch the source addresses while they're being migrated as it could SEGFAULT at any time. So a very long mremap basically makes a mapping "not available" for a longer time, just now it'll be shorter because mremap will run faster. The mapping being available for the secondary mmu was incidental implementation detail, now it's not available to the secondary mmu during the move, like it is not available to userland. Trapping sigsegv to modify the mapping while it's under mremap I doubt anybody is depending on.

Quoted 2 lines

If it's safe to use larger ranges, it would be preferable to see it in a separate patch or at the very least explained in the changelog.

I can split it off to a separate patch. I knew I should have done that in the first place and rightfully I got caught :).

Quoted 10 lines

mm/mremap.c static void move_ptes(struct vm_area_str · line 107

Quoted 9 lines

mm/mremap.c +1 -1

 	if (vma->vm_file) {
 		/*
 		 * Subtle point from Rajesh Venkatasubramanian: before

@@ -111,7 +104,7 @@ static void move_ptes(struct vm_area_str
 				   new_pte++, new_addr += PAGE_SIZE) {
 		if (pte_none(*old_pte))
 			continue;
-		pte = ptep_clear_flush(vma, old_addr, old_pte);
+		pte = ptep_get_and_clear(mm, old_addr, old_pte);

Quoted 7 lines

This looks like an unrelated optimisation. You hint at this in the patch subject but it needs a separate patch or a better explanation in the leader. If I'm reading this right, it looks like you are deferring a TLB flush on a single page and calling one call later at the end of move_page_tables. At a glance, that seems ok and would reduce IPIs but I'm not thinking about it properly because I'm trying to think about THP shenanigans :)

That's exactly what it does. Once I split it off you can concentrate on the two parts separately. This is also the parts that requires moving the mmu notifier outside along with the tlb flush outside.

The THP shenanigans don't require moving the mmu notifier outside.

The one IPI per page is a major bottleneck for java, lack of hugepmd migrate also major bottleneck, here we get both combined so we get 1 IPI for a ton of THP. The benchmark I run was single threaded on a 12 core system (and single threaded if scheduler is doing good won't require any IPI), you can only imagine the boost it gets on heavily multithreaded apps that requires flooding IPI on large SMP (I didn't measure that as I was already happy with what I got single threaded :).

Quoted 16 lines

mm/mremap.c unsigned long move_page_tables(struct vm · line 143

Quoted 10 lines

mm/mremap.c +3 -1

+	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;
 		extent = next - old_addr;
 		old_pmd = get_old_pmd(vma->vm_mm, old_addr);

You asked if removing this "- 1" is correct. It's an overflow check for a situation where old_addr + PMD_SIZE overflows. On what architecture is it possible to call mremap() at the very top of the address space or am I missing the point?

Quoted 3 lines

Otherwise I think the existing check is harmless if obscure. It's reasonable to assume PAGE_SIZE will be > 1 and I'm not seeing why it is required by the rest of the patch.

An arch where the -TASK_SIZE is less than PMD_SIZE didn't cross my mind sorry, Johannes also pointed it out. I'd find this more readable than a off by one -1 that looks erroneous.

mm/mremap.c +3 -2

@@ -136,9 +136,10 @@ unsigned long move_page_tables(struct vm
 	for (; old_addr < old_end; old_addr += extent, new_addr += extent) {
 		cond_resched();
 		next = (old_addr + PMD_SIZE) & PMD_MASK;
-		if (next > old_end)
-			next = old_end;
+		/* even if next overflowed, extent below will be ok */
 		extent = next - old_addr;
+		if (extent > old_end - old_addr)
+			extent = old_end - old_addr;
 		old_pmd = get_old_pmd(vma->vm_mm, old_addr);
 		if (!old_pmd)
 			continue;

What do you think? I can put the -1 back if you prefer. I doubt the speed difference can matter here, all values should be in registers.

Quoted 10 lines

Quoted 10 lines

mm/mremap.c +6 -0

@@ -150,6 +145,23 @@ unsigned long move_page_tables(struct vm
 		new_pmd = alloc_new_pmd(vma->vm_mm, vma, new_addr);
 		if (!new_pmd)
 			break;
+		if (pmd_trans_huge(*old_pmd)) {
+			int err = 0;
+			if (extent == HPAGE_PMD_SIZE)
+				err = move_huge_pmd(vma, new_vma, old_addr,
+						    new_addr, old_end,
+						    old_pmd, new_pmd);

Quoted 8 lines

mm/mremap.c unsigned long move_page_tables(struct vm · line 159

Quoted 6 lines

mm/mremap.c +6 -0

+			if (err > 0) {
+				need_flush = true;
+				continue;
+			} else if (!err)
+				split_huge_page_pmd(vma->vm_mm, old_pmd);
+			VM_BUG_ON(pmd_trans_huge(*old_pmd));

This tristate is hard to parse but I mentioned this already.

Yep I'll try to make it 0/1.

Quoted 5 lines

Functionally, I can't see a major problem with the patch. The minor problems are that I'd like to see that tristate replaced for readability, the optimisation better explained or in a separate patch and an explanation why the larger ranges for mmu_notifiers is not a problem.

Thanks for the review, very helpful as usual, I'll try to submit a 3 patches version with the cleanups you suggested soon enough. Ideally I'd like to replace the -1 with the above change that also should guard against TASK_SIZE ending less than one pmd away from the end of the address space if you like it.

That's a tough call. Increased jitter within KVM might be unwelcome but ordinarily the only people that might care about latencies are real time people and I dont think they would care about the KVM case.

I agree with you that overall it's probably for the best but splitting it out as a separate patch and cc'ing the KVM people would do no harm. Is there any impact for things like xpmem that might be using MMU notifiers in some creative manner?

Quoted 8 lines

The one IPI per page is a major bottleneck for java, lack of hugepmd migrate also major bottleneck, here we get both combined so we get 1 IPI for a ton of THP. The benchmark I run was single threaded on a 12 core system (and single threaded if scheduler is doing good won't require any IPI), you can only imagine the boost it gets on heavily multithreaded apps that requires flooding IPI on large SMP (I didn't measure that as I was already happy with what I got single threaded :).

Yeah, I imagine it should also be a boost during GC if the JVM is using mremap to migrate full pages from an old heap to a new one.

I don't feel very strongly about it. The change looks reasonable and with a fresh head with the patch split out, it probably is more readable.

The meaning of the return values of -1, 0, 1 with the caller doing

if (err)
...
else if (!err)

	...

is tricky to work out. split_huge_page only needs to be called if returning 0. Would it be possible to have the split_huge_page called in this function? The end of the function would then look like