mm: faster get user pages

PATCH In review Keith Busch 3 participants 15 messages Sep 21, 2018 – Oct 2, 2018

Subsystems (6)

Memory management Kselftest MM core Memory mapping +2 more subsystems

lore

Share & tools

lore

Keith Busch

author

2018-09-21 22:40 UTC #0 lore raw

v3 cover mm: faster get user pages

Changes since v2:

Combine only the output parameters in a struct that need tracking, and squash to just one final kernel patch.

Fixed compile bugs for all configs

Keith Busch (6):
mm/gup_benchmark: Time put_page mm/gup_benchmark: Add additional pinning methods tools/gup_benchmark: Fix 'write' flag usage tools/gup_benchmark: Allow user specified file tools/gup_benchmark: Add parameter for hugetlb mm/gup: Cache dev_pagemap while pinning pages

 include/linux/huge_mm.h                    |  8 +--
 include/linux/mm.h                         | 19 ++++++-
 mm/gup.c                                   | 90 +++++++++++++++++-------------
 mm/gup_benchmark.c                         | 36 ++++++++++--
 mm/huge_memory.c                           | 38 ++++++-------
 mm/nommu.c                                 |  4 +-
 tools/testing/selftests/vm/gup_benchmark.c | 40 +++++++++++--
 7 files changed, 154 insertions(+), 81 deletions(-)

Signature · 1 line

2.14.4

Keith Busch

author

2018-09-21 22:40 UTC #1 lore raw

PATCH v3 1/6 mm/gup_benchmark: Time put_page

We'd like to measure time to unpin user pages, so this adds a second benchmark timer on put_page, separate from get_page.

Adding the field will breaks this ioctl ABI, but should be okay since this an in-tree kernel selftest.

Cc Kirill Shutemov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---
 mm/gup_benchmark.c                         | 8 ++++++--
 tools/testing/selftests/vm/gup_benchmark.c | 6 ++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

mm/gup_benchmark.c +6 -2

index 6a473709e9b6..76cd35e477af 100644
@@ -8,7 +8,8 @@
 #define GUP_FAST_BENCHMARK	_IOWR('g', 1, struct gup_benchmark)
 
 struct gup_benchmark {
-	__u64 delta_usec;
+	__u64 get_delta_usec;
+	__u64 put_delta_usec;
 	__u64 addr;
 	__u64 size;
 	__u32 nr_pages_per_call;

@@ -47,14 +48,17 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
 	}
 	end_time = ktime_get();
 
-	gup->delta_usec = ktime_us_delta(end_time, start_time);
+	gup->get_delta_usec = ktime_us_delta(end_time, start_time);
 	gup->size = addr - gup->addr;
 
+	start_time = ktime_get();
 	for (i = 0; i < nr_pages; i++) {
 		if (!pages[i])
 			break;
 		put_page(pages[i]);
 	}
+	end_time = ktime_get();
+	gup->put_delta_usec = ktime_us_delta(end_time, start_time);
 
 	kvfree(pages);
 	return 0;

tools/testing/selftests/vm/gup_benchmark.c +4 -2

