Skip to contentSkip to conversation

mm/gup_benchmark: Time put_page

PATCH In review Keith Busch 2 participants 12 messages Oct 10, 2018

Subsystems (2)
Memory management Kselftest
lore
Share & tools
lore

Keith Busch

author
#0 lore raw

PATCH 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 Dave Hansen <[email protected]>
Cc Dan Williams <[email protected]>
Acked-by Kirill A. Shutemov <[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 7405c9d89d65..b344abd6e8e4 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;

@@ -48,14 +49,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
author
#1 lore raw

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

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 Dave Hansen <[email protected]>
Cc Dan Williams <[email protected]>
Acked-by Kirill A. Shutemov <[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 b344abd6e8e4..ab103a018627 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;

@@ -42,7 +44,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;

@@ -71,8 +89,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
author
#2 lore raw

PATCH 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 Dave Hansen <[email protected]>
Cc Dan Williams <[email protected]>
Acked-by Kirill A. Shutemov <[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
author
#3 lore raw

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

In reply to Keith Busch

This patch allows a user to specify a file to map by adding a new option, '-f', providing a means to test various file backings.

If not specified, the benchmark will use a private mapping of /dev/zero, which produces an anonymous mapping as before.

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 | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
tools/testing/selftests/vm/gup_benchmark.c +11 -4
index b2082df8beb4..b675a3d60975 100644
@@ -31,11 +31,12 @@ 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 i, fd, filed, opt, nr_pages = 1, thp = -1, repeats = 1, write = 0;
 	int cmd = GUP_FAST_BENCHMARK;
+	char *file = "/dev/zero";
 	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 +62,18 @@ int main(int argc, char **argv)
 		case 'w':
 			write = 1;
 			break;
+		case 'f':
+			file = optarg;
+			break;
 		default:
 			return -1;
 		}
 	}
 
+	filed = open(file, O_RDWR|O_CREAT);
+	if (filed < 0)
+		perror("open"), exit(filed);
+
 	gup.nr_pages_per_call = nr_pages;
 	gup.flags = write;
 

@@ -73,8 +81,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, MAP_PRIVATE, filed, 0);
 	if (p == MAP_FAILED)
 		perror("mmap"), exit(1);
 	gup.addr = (unsigned long)p;
Signature · 1 line
2.14.4
maintainer
#4 lore raw

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

Quoted 18 lines · On Wed, 10 Oct 2018 13:56:03 -0600 Keith Busch <[email protected]> wrote:

This patch allows a user to specify a file to map by adding a new option, '-f', providing a means to test various file backings.

If not specified, the benchmark will use a private mapping of /dev/zero, which produces an anonymous mapping as before.

...

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

@@ -61,11 +62,18 @@ int main(int argc, char **argv)
 		case 'w':
 			write = 1;
 			break;
+		case 'f':

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

Quoted 10 lines
tools/testing/selftests/vm/gup_benchmark.c +5 -0
+			file = optarg;
+			break;
 		default:
 			return -1;
 		}
 	}
 
+	filed = open(file, O_RDWR|O_CREAT);
+	if (filed < 0)
+		perror("open"), exit(filed);

Ick. Like this, please:

tools/testing/selftests/vm/gup_benchmark.c renamed from tools/testing/selftests/vm/gup_benchmark.c~tools-gup_benchmark-allow-user-specified-file-fix +4 -2
@@ -71,8 +71,10 @@ int main(int argc, char **argv)
 	}
 
 	filed = open(file, O_RDWR|O_CREAT);
-	if (filed < 0)
-		perror("open"), exit(filed);
+	if (filed < 0) {
+		perror("open");
+		exit(filed);
+	}
 
 	gup.nr_pages_per_call = nr_pages;
 	gup.flags = write;
author
#5 lore raw

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

Quoted 6 lines · Andrew Morton

Review

