[PATCH 1/6] mm/gup_benchmark: Time put_page

[PATCH 1/6] mm/gup_benchmark: Time put_page

[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: Dave Hansen <dave.hansen@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 mm/gup_benchmark.c                         | 8 ++++++--
 tools/testing/selftests/vm/gup_benchmark.c | 6 ++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
index 7405c9d89d65..b344abd6e8e4 100644
--- a/mm/gup_benchmark.c
+++ b/mm/gup_benchmark.c
@@ -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;
diff --git a/tools/testing/selftests/vm/gup_benchmark.c b/tools/testing/selftests/vm/gup_benchmark.c
index 36df55132036..bdcb97acd0ac 100644
--- a/tools/testing/selftests/vm/gup_benchmark.c
+++ b/tools/testing/selftests/vm/gup_benchmark.c
@@ -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");
-- 
2.14.4

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

[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: Dave Hansen <dave.hansen@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 mm/gup_benchmark.c                         | 28 ++++++++++++++++++++++++++--
 tools/testing/selftests/vm/gup_benchmark.c | 13 +++++++++++--
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
index b344abd6e8e4..ab103a018627 100644
--- a/mm/gup_benchmark.c
+++ b/mm/gup_benchmark.c
@@ -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;
diff --git a/tools/testing/selftests/vm/gup_benchmark.c b/tools/testing/selftests/vm/gup_benchmark.c
index bdcb97acd0ac..c2f785ded9b9 100644
--- a/tools/testing/selftests/vm/gup_benchmark.c
+++ b/tools/testing/selftests/vm/gup_benchmark.c
@@ -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,
-- 
2.14.4

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

[Keith Busch]

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

Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 tools/testing/selftests/vm/gup_benchmark.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/vm/gup_benchmark.c b/tools/testing/selftests/vm/gup_benchmark.c
index c2f785ded9b9..b2082df8beb4 100644
--- a/tools/testing/selftests/vm/gup_benchmark.c
+++ b/tools/testing/selftests/vm/gup_benchmark.c
@@ -60,6 +60,7 @@ int main(int argc, char **argv)
 			break;
 		case 'w':
 			write = 1;
+			break;
 		default:
 			return -1;
 		}
-- 
2.14.4

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

[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 <kirill.shutemov@linux.intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 tools/testing/selftests/vm/gup_benchmark.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/vm/gup_benchmark.c b/tools/testing/selftests/vm/gup_benchmark.c
index b2082df8beb4..b675a3d60975 100644
--- a/tools/testing/selftests/vm/gup_benchmark.c
+++ b/tools/testing/selftests/vm/gup_benchmark.c
@@ -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;
-- 
2.14.4

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

[Andrew Morton]

On Wed, 10 Oct 2018 13:56:03 -0600 Keith Busch <keith.busch@intel.com> 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.
> 
> ...
>
> --- a/tools/testing/selftests/vm/gup_benchmark.c
> +++ b/tools/testing/selftests/vm/gup_benchmark.c
>
> ...
>
> @@ -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);

Ick.  Like this, please:

--- a/tools/testing/selftests/vm/gup_benchmark.c~tools-gup_benchmark-allow-user-specified-file-fix
+++ a/tools/testing/selftests/vm/gup_benchmark.c
@@ -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;

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

[Keith Busch]

On Wed, Oct 10, 2018 at 03:31:01PM -0700, Andrew Morton wrote:
> On Wed, 10 Oct 2018 13:56:03 -0600 Keith Busch <keith.busch@intel.com> wrote:
> > +	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. 
 
> --- a/tools/testing/selftests/vm/gup_benchmark.c~tools-gup_benchmark-allow-user-specified-file-fix
> +++ a/tools/testing/selftests/vm/gup_benchmark.c
> @@ -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;
> 

[PATCH 5/6] tools/gup_benchmark: Add MAP_SHARED option

[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 <keith.busch@intel.com>
---
 tools/testing/selftests/vm/gup_benchmark.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/vm/gup_benchmark.c b/tools/testing/selftests/vm/gup_benchmark.c
index b675a3d60975..24528b54549d 100644
--- a/tools/testing/selftests/vm/gup_benchmark.c
+++ b/tools/testing/selftests/vm/gup_benchmark.c
@@ -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;
-- 
2.14.4

[PATCH 6/6] tools/gup_benchmark: Add MAP_HUGETLB option

[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 <keith.busch@intel.com>
---
 tools/testing/selftests/vm/gup_benchmark.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/vm/gup_benchmark.c b/tools/testing/selftests/vm/gup_benchmark.c
index 24528b54549d..c7b5934c6d7f 100644
--- a/tools/testing/selftests/vm/gup_benchmark.c
+++ b/tools/testing/selftests/vm/gup_benchmark.c
@@ -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;
 		}
-- 
2.14.4

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

[Andrew Morton]

On Wed, 10 Oct 2018 13:56:00 -0600 Keith Busch <keith.busch@intel.com> 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.
> 
> ...
>
> --- a/mm/gup_benchmark.c
> +++ b/mm/gup_benchmark.c
> @@ -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?

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

[Keith Busch]

On Wed, Oct 10, 2018 at 03:26:55PM -0700, Andrew Morton wrote:
> On Wed, 10 Oct 2018 13:56:00 -0600 Keith Busch <keith.busch@intel.com> 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.
> > 
> > ...
> >
> > --- a/mm/gup_benchmark.c
> > +++ b/mm/gup_benchmark.c
> > @@ -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?

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?

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

[Andrew Morton]

On Wed, 10 Oct 2018 16:28:43 -0600 Keith Busch <keith.busch@intel.com> wrote:

> > >  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?

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

--- a/mm/gup_benchmark.c~mm-gup_benchmark-time-put_page-fix
+++ a/mm/gup_benchmark.c
@@ -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,

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

[Keith Busch]

On Wed, Oct 10, 2018 at 03:41:11PM -0700, Andrew Morton wrote:
> On Wed, 10 Oct 2018 16:28:43 -0600 Keith Busch <keith.busch@intel.com> wrote:
> 
> > > >  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?
> 
> Looks like it.  How about we do this while we're breaking it?

Yep, that sounds good to me!
 
> --- a/mm/gup_benchmark.c~mm-gup_benchmark-time-put_page-fix
> +++ a/mm/gup_benchmark.c
> @@ -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,
>