index 36df55132036..bdcb97acd0ac 100644
@@ -17,7 +17,8 @@
 #define GUP_FAST_BENCHMARK	_IOWR('g', 1, struct gup_benchmark)
 
 struct gup_benchmark {
-	__u64 delta_usec;
+	__u64 get_delta_usec;
+	__u64 put_delta_usec;
 	__u64 addr;
 	__u64 size;
 	__u32 nr_pages_per_call;

@@ -81,7 +82,8 @@ int main(int argc, char **argv)
 		if (ioctl(fd, GUP_FAST_BENCHMARK, &gup))
 			perror("ioctl"), exit(1);
 
-		printf("Time: %lld us", gup.delta_usec);
+		printf("Time: get:%lld put:%lld us", gup.get_delta_usec,
+			gup.put_delta_usec);
 		if (gup.size != size)
 			printf(", truncated (size: %lld)", gup.size);
 		printf("\n");

Signature · 1 line

2.14.4

Kirill A. Shutemov

Acked-by on v3 1/6

2018-10-02 10:54 UTC #2 lore raw

Re: [PATCHv3 1/6] mm/gup_benchmark: Time put_page

Quoted 10 lines · Keith Busch

We'd like to measure time to unpin user pages, so this adds a second benchmark timer on put_page, separate from get_page.
Adding the field will breaks this ioctl ABI, but should be okay since this an in-tree kernel selftest.
Cc Kirill Shutemov <[email protected]>
Cc Dave Hansen <[email protected]>
Cc Dan Williams <[email protected]>
Signed-off-by Keith Busch <[email protected]>

Acked-by Kirill A. Shutemov <[email protected]>

Signature · 1 line

 Kirill A. Shutemov

Keith Busch

author

2018-09-21 22:40 UTC #3 lore raw

PATCH v3 2/6 mm/gup_benchmark: Add additional pinning methods

In reply to Keith Busch

This patch provides new gup benchmark ioctl commands to run different user page pinning methods, get_user_pages_longterm and get_user_pages, in addition to the existing get_user_pages_fast.

Cc Kirill Shutemov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---
 mm/gup_benchmark.c                         | 28 ++++++++++++++++++++++++++--
 tools/testing/selftests/vm/gup_benchmark.c | 13 +++++++++++--
 2 files changed, 37 insertions(+), 4 deletions(-)

mm/gup_benchmark.c +26 -2

index 76cd35e477af..e6d9ce001ffa 100644
@@ -6,6 +6,8 @@
 #include <linux/debugfs.h>
 
 #define GUP_FAST_BENCHMARK	_IOWR('g', 1, struct gup_benchmark)
+#define GUP_LONGTERM_BENCHMARK	_IOWR('g', 2, struct gup_benchmark)
+#define GUP_BENCHMARK		_IOWR('g', 3, struct gup_benchmark)
 
 struct gup_benchmark {
 	__u64 get_delta_usec;

@@ -41,7 +43,23 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
 			nr = (next - addr) / PAGE_SIZE;
 		}
 
-		nr = get_user_pages_fast(addr, nr, gup->flags & 1, pages + i);
+		switch (cmd) {
+		case GUP_FAST_BENCHMARK:
+			nr = get_user_pages_fast(addr, nr, gup->flags & 1,
+						 pages + i);
+			break;
+		case GUP_LONGTERM_BENCHMARK:
+			nr = get_user_pages_longterm(addr, nr, gup->flags & 1,
+						     pages + i, NULL);
+			break;
+		case GUP_BENCHMARK:
+			nr = get_user_pages(addr, nr, gup->flags & 1, pages + i,
+					    NULL);
+			break;
+		default:
+			return -1;
+		}
+
 		if (nr <= 0)
 			break;
 		i += nr;

@@ -70,8 +88,14 @@ static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,
 	struct gup_benchmark gup;
 	int ret;
 
-	if (cmd != GUP_FAST_BENCHMARK)
+	switch (cmd) {
+	case GUP_FAST_BENCHMARK:
+	case GUP_LONGTERM_BENCHMARK:
+	case GUP_BENCHMARK:
+		break;
+	default:
 		return -EINVAL;
+	}
 
 	if (copy_from_user(&gup, (void __user *)arg, sizeof(gup)))
 		return -EFAULT;

tools/testing/selftests/vm/gup_benchmark.c +11 -2

index bdcb97acd0ac..c2f785ded9b9 100644
@@ -15,6 +15,8 @@
 #define PAGE_SIZE sysconf(_SC_PAGESIZE)
 
 #define GUP_FAST_BENCHMARK	_IOWR('g', 1, struct gup_benchmark)
+#define GUP_LONGTERM_BENCHMARK	_IOWR('g', 2, struct gup_benchmark)
+#define GUP_BENCHMARK		_IOWR('g', 3, struct gup_benchmark)
 
 struct gup_benchmark {
 	__u64 get_delta_usec;

@@ -30,9 +32,10 @@ int main(int argc, char **argv)
 	struct gup_benchmark gup;
 	unsigned long size = 128 * MB;
 	int i, fd, opt, nr_pages = 1, thp = -1, repeats = 1, write = 0;
+	int cmd = GUP_FAST_BENCHMARK;
 	char *p;
 
-	while ((opt = getopt(argc, argv, "m:r:n:tT")) != -1) {
+	while ((opt = getopt(argc, argv, "m:r:n:tTLU")) != -1) {
 		switch (opt) {
 		case 'm':
 			size = atoi(optarg) * MB;

@@ -49,6 +52,12 @@ int main(int argc, char **argv)
 		case 'T':
 			thp = 0;
 			break;
+		case 'L':
+			cmd = GUP_LONGTERM_BENCHMARK;
+			break;
+		case 'U':
+			cmd = GUP_BENCHMARK;
+			break;
 		case 'w':
 			write = 1;
 		default:

@@ -79,7 +88,7 @@ int main(int argc, char **argv)
 
 	for (i = 0; i < repeats; i++) {
 		gup.size = size;
-		if (ioctl(fd, GUP_FAST_BENCHMARK, &gup))
+		if (ioctl(fd, cmd, &gup))
 			perror("ioctl"), exit(1);
 
 		printf("Time: get:%lld put:%lld us", gup.get_delta_usec,

Signature · 1 line

2.14.4

Kirill A. Shutemov

Acked-by on v3 2/6

2018-10-02 10:56 UTC #4 lore raw

Re: [PATCHv3 2/6] mm/gup_benchmark: Add additional pinning methods

Quoted 8 lines · Keith Busch

This patch provides new gup benchmark ioctl commands to run different user page pinning methods, get_user_pages_longterm and get_user_pages, in addition to the existing get_user_pages_fast.
Cc Kirill Shutemov <[email protected]>
Cc Dave Hansen <[email protected]>
Cc Dan Williams <[email protected]>
Signed-off-by Keith Busch <[email protected]>

Acked-by Kirill A. Shutemov <[email protected]>

Signature · 1 line

 Kirill A. Shutemov

Keith Busch

author

2018-09-21 22:40 UTC #5 lore raw

PATCH v3 3/6 tools/gup_benchmark: Fix 'write' flag usage

In reply to Keith Busch

If the '-w' parameter was provided, the benchmark would exit due to a mssing 'break'.

Cc Kirill Shutemov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---
 tools/testing/selftests/vm/gup_benchmark.c | 1 +
 1 file changed, 1 insertion(+)

tools/testing/selftests/vm/gup_benchmark.c +1 -0

index c2f785ded9b9..b2082df8beb4 100644
@@ -60,6 +60,7 @@ int main(int argc, char **argv)
 			break;
 		case 'w':
 			write = 1;
+			break;
 		default:
 			return -1;
 		}

Signature · 1 line

2.14.4

Kirill A. Shutemov

Acked-by on v3 3/6

2018-10-02 10:57 UTC #6 lore raw

Re: [PATCHv3 3/6] tools/gup_benchmark: Fix 'write' flag usage

Quoted 7 lines · Keith Busch

If the '-w' parameter was provided, the benchmark would exit due to a mssing 'break'.
Cc Kirill Shutemov <[email protected]>
Cc Dave Hansen <[email protected]>
Cc Dan Williams <[email protected]>
Signed-off-by Keith Busch <[email protected]>

Acked-by Kirill A. Shutemov <[email protected]>

Signature · 1 line

 Kirill A. Shutemov

Keith Busch

author

2018-09-21 22:40 UTC #7 lore raw

PATCH v3 4/6 tools/gup_benchmark: Allow user specified file

In reply to Keith Busch

The gup benchmark by default maps anonymous memory. This patch allows a user to specify a file to map, providing a means to test various file backings, like device and filesystem DAX.

Cc Kirill Shutemov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---
 tools/testing/selftests/vm/gup_benchmark.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

tools/testing/selftests/vm/gup_benchmark.c +16 -3

index b2082df8beb4..f2c99e2436f8 100644
@@ -33,9 +33,12 @@ int main(int argc, char **argv)
 	unsigned long size = 128 * MB;
 	int i, fd, opt, nr_pages = 1, thp = -1, repeats = 1, write = 0;
 	int cmd = GUP_FAST_BENCHMARK;
+	int file_map = -1;
+	int flags = MAP_ANONYMOUS | MAP_PRIVATE;
+	char *file = NULL;
 	char *p;
 
-	while ((opt = getopt(argc, argv, "m:r:n:tTLU")) != -1) {
+	while ((opt = getopt(argc, argv, "m:r:n:f:tTLU")) != -1) {
 		switch (opt) {
 		case 'm':
 			size = atoi(optarg) * MB;

@@ -61,11 +64,22 @@ int main(int argc, char **argv)
 		case 'w':
 			write = 1;
 			break;
+		case 'f':
+			file = optarg;
+			flags &= ~(MAP_PRIVATE | MAP_ANONYMOUS);
+			flags |= MAP_SHARED;
+			break;
 		default:
 			return -1;
 		}
 	}
 
+	if (file) {
+		file_map = open(file, O_RDWR|O_CREAT);
+		if (file_map < 0)
+			perror("open"), exit(file_map);
+	}
+
 	gup.nr_pages_per_call = nr_pages;
 	gup.flags = write;

@@ -73,8 +87,7 @@ int main(int argc, char **argv)
 	if (fd == -1)
 		perror("open"), exit(1);
 
-	p = mmap(NULL, size, PROT_READ | PROT_WRITE,
-			MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+	p = mmap(NULL, size, PROT_READ | PROT_WRITE, flags, file_map, 0);
 	if (p == MAP_FAILED)
 		perror("mmap"), exit(1);
 	gup.addr = (unsigned long)p;

Signature · 1 line

2.14.4

Kirill A. Shutemov

2018-10-02 11:03 UTC #8 lore raw

Re: [PATCHv3 4/6] tools/gup_benchmark: Allow user specified file

Quoted 31 lines · Keith Busch

The gup benchmark by default maps anonymous memory. This patch allows a user to specify a file to map, providing a means to test various file backings, like device and filesystem DAX.
Cc Kirill Shutemov <[email protected]>
Cc Dave Hansen <[email protected]>
Cc Dan Williams <[email protected]>
Signed-off-by Keith Busch <[email protected]>
---
 tools/testing/selftests/vm/gup_benchmark.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
tools/testing/selftests/vm/gup_benchmark.c +4 -1
index b2082df8beb4..f2c99e2436f8 100644
@@ -33,9 +33,12 @@ int main(int argc, char **argv)
 	unsigned long size = 128 * MB;
 	int i, fd, opt, nr_pages = 1, thp = -1, repeats = 1, write = 0;
 	int cmd = GUP_FAST_BENCHMARK;
+	int file_map = -1;
+	int flags = MAP_ANONYMOUS | MAP_PRIVATE;
+	char *file = NULL;
 	char *p;
 
-	while ((opt = getopt(argc, argv, "m:r:n:tTLU")) != -1) {
+	while ((opt = getopt(argc, argv, "m:r:n:f:tTLU")) != -1) {
 		switch (opt) {
 		case 'm':
 			size = atoi(optarg) * MB;

tools/testing/selftests/vm/gup_benchmark.c int main(int argc, char **argv) · line 70

Quoted 7 lines

tools/testing/selftests/vm/gup_benchmark.c +4 -0

@@ -61,11 +64,22 @@ int main(int argc, char **argv)
 		case 'w':
 			write = 1;
 			break;
+		case 'f':
+			file = optarg;
+			flags &= ~(MAP_PRIVATE | MAP_ANONYMOUS);
+			flags |= MAP_SHARED;

Why do we want to assume shared mapping if a file is passed? Private-file mapping is also valid target for the benchmark.

Maybe a separate option for shared? It would keep options more independent.

BTW, we can make a default file /dev/zero and don't have MAP_ANONYMOUS in the flags: private mapping of /dev/zero would produce anonymous mapping. No need in masking out MAP_ANONYMOUS on -f and no branch on 'if (file)' below.

Quoted 28 lines

tools/testing/selftests/vm/gup_benchmark.c +8 -2

+			break;
 		default:
 			return -1;
 		}
 	}
 
+	if (file) {
+		file_map = open(file, O_RDWR|O_CREAT);
+		if (file_map < 0)
+			perror("open"), exit(file_map);
+	}
+
 	gup.nr_pages_per_call = nr_pages;
 	gup.flags = write;

@@ -73,8 +87,7 @@ int main(int argc, char **argv)
 	if (fd == -1)
 		perror("open"), exit(1);
 
-	p = mmap(NULL, size, PROT_READ | PROT_WRITE,
-			MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+	p = mmap(NULL, size, PROT_READ | PROT_WRITE, flags, file_map, 0);
 	if (p == MAP_FAILED)
 		perror("mmap"), exit(1);
 	gup.addr = (unsigned long)p;

Signature · 1 line

2.14.4

Signature · 1 line

 Kirill A. Shutemov

Keith Busch

author

2018-09-21 22:40 UTC #9 lore raw

PATCH v3 5/6 tools/gup_benchmark: Add parameter for hugetlb

In reply to Keith Busch

Cc Kirill Shutemov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---
 tools/testing/selftests/vm/gup_benchmark.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

tools/testing/selftests/vm/gup_benchmark.c +4 -1

index f2c99e2436f8..5d96e2b3d2f1 100644
@@ -38,7 +38,7 @@ int main(int argc, char **argv)
 	char *file = NULL;
 	char *p;
 
-	while ((opt = getopt(argc, argv, "m:r:n:f:tTLU")) != -1) {
+	while ((opt = getopt(argc, argv, "m:r:n:f:tTLUH")) != -1) {
 		switch (opt) {
 		case 'm':
 			size = atoi(optarg) * MB;

@@ -64,6 +64,9 @@ int main(int argc, char **argv)
 		case 'w':
 			write = 1;
 			break;
+		case 'H':
+			flags |= MAP_HUGETLB;
+			break;
 		case 'f':
 			file = optarg;
 			flags &= ~(MAP_PRIVATE | MAP_ANONYMOUS);

Signature · 1 line

2.14.4

Kirill A. Shutemov

2018-10-02 11:05 UTC #10 lore raw

Re: [PATCHv3 5/6] tools/gup_benchmark: Add parameter for hugetlb

On Fri, Sep 21, 2018 at 10:39:55PM +0000, Keith Busch wrote:

-ENOMSG

Quoted 34 lines

Cc Kirill Shutemov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---
 tools/testing/selftests/vm/gup_benchmark.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

tools/testing/selftests/vm/gup_benchmark.c +4 -1

index f2c99e2436f8..5d96e2b3d2f1 100644
@@ -38,7 +38,7 @@ int main(int argc, char **argv)
 	char *file = NULL;
 	char *p;
 
-	while ((opt = getopt(argc, argv, "m:r:n:f:tTLU")) != -1) {
+	while ((opt = getopt(argc, argv, "m:r:n:f:tTLUH")) != -1) {
 		switch (opt) {
 		case 'm':
 			size = atoi(optarg) * MB;

@@ -64,6 +64,9 @@ int main(int argc, char **argv)
 		case 'w':
 			write = 1;
 			break;
+		case 'H':
+			flags |= MAP_HUGETLB;
+			break;
 		case 'f':
 			file = optarg;
 			flags &= ~(MAP_PRIVATE | MAP_ANONYMOUS);

Signature · 1 line

2.14.4

Signature · 1 line

 Kirill A. Shutemov

Keith Busch

author

2018-09-21 22:40 UTC #11 lore raw

PATCH v3 6/6 mm/gup: Cache dev_pagemap while pinning pages

In reply to Keith Busch

Pinning pages from ZONE_DEVICE memory needs to check the backing device's live-ness, which is tracked in the device's dev_pagemap metadata. This metadata is stored in a radix tree and looking it up adds measurable software overhead.

This patch avoids repeating this relatively costly operation when dev_pagemap is used by caching the last dev_pagemap when getting user pages. The gup_benchmark reports this reduces the time to get user pages to as low as 1/3 of the previous time.

The cached value is combined with other output parameters into a context struct to keep the parameters fewer.

Cc Kirill Shutemov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---
 include/linux/huge_mm.h |  8 ++---
 include/linux/mm.h      | 19 +++++++++--
 mm/gup.c                | 90 +++++++++++++++++++++++++++----------------------
 mm/huge_memory.c        | 38 +++++++++------------
 mm/nommu.c              |  4 +--
 5 files changed, 88 insertions(+), 71 deletions(-)

include/linux/huge_mm.h +4 -4

index 99c19b06d9a4..5cbabdebe9af 100644
@@ -213,9 +213,9 @@ static inline int hpage_nr_pages(struct page *page)
 }
 
 struct page *follow_devmap_pmd(struct vm_area_struct *vma, unsigned long addr,
-		pmd_t *pmd, int flags);
+		pmd_t *pmd, int flags, struct dev_pagemap **pgmap);
 struct page *follow_devmap_pud(struct vm_area_struct *vma, unsigned long addr,
-		pud_t *pud, int flags);
+		pud_t *pud, int flags, struct dev_pagemap **pgmap);
 
 extern vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf, pmd_t orig_pmd);

@@ -344,13 +344,13 @@ static inline void mm_put_huge_zero_page(struct mm_struct *mm)
 }
 
 static inline struct page *follow_devmap_pmd(struct vm_area_struct *vma,
-		unsigned long addr, pmd_t *pmd, int flags)
+	unsigned long addr, pmd_t *pmd, int flags, struct dev_pagemap **pgmap)
 {
 	return NULL;
 }
 
 static inline struct page *follow_devmap_pud(struct vm_area_struct *vma,
-		unsigned long addr, pud_t *pud, int flags)
+	unsigned long addr, pud_t *pud, int flags, struct dev_pagemap **pgmap)
 {
 	return NULL;
 }

include/linux/mm.h +16 -3

index a61ebe8ad4ca..79c80496dd50 100644
@@ -2534,15 +2534,28 @@ static inline vm_fault_t vmf_error(int err)
 	return VM_FAULT_SIGBUS;
 }
 
+struct follow_page_context {
+	struct dev_pagemap *pgmap;
+	unsigned int page_mask;
+};
+
 struct page *follow_page_mask(struct vm_area_struct *vma,
 			      unsigned long address, unsigned int foll_flags,
-			      unsigned int *page_mask);
+			      struct follow_page_context *ctx);
 
 static inline struct page *follow_page(struct vm_area_struct *vma,
 		unsigned long address, unsigned int foll_flags)
 {
-	unsigned int unused_page_mask;
-	return follow_page_mask(vma, address, foll_flags, &unused_page_mask);
+	struct page *page;
+	struct follow_page_context ctx = {
+		.pgmap = NULL,
+		.page_mask = 0,
+	};
+
+	page = follow_page_mask(vma, address, foll_flags, &ctx);
+	if (ctx.pgmap)
+		put_dev_pagemap(ctx.pgmap);
+	return page;
 }
 
 #define FOLL_WRITE	0x01	/* check pte is writable */

mm/gup.c +50 ~~-40~~

240 lines not on this page · raw message

mm/huge_memory.c +16 ~~-22~~

index 533f9b00147d..9839bf91b057 100644
@@ -851,13 +851,23 @@ static void touch_pmd(struct vm_area_struct *vma, unsigned long addr,
 		update_mmu_cache_pmd(vma, addr, pmd);
 }
 
+static struct page *pagemap_page(unsigned long pfn, struct dev_pagemap **pgmap)
+{
+	struct page *page;
+
+	*pgmap = get_dev_pagemap(pfn, *pgmap);
+	if (!*pgmap)
+		return ERR_PTR(-EFAULT);
+	page = pfn_to_page(pfn);
+	get_page(page);
+	return page;
+}
+
 struct page *follow_devmap_pmd(struct vm_area_struct *vma, unsigned long addr,
-		pmd_t *pmd, int flags)
+		pmd_t *pmd, int flags, struct dev_pagemap **pgmap)
 {
 	unsigned long pfn = pmd_pfn(*pmd);
 	struct mm_struct *mm = vma->vm_mm;
-	struct dev_pagemap *pgmap;
-	struct page *page;
 
 	assert_spin_locked(pmd_lockptr(mm, pmd));

@@ -886,14 +896,7 @@ struct page *follow_devmap_pmd(struct vm_area_struct *vma, unsigned long addr,
 		return ERR_PTR(-EEXIST);
 
 	pfn += (addr & ~PMD_MASK) >> PAGE_SHIFT;
-	pgmap = get_dev_pagemap(pfn, NULL);
-	if (!pgmap)
-		return ERR_PTR(-EFAULT);
-	page = pfn_to_page(pfn);
-	get_page(page);
-	put_dev_pagemap(pgmap);
-
-	return page;
+	return pagemap_page(pfn, pgmap);
 }
 
 int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,

@@ -1000,12 +1003,10 @@ static void touch_pud(struct vm_area_struct *vma, unsigned long addr,
 }
 
 struct page *follow_devmap_pud(struct vm_area_struct *vma, unsigned long addr,
-		pud_t *pud, int flags)
+               pud_t *pud, int flags, struct dev_pagemap **pgmap)
 {
 	unsigned long pfn = pud_pfn(*pud);
 	struct mm_struct *mm = vma->vm_mm;
-	struct dev_pagemap *pgmap;
-	struct page *page;
 
 	assert_spin_locked(pud_lockptr(mm, pud));

@@ -1028,14 +1029,7 @@ struct page *follow_devmap_pud(struct vm_area_struct *vma, unsigned long addr,
 		return ERR_PTR(-EEXIST);
 
 	pfn += (addr & ~PUD_MASK) >> PAGE_SHIFT;
-	pgmap = get_dev_pagemap(pfn, NULL);
-	if (!pgmap)
-		return ERR_PTR(-EFAULT);
-	page = pfn_to_page(pfn);
-	get_page(page);
-	put_dev_pagemap(pgmap);
-
-	return page;
+	return pagemap_page(pfn, pgmap);
 }
 
 int copy_huge_pud(struct mm_struct *dst_mm, struct mm_struct *src_mm,

mm/nommu.c +2 -2

index e4aac33216ae..a795c70cf21e 100644
@@ -1711,9 +1711,9 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
 
 struct page *follow_page_mask(struct vm_area_struct *vma,
 			      unsigned long address, unsigned int flags,
-			      unsigned int *page_mask)
+			      struct follow_page_context *ctx)
 {
-	*page_mask = 0;
+	ctx->page_mask = 0;
 	return NULL;
 }

Signature · 1 line

2.14.4

Kirill A. Shutemov

2018-10-02 11:26 UTC #12 lore raw

Re: [PATCHv3 6/6] mm/gup: Cache dev_pagemap while pinning pages

Quoted 18 lines · Keith Busch

Pinning pages from ZONE_DEVICE memory needs to check the backing device's live-ness, which is tracked in the device's dev_pagemap metadata. This metadata is stored in a radix tree and looking it up adds measurable software overhead.
This patch avoids repeating this relatively costly operation when dev_pagemap is used by caching the last dev_pagemap when getting user pages. The gup_benchmark reports this reduces the time to get user pages to as low as 1/3 of the previous time.
The cached value is combined with other output parameters into a context struct to keep the parameters fewer.
Cc Kirill Shutemov <[email protected]>
Cc Dave Hansen <[email protected]>
Cc Dan Williams <[email protected]>
Signed-off-by Keith Busch <[email protected]>
---

....

Quoted 24 lines

include/linux/mm.h +7 -3

index a61ebe8ad4ca..79c80496dd50 100644
@@ -2534,15 +2534,28 @@ static inline vm_fault_t vmf_error(int err)
 	return VM_FAULT_SIGBUS;
 }
 
+struct follow_page_context {
+	struct dev_pagemap *pgmap;
+	unsigned int page_mask;
+};
+
 struct page *follow_page_mask(struct vm_area_struct *vma,
 			      unsigned long address, unsigned int foll_flags,
-			      unsigned int *page_mask);
+			      struct follow_page_context *ctx);
 
 static inline struct page *follow_page(struct vm_area_struct *vma,
 		unsigned long address, unsigned int foll_flags)
 {
-	unsigned int unused_page_mask;
-	return follow_page_mask(vma, address, foll_flags, &unused_page_mask);
+	struct page *page;

include/linux/mm.h static inline vm_fault_t vmf_error(int err) · line 2559

Quoted 10 lines

include/linux/mm.h +9 -0

+	struct follow_page_context ctx = {
+		.pgmap = NULL,
+		.page_mask = 0,
+	};
+
+	page = follow_page_mask(vma, address, foll_flags, &ctx);
+	if (ctx.pgmap)
+		put_dev_pagemap(ctx.pgmap);
+	return page;
 }

Do we still want to keep the function as inline? I don't think so. Let's move it into mm/gup.c and make struct follow_page_context private to the file.

Quoted 20 lines

include/linux/mm.h +0 -0

 #define FOLL_WRITE	0x01	/* check pte is writable */

mm/gup.c +2 -2

index 1abc8b4afff6..124e7293e381 100644
@@ -71,10 +71,10 @@ static inline bool can_follow_write_pte(pte_t pte, unsigned int flags)
 }
 
 static struct page *follow_page_pte(struct vm_area_struct *vma,
-		unsigned long address, pmd_t *pmd, unsigned int flags)
+		unsigned long address, pmd_t *pmd, unsigned int flags,
+		struct dev_pagemap **pgmap)
 {
 	struct mm_struct *mm = vma->vm_mm;
-	struct dev_pagemap *pgmap = NULL;
 	struct page *page;
 	spinlock_t *ptl;
 	pte_t *ptep, pte;

mm/gup.c static struct page *follow_page_pte(struct vm_area_struct *vma, · line 123

Quoted 10 lines

mm/gup.c +2 -2

@@ -116,8 +116,8 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
 		 * Only return device mapping pages in the FOLL_GET case since
 		 * they are only valid while holding the pgmap reference.
 		 */
-		pgmap = get_dev_pagemap(pte_pfn(pte), NULL);
-		if (pgmap)
+		*pgmap = get_dev_pagemap(pte_pfn(pte), *pgmap);
+		if (*pgmap)
 			page = pte_page(pte);
 		else
 			goto no_page;

Hm. Shouldn't get_dev_pagemap() call be under if (!*pgmap)?

... ah, never mind. I've got confused by get_dev_pagemap() interface.

mm/huge_memory.c static void touch_pmd(struct vm_area_struct *vma, unsigned long addr, · line 854

Quoted 10 lines

static bool vma_permits_fault(struct vm_area_struct *vma,

mm/huge_memory.c +1 -0

index 533f9b00147d..9839bf91b057 100644
@@ -851,13 +851,23 @@ static void touch_pmd(struct vm_area_struct *vma, unsigned long addr,
 		update_mmu_cache_pmd(vma, addr, pmd);
 }
 
+static struct page *pagemap_page(unsigned long pfn, struct dev_pagemap **pgmap)

The function name doesn't reflect the fact that it takes pin on the page. Maybe pagemap_get_page()?

Quoted 11 lines

mm/huge_memory.c +11 -0

+{
+	struct page *page;
+
+	*pgmap = get_dev_pagemap(pfn, *pgmap);
+	if (!*pgmap)
+		return ERR_PTR(-EFAULT);
+	page = pfn_to_page(pfn);
+	get_page(page);
+	return page;
+}
+

Signature · 1 line

 Kirill A. Shutemov

Dave Hansen

2018-10-02 15:49 UTC #13 lore raw

Re: [PATCHv3 6/6] mm/gup: Cache dev_pagemap while pinning pages

Quoted 8 lines · Kirill A. Shutemov

include/linux/mm.h static inline vm_fault_t vmf_error(int err) · line 2559
Quoted 5 lines
include/linux/mm.h +4 -0
+	page = follow_page_mask(vma, address, foll_flags, &ctx);
+	if (ctx.pgmap)
+		put_dev_pagemap(ctx.pgmap);
+	return page;
 }
Do we still want to keep the function as inline? I don't think so. Let's move it into mm/gup.c and make struct follow_page_context private to the file.

Yeah, and let's have a put_follow_page_context() that does the put_dev_pagemap() rather than spreading that if() to each call site.

Keith Busch

author

2018-10-02 16:03 UTC #14 lore raw

Re: [PATCHv3 6/6] mm/gup: Cache dev_pagemap while pinning pages

Quoted 10 lines · Dave Hansen

Quoted 8 lines · Kirill A. Shutemov
include/linux/mm.h static inline vm_fault_t vmf_error(int err) · line 2559
Quoted 5 lines
include/linux/mm.h +4 -0
+	page = follow_page_mask(vma, address, foll_flags, &ctx);
+	if (ctx.pgmap)
+		put_dev_pagemap(ctx.pgmap);
+	return page;
 }
Do we still want to keep the function as inline? I don't think so. Let's move it into mm/gup.c and make struct follow_page_context private to the file.

Quoted 2 lines

Yeah, and let's have a put_follow_page_context() that does the put_dev_pagemap() rather than spreading that if() to each call site.

Thanks for all the feedback. I will make a new version, but with the gup_benchmark part split into an independent set since it is logically separate from the final patch.