Quoted 3 lines · On Wed, 10 Oct 2018 13:56:03 -0600 Keith Busch <[email protected]> wrote:
(quoted diff) +3 -0
+	filed = open(file, O_RDWR|O_CREAT);
+	if (filed < 0)
+		perror("open"), exit(filed);

Ick. Like this, please:

Yeah, I agree. I just copied the style this file had been using in other error cases, but I still find it less readable than your recommendation.

Quoted 16 lines
tools/testing/selftests/vm/gup_benchmark.c renamed from tools/testing/selftests/vm/gup_benchmark.c~tools-gup_benchmark-allow-user-specified-file-fix +4 -2
@@ -71,8 +71,10 @@ int main(int argc, char **argv)
 	}
 
 	filed = open(file, O_RDWR|O_CREAT);
-	if (filed < 0)
-		perror("open"), exit(filed);
+	if (filed < 0) {
+		perror("open");
+		exit(filed);
+	}
 
 	gup.nr_pages_per_call = nr_pages;
 	gup.flags = write;
author
#6 lore raw

PATCH 5/6 tools/gup_benchmark: Add MAP_SHARED option

In reply to Keith Busch

This patch adds a new benchmark option, -S, to request MAP_SHARED. This can be used to compare with MAP_PRIVATE, or for files that require this option, like dax.

Signed-off-by Keith Busch <[email protected]>
---
 tools/testing/selftests/vm/gup_benchmark.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
tools/testing/selftests/vm/gup_benchmark.c +7 -3
index b675a3d60975..24528b54549d 100644
@@ -32,11 +32,11 @@ int main(int argc, char **argv)
 	struct gup_benchmark gup;
 	unsigned long size = 128 * MB;
 	int i, fd, filed, opt, nr_pages = 1, thp = -1, repeats = 1, write = 0;
-	int cmd = GUP_FAST_BENCHMARK;
+	int cmd = GUP_FAST_BENCHMARK, flags = MAP_PRIVATE;
 	char *file = "/dev/zero";
 	char *p;
 
