diff mbox series

[v4,23/23] mm/gup: remove support for gup(FOLL_LONGTERM)

Message ID 20191113042710.3997854-24-jhubbard@nvidia.com (mailing list archive)
State New, archived
Headers show
Series mm/gup: track dma-pinned pages: FOLL_PIN, FOLL_LONGTERM | expand

Commit Message

John Hubbard Nov. 13, 2019, 4:27 a.m. UTC 
Now that all other kernel callers of get_user_pages(FOLL_LONGTERM)
have been converted to pin_longterm_pages(), lock it down:

1) Add an assertion to get_user_pages(), preventing callers from
   passing FOLL_LONGTERM (in addition to the existing assertion that
   prevents FOLL_PIN).

2) Remove the associated GUP_LONGTERM_BENCHMARK test.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 mm/gup.c                                   | 8 ++++----
 mm/gup_benchmark.c                         | 9 +--------
 tools/testing/selftests/vm/gup_benchmark.c | 7 ++-----
 3 files changed, 7 insertions(+), 17 deletions(-)

Comments

Ira Weiny Nov. 13, 2019, 7:09 p.m. UTC | #1 
On Tue, Nov 12, 2019 at 08:27:10PM -0800, John Hubbard wrote:
> Now that all other kernel callers of get_user_pages(FOLL_LONGTERM)
> have been converted to pin_longterm_pages(), lock it down:
> 
> 1) Add an assertion to get_user_pages(), preventing callers from
>    passing FOLL_LONGTERM (in addition to the existing assertion that
>    prevents FOLL_PIN).
> 
> 2) Remove the associated GUP_LONGTERM_BENCHMARK test.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  mm/gup.c                                   | 8 ++++----
>  mm/gup_benchmark.c                         | 9 +--------
>  tools/testing/selftests/vm/gup_benchmark.c | 7 ++-----
>  3 files changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 82e7e4ce5027..90f5f95ee7ac 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1756,11 +1756,11 @@ long get_user_pages(unsigned long start, unsigned long nr_pages,
>  		struct vm_area_struct **vmas)
>  {
>  	/*
> -	 * FOLL_PIN must only be set internally by the pin_user_page*() and
> -	 * pin_longterm_*() APIs, never directly by the caller, so enforce that
> -	 * with an assertion:
> +	 * FOLL_PIN and FOLL_LONGTERM must only be set internally by the
> +	 * pin_user_page*() and pin_longterm_*() APIs, never directly by the
> +	 * caller, so enforce that with an assertion:
>  	 */
> -	if (WARN_ON_ONCE(gup_flags & FOLL_PIN))
> +	if (WARN_ON_ONCE(gup_flags & (FOLL_PIN | FOLL_LONGTERM)))

Don't we want to block FOLL_LONGTERM in get_user_pages_fast() as well after all
this?

Ira

>  		return -EINVAL;
>  
>  	return __gup_longterm_locked(current, current->mm, start, nr_pages,
> diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> index 8f980d91dbf5..679f0e6a0adb 100644
> --- a/mm/gup_benchmark.c
> +++ b/mm/gup_benchmark.c
> @@ -6,7 +6,7 @@
>  #include <linux/debugfs.h>
>  
>  #define GUP_FAST_BENCHMARK	_IOWR('g', 1, struct gup_benchmark)
> -#define GUP_LONGTERM_BENCHMARK	_IOWR('g', 2, struct gup_benchmark)
> +/* Command 2 has been deleted. */
>  #define GUP_BENCHMARK		_IOWR('g', 3, struct gup_benchmark)
>  #define PIN_FAST_BENCHMARK	_IOWR('g', 4, struct gup_benchmark)
>  #define PIN_LONGTERM_BENCHMARK	_IOWR('g', 5, struct gup_benchmark)
> @@ -28,7 +28,6 @@ static void put_back_pages(int cmd, struct page **pages, unsigned long nr_pages)
>  
>  	switch (cmd) {
>  	case GUP_FAST_BENCHMARK:
> -	case GUP_LONGTERM_BENCHMARK:
>  	case GUP_BENCHMARK:
>  		for (i = 0; i < nr_pages; i++)
>  			put_page(pages[i]);
> @@ -97,11 +96,6 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
>  			nr = get_user_pages_fast(addr, nr, gup->flags,
>  						 pages + i);
>  			break;
> -		case GUP_LONGTERM_BENCHMARK:
> -			nr = get_user_pages(addr, nr,
> -					    gup->flags | FOLL_LONGTERM,
> -					    pages + i, NULL);
> -			break;
>  		case GUP_BENCHMARK:
>  			nr = get_user_pages(addr, nr, gup->flags, pages + i,
>  					    NULL);
> @@ -159,7 +153,6 @@ static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,
>  
>  	switch (cmd) {
>  	case GUP_FAST_BENCHMARK:
> -	case GUP_LONGTERM_BENCHMARK:
>  	case GUP_BENCHMARK:
>  	case PIN_FAST_BENCHMARK:
>  	case PIN_LONGTERM_BENCHMARK:
> diff --git a/tools/testing/selftests/vm/gup_benchmark.c b/tools/testing/selftests/vm/gup_benchmark.c
> index 03928e47a86f..836b7082db13 100644
> --- a/tools/testing/selftests/vm/gup_benchmark.c
> +++ b/tools/testing/selftests/vm/gup_benchmark.c
> @@ -15,7 +15,7 @@
>  #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)
> +/* Command 2 has been deleted. */
>  #define GUP_BENCHMARK		_IOWR('g', 3, struct gup_benchmark)
>  
>  /*
> @@ -49,7 +49,7 @@ int main(int argc, char **argv)
>  	char *file = "/dev/zero";
>  	char *p;
>  
> -	while ((opt = getopt(argc, argv, "m:r:n:f:abctTLUuwSH")) != -1) {
> +	while ((opt = getopt(argc, argv, "m:r:n:f:abctTUuwSH")) != -1) {
>  		switch (opt) {
>  		case 'a':
>  			cmd = PIN_FAST_BENCHMARK;
> @@ -75,9 +75,6 @@ int main(int argc, char **argv)
>  		case 'T':
>  			thp = 0;
>  			break;
> -		case 'L':
> -			cmd = GUP_LONGTERM_BENCHMARK;
> -			break;
>  		case 'U':
>  			cmd = GUP_BENCHMARK;
>  			break;
> -- 
> 2.24.0
>
John Hubbard Nov. 13, 2019, 11:27 p.m. UTC | #2 
On 11/13/19 11:09 AM, Ira Weiny wrote:
...
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 82e7e4ce5027..90f5f95ee7ac 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -1756,11 +1756,11 @@ long get_user_pages(unsigned long start, unsigned long nr_pages,
>>   		struct vm_area_struct **vmas)
>>   {
>>   	/*
>> -	 * FOLL_PIN must only be set internally by the pin_user_page*() and
>> -	 * pin_longterm_*() APIs, never directly by the caller, so enforce that
>> -	 * with an assertion:
>> +	 * FOLL_PIN and FOLL_LONGTERM must only be set internally by the
>> +	 * pin_user_page*() and pin_longterm_*() APIs, never directly by the
>> +	 * caller, so enforce that with an assertion:
>>   	 */
>> -	if (WARN_ON_ONCE(gup_flags & FOLL_PIN))
>> +	if (WARN_ON_ONCE(gup_flags & (FOLL_PIN | FOLL_LONGTERM)))
> 
> Don't we want to block FOLL_LONGTERM in get_user_pages_fast() as well after all
> this?
> 

Yes. But with the latest idea to restore FOLL_LONGTERM to its original glory,
that won't be an issue in the next version. heh.


thanks,
diff mbox series

Patch

diff --git a/mm/gup.c b/mm/gup.c
index 82e7e4ce5027..90f5f95ee7ac 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1756,11 +1756,11 @@  long get_user_pages(unsigned long start, unsigned long nr_pages,
 		struct vm_area_struct **vmas)
 {
 	/*
-	 * FOLL_PIN must only be set internally by the pin_user_page*() and
-	 * pin_longterm_*() APIs, never directly by the caller, so enforce that
-	 * with an assertion:
+	 * FOLL_PIN and FOLL_LONGTERM must only be set internally by the
+	 * pin_user_page*() and pin_longterm_*() APIs, never directly by the
+	 * caller, so enforce that with an assertion:
 	 */
-	if (WARN_ON_ONCE(gup_flags & FOLL_PIN))
+	if (WARN_ON_ONCE(gup_flags & (FOLL_PIN | FOLL_LONGTERM)))
 		return -EINVAL;
 
 	return __gup_longterm_locked(current, current->mm, start, nr_pages,
diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
index 8f980d91dbf5..679f0e6a0adb 100644
--- a/mm/gup_benchmark.c
+++ b/mm/gup_benchmark.c
@@ -6,7 +6,7 @@ 
 #include <linux/debugfs.h>
 
 #define GUP_FAST_BENCHMARK	_IOWR('g', 1, struct gup_benchmark)
-#define GUP_LONGTERM_BENCHMARK	_IOWR('g', 2, struct gup_benchmark)
+/* Command 2 has been deleted. */
 #define GUP_BENCHMARK		_IOWR('g', 3, struct gup_benchmark)
 #define PIN_FAST_BENCHMARK	_IOWR('g', 4, struct gup_benchmark)
 #define PIN_LONGTERM_BENCHMARK	_IOWR('g', 5, struct gup_benchmark)
@@ -28,7 +28,6 @@  static void put_back_pages(int cmd, struct page **pages, unsigned long nr_pages)
 
 	switch (cmd) {
 	case GUP_FAST_BENCHMARK:
-	case GUP_LONGTERM_BENCHMARK:
 	case GUP_BENCHMARK:
 		for (i = 0; i < nr_pages; i++)
 			put_page(pages[i]);
@@ -97,11 +96,6 @@  static int __gup_benchmark_ioctl(unsigned int cmd,
 			nr = get_user_pages_fast(addr, nr, gup->flags,
 						 pages + i);
 			break;
-		case GUP_LONGTERM_BENCHMARK:
-			nr = get_user_pages(addr, nr,
-					    gup->flags | FOLL_LONGTERM,
-					    pages + i, NULL);
-			break;
 		case GUP_BENCHMARK:
 			nr = get_user_pages(addr, nr, gup->flags, pages + i,
 					    NULL);
@@ -159,7 +153,6 @@  static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,
 
 	switch (cmd) {
 	case GUP_FAST_BENCHMARK:
-	case GUP_LONGTERM_BENCHMARK:
 	case GUP_BENCHMARK:
 	case PIN_FAST_BENCHMARK:
 	case PIN_LONGTERM_BENCHMARK:
diff --git a/tools/testing/selftests/vm/gup_benchmark.c b/tools/testing/selftests/vm/gup_benchmark.c
index 03928e47a86f..836b7082db13 100644
--- a/tools/testing/selftests/vm/gup_benchmark.c
+++ b/tools/testing/selftests/vm/gup_benchmark.c
@@ -15,7 +15,7 @@ 
 #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)
+/* Command 2 has been deleted. */
 #define GUP_BENCHMARK		_IOWR('g', 3, struct gup_benchmark)
 
 /*
@@ -49,7 +49,7 @@  int main(int argc, char **argv)
 	char *file = "/dev/zero";
 	char *p;
 
-	while ((opt = getopt(argc, argv, "m:r:n:f:abctTLUuwSH")) != -1) {
+	while ((opt = getopt(argc, argv, "m:r:n:f:abctTUuwSH")) != -1) {
 		switch (opt) {
 		case 'a':
 			cmd = PIN_FAST_BENCHMARK;
@@ -75,9 +75,6 @@  int main(int argc, char **argv)
 		case 'T':
 			thp = 0;
 			break;
-		case 'L':
-			cmd = GUP_LONGTERM_BENCHMARK;
-			break;
 		case 'U':
 			cmd = GUP_BENCHMARK;
 			break;