diff mbox series

[PATCHv6,3/3] mm/gup_benchemark: add LONGTERM_BENCHMARK test in gup fast path

Message ID 1584333244-10480-4-git-send-email-kernelfans@gmail.com (mailing list archive)
State New, archived
Headers show
Series fix omission of check on FOLL_LONGTERM in gup fast path | expand

Commit Message

Pingfan Liu March 16, 2020, 4:34 a.m. UTC 
Introduce a PIN_FAST_LONGTERM_BENCHMARK ioctl to test longterm pin in gup fast
path.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Alexander Duyck <alexander.duyck@gmail.com>
To: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 mm/gup_benchmark.c                         | 7 +++++++
 tools/testing/selftests/vm/gup_benchmark.c | 6 +++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

--
2.7.5

Comments

John Hubbard March 19, 2020, 10:27 p.m. UTC | #1 
On 3/15/20 9:34 PM, Pingfan Liu wrote:
> Introduce a PIN_FAST_LONGTERM_BENCHMARK ioctl to test longterm pin in gup fast
> path.

1. The subject line still has "benchemark", which should be "benchmark".

2. What do you really want to test? More about the use case to be tested would help.
Just another sentence. I wouldn't normally ask, but in this case the implementation
is slightly scrambled, and I can't suggest how to fix it because there's no information
in the commit log as to the use case, nor the motivation.

Below...


> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mike Rapoport <rppt@linux.ibm.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> To: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   mm/gup_benchmark.c                         | 7 +++++++
>   tools/testing/selftests/vm/gup_benchmark.c | 6 +++++-
>   2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> index be690fa..09fba20 100644
> --- a/mm/gup_benchmark.c
> +++ b/mm/gup_benchmark.c
> @@ -10,6 +10,7 @@
>   #define GUP_BENCHMARK		_IOWR('g', 3, struct gup_benchmark)
>   #define PIN_FAST_BENCHMARK	_IOWR('g', 4, struct gup_benchmark)
>   #define PIN_BENCHMARK		_IOWR('g', 5, struct gup_benchmark)
> +#define PIN_FAST_LONGTERM_BENCHMARK	_IOWR('g', 6, struct gup_benchmark)


Use PIN_ prefixes for things that test pin_user_pages*(), and GUP_ prefixes for
things that test get_user_pages*().

I can't really be sure which one you want to test, but these wrongly intermixed:
here you are using PIN_ and get_user_pages*().


