2018-10-10 20:00:39

by Keith Busch

[permalink] [raw]
Subject: [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(-)

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



2018-10-10 20:00:49

by Keith Busch

[permalink] [raw]
Subject: [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(-)

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


2018-10-10 20:01:18

by Keith Busch

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

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(+)

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


2018-10-10 20:01:18

by Keith Busch

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

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(-)

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


2018-10-10 20:01:37

by Keith Busch

[permalink] [raw]
Subject: [PATCH 6/6] tools/gup_benchmark: Add MAP_HUGETLB option

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(-)

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


2018-10-10 20:02:03

by Keith Busch

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

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(-)

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


2018-10-10 22:27:35

by Andrew Morton

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

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



2018-10-10 22:31:42

by Andrew Morton

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

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


2018-10-10 22:33:00

by Keith Busch

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

On Wed, Oct 10, 2018 at 03:26:55PM -0700, Andrew Morton wrote:
> 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.
> >
> > ...
> >
> > --- 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?

2018-10-10 22:41:58

by Andrew Morton

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

On Wed, 10 Oct 2018 16:28:43 -0600 Keith Busch <[email protected]> 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,


2018-10-10 22:44:48

by Keith Busch

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

On Wed, Oct 10, 2018 at 03:41:11PM -0700, Andrew Morton wrote:
> On Wed, 10 Oct 2018 16:28:43 -0600 Keith Busch <[email protected]> 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,
>

2018-10-10 22:46:28

by Keith Busch

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

On Wed, Oct 10, 2018 at 03:31:01PM -0700, Andrew Morton wrote:
> On Wed, 10 Oct 2018 13:56:03 -0600 Keith Busch <[email protected]> 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;
>