-	while ((opt = getopt(argc, argv, "m:r:n:f:tTLU")) != -1) {
+	while ((opt = getopt(argc, argv, "m:r:n:f:tTLUS")) != -1) {
 		switch (opt) {
 		case 'm':
 			size = atoi(optarg) * MB;

@@ -65,6 +65,10 @@ int main(int argc, char **argv)
 		case 'f':
 			file = optarg;
 			break;
+		case 'S':
+			flags &= ~MAP_PRIVATE;
+			flags |= MAP_SHARED;
+			break;
 		default:
 			return -1;
 		}

@@ -81,7 +85,7 @@ int main(int argc, char **argv)
 	if (fd == -1)
 		perror("open"), exit(1);
 
-	p = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, filed, 0);
+	p = mmap(NULL, size, PROT_READ | PROT_WRITE, flags, filed, 0);
 	if (p == MAP_FAILED)
 		perror("mmap"), exit(1);
 	gup.addr = (unsigned long)p;
Signature · 1 line
2.14.4
author
#7 lore raw

PATCH 6/6 tools/gup_benchmark: Add MAP_HUGETLB option

In reply to Keith Busch

This patch adds a new option, '-H', to the gup benchmark to help compare how hugetlb mapping pages compare with the default.

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 24528b54549d..c7b5934c6d7f 100644
@@ -36,7 +36,7 @@ int main(int argc, char **argv)
 	char *file = "/dev/zero";
 	char *p;
 
-	while ((opt = getopt(argc, argv, "m:r:n:f:tTLUS")) != -1) {
+	while ((opt = getopt(argc, argv, "m:r:n:f:tTLUSH")) != -1) {
 		switch (opt) {
 		case 'm':
 			size = atoi(optarg) * MB;

@@ -69,6 +69,9 @@ int main(int argc, char **argv)
 			flags &= ~MAP_PRIVATE;
 			flags |= MAP_SHARED;
 			break;
+		case 'H':
+			flags |= MAP_HUGETLB;
+			break;
 		default:
 			return -1;
 		}
Signature · 1 line
2.14.4
maintainer
#8 lore raw
In reply to Keith Busch
Quoted 11 lines · On Wed, 10 Oct 2018 13:56:00 -0600 Keith Busch <[email protected]> wrote:

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.

...

mm/gup_benchmark.c line 15

Quoted 9 lines
mm/gup_benchmark.c +2 -1
@@ -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;

If we move put_delta_usec to the end of this struct, the ABI remains back-compatible?

author
#9 lore raw
Quoted 15 lines · Andrew Morton
Quoted 13 lines · On Wed, 10 Oct 2018 13:56:00 -0600 Keith Busch <[email protected]> wrote:

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.

...

mm/gup_benchmark.c +0 -0
@@ -8,7 +8,8 @@
 #define GUP_FAST_BENCHMARK	_IOWR('g', 1, struct gup_benchmark)
Quoted 10 lines

mm/gup_benchmark.c line 15

Quoted 7 lines
mm/gup_benchmark.c +2 -1
 struct gup_benchmark {
-	__u64 delta_usec;
+	__u64 get_delta_usec;
+	__u64 put_delta_usec;
 	__u64 addr;
 	__u64 size;
 	__u32 nr_pages_per_call;

If we move put_delta_usec to the end of this struct, the ABI remains back-compatible?

If the kernel writes to a new value appended to the end of the struct, and the application allocated the older sized struct, wouldn't that corrupt the user memory?

maintainer
#10 lore raw
Quoted 11 lines · On Wed, 10 Oct 2018 16:28:43 -0600 Keith Busch <[email protected]> wrote:
Quoted 10 lines

Review

Quoted 7 lines

struct gup_benchmark {

(quoted diff) +2 -1
-	__u64 delta_usec;
+	__u64 get_delta_usec;
+	__u64 put_delta_usec;
 	__u64 addr;
 	__u64 size;
 	__u32 nr_pages_per_call;

If we move put_delta_usec to the end of this struct, the ABI remains back-compatible?

Quoted 3 lines

If the kernel writes to a new value appended to the end of the struct, and the application allocated the older sized struct, wouldn't that corrupt the user memory?

Looks like it. How about we do this while we're breaking it?

mm/gup_benchmark.c renamed from mm/gup_benchmark.c~mm-gup_benchmark-time-put_page-fix +1 -0
@@ -14,6 +14,7 @@ struct gup_benchmark {
 	__u64 size;
 	__u32 nr_pages_per_call;
 	__u32 flags;
+	__u64 expansion[10];	/* For future use */
 };
 
 static int __gup_benchmark_ioctl(unsigned int cmd,
author
#11 lore raw
Quoted 13 lines · Andrew Morton
Quoted 11 lines · On Wed, 10 Oct 2018 16:28:43 -0600 Keith Busch <[email protected]> wrote:
Quoted 10 lines

Review

Quoted 7 lines

struct gup_benchmark {

(quoted diff) +2 -1
-	__u64 delta_usec;
+	__u64 get_delta_usec;
+	__u64 put_delta_usec;
 	__u64 addr;
 	__u64 size;
 	__u32 nr_pages_per_call;

If we move put_delta_usec to the end of this struct, the ABI remains back-compatible?

Quoted 5 lines
Quoted 3 lines

If the kernel writes to a new value appended to the end of the struct, and the application allocated the older sized struct, wouldn't that corrupt the user memory?

Looks like it. How about we do this while we're breaking it?

Yep, that sounds good to me!

Quoted 11 lines
mm/gup_benchmark.c renamed from mm/gup_benchmark.c~mm-gup_benchmark-time-put_page-fix +1 -0
@@ -14,6 +14,7 @@ struct gup_benchmark {
 	__u64 size;
 	__u32 nr_pages_per_call;
 	__u32 flags;
+	__u64 expansion[10];	/* For future use */
 };
 
 static int __gup_benchmark_ioctl(unsigned int cmd,