> 
>   struct gup_benchmark {
>   	__u64 get_delta_usec;
> @@ -101,6 +102,11 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
>   			nr = get_user_pages_fast(addr, nr, gup->flags,
>   						 pages + i);
>   			break;
> +		case PIN_FAST_LONGTERM_BENCHMARK:
> +			nr = get_user_pages_fast(addr, nr,
> +					gup->flags | FOLL_LONGTERM,
> +					pages + i);
> +			break;


Probably, pin_user_pages_fast() is where you want to go here, not get_user_pages_fast().
See the guidance in Documentation/core-api/pin_user_pages.rst to help decide.

thanks,
Pingfan Liu March 20, 2020, 9:17 a.m. UTC | #2 
On Fri, Mar 20, 2020 at 6:27 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 3/15/20 9:34 PM, Pingfan Liu wrote:
> > Introduce a PIN_FAST_LONGTERM_BENCHMARK ioctl to test longterm pin in gup fast
> > path.
>
> 1. The subject line still has "benchemark", which should be "benchmark".
>
> 2. What do you really want to test? More about the use case to be tested would help.
> Just another sentence. I wouldn't normally ask, but in this case the implementation
> is slightly scrambled, and I can't suggest how to fix it because there's no information
> in the commit log as to the use case, nor the motivation.
Oh, the history about [3/3] is to verify the implementation method of
[2/3]. Please refer to
https://lore.kernel.org/linux-mm/20190611122935.GA9919@dhcp-128-55.nay.redhat.com/
Cite "
> > I think the concern is: for the successful gup_fast case with no CMA

> > pages, this patch is adding another complete loop through all the
> > pages. In the fast case.
> >
> > If the check were instead done as part of the gup_pte_range(), then
> > it would be a little more efficient for that case.
> >
> > As for whether it's worth it, *probably* this is too small an effect to measure.
> > But in order to attempt a measurement: running fio (https://github.com/axboe/fio)
> > with O_DIRECT on an NVMe drive, might shed some light. Here's an fio.conf file
> > that Jan Kara and Tom Talpey helped me come up with, for related testing:
"
But I think now, there is no motivation for it, and can be dropped it now.

Thanks,
Pingfan
diff mbox series

Patch

diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
index be690fa..09fba20 100644
--- a/mm/gup_benchmark.c
+++ b/mm/gup_benchmark.c
@@ -10,6 +10,7 @@ 
 #define GUP_BENCHMARK		_IOWR('g', 3, struct gup_benchmark)
 #define PIN_FAST_BENCHMARK	_IOWR('g', 4, struct gup_benchmark)
 #define PIN_BENCHMARK		_IOWR('g', 5, struct gup_benchmark)
+#define PIN_FAST_LONGTERM_BENCHMARK	_IOWR('g', 6, struct gup_benchmark)

 struct gup_benchmark {
 	__u64 get_delta_usec;
@@ -101,6 +102,11 @@  static int __gup_benchmark_ioctl(unsigned int cmd,
 			nr = get_user_pages_fast(addr, nr, gup->flags,
 						 pages + i);
 			break;
+		case PIN_FAST_LONGTERM_BENCHMARK:
+			nr = get_user_pages_fast(addr, nr,
+					gup->flags | FOLL_LONGTERM,
+					pages + i);
+			break;
 		case GUP_LONGTERM_BENCHMARK:
 			nr = get_user_pages(addr, nr,
 					    gup->flags | FOLL_LONGTERM,
@@ -166,6 +172,7 @@  static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,
 	case GUP_BENCHMARK:
 	case PIN_FAST_BENCHMARK:
 	case PIN_BENCHMARK:
+	case PIN_FAST_LONGTERM_BENCHMARK:
 		break;
 	default:
 		return -EINVAL;
diff --git a/tools/testing/selftests/vm/gup_benchmark.c b/tools/testing/selftests/vm/gup_benchmark.c
index 43b4dfe..f024ff3 100644
--- a/tools/testing/selftests/vm/gup_benchmark.c
+++ b/tools/testing/selftests/vm/gup_benchmark.c
@@ -21,6 +21,7 @@ 
 /* Similar to above, but use FOLL_PIN instead of FOLL_GET. */
 #define PIN_FAST_BENCHMARK	_IOWR('g', 4, struct gup_benchmark)
 #define PIN_BENCHMARK		_IOWR('g', 5, struct gup_benchmark)
+#define PIN_FAST_LONGTERM_BENCHMARK	_IOWR('g', 6, struct gup_benchmark)

 /* Just the flags we need, copied from mm.h: */
 #define FOLL_WRITE	0x01	/* check pte is writable */
@@ -44,7 +45,7 @@  int main(int argc, char **argv)
 	char *file = "/dev/zero";
 	char *p;

-	while ((opt = getopt(argc, argv, "m:r:n:f:abtTLUuwSH")) != -1) {
+	while ((opt = getopt(argc, argv, "m:r:n:f:abtTlLUuwSH")) != -1) {
 		switch (opt) {
 		case 'a':
 			cmd = PIN_FAST_BENCHMARK;
@@ -67,6 +68,9 @@  int main(int argc, char **argv)
 		case 'T':
 			thp = 0;
 			break;
+		case 'l':
+			cmd = PIN_FAST_LONGTERM_BENCHMARK;
+			break;
 		case 'L':
 			cmd = GUP_LONGTERM_BENCHMARK;
 			break;