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

[John Hubbard <jhubbard@nvidia.com>]

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,
-- 
John Hubbard
NVIDIA

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