fix omission of check on FOLL_LONGTERM in gup fast path

To better reflect the held state of pages and make code self-explaining, rename nr as nr_pinned.

To better reflect the held state of pages and make code self-explaining, rename nr as nr_pinned.

Signed-off-by Pingfan Liu <[email protected]> Cc: Ira Weiny

<[email protected]> Cc: Andrew Morton <[email protected]> Cc: Mike Rapoport <[email protected]> Cc: Dan Williams <[email protected]> Cc: Matthew Wilcox <[email protected]> Cc: John Hubbard <[email protected]> Cc: "Aneesh Kumar K.V" <[email protected]> Cc: Christoph Hellwig
<[email protected]> Cc: Shuah Khan <[email protected]> To:
[email protected] Cc: [email protected] --- mm/gup.c | 20
++++++++++---------- 1 file changed, 10 insertions(+), 10
deletions(-)

mm/gup.c index e8aaa40..9df77b1 100644 --- renamed from mm/gup.c +0 -0

a/mm/gup.c +++ b/mm/gup.c @@ -2735,7 +2735,7 @@ static int
internal_get_user_pages_fast(unsigned long start, int nr_pages, struct page **pages) { unsigned long addr, len, end; - int nr = 0,
ret = 0; + int nr_pinned = 0, ret = 0;

if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM | FOLL_FORCE | FOLL_PIN | FOLL_GET))) @@ -2754,25 +2754,25 @@ static int internal_get_user_pages_fast(unsigned long start, int nr_pages, if (IS_ENABLED(CONFIG_HAVE_FAST_GUP) && gup_fast_permitted(start,
end)) { local_irq_disable(); - gup_pgd_range(addr, end, gup_flags,
pages, &nr); + gup_pgd_range(addr, end, gup_flags, pages,
&nr_pinned); local_irq_enable(); - ret = nr; + ret = nr_pinned; }

mm/gup.c index e8aaa40..9df77b1 100644 --- renamed from mm/gup.c +0 -1

-	if (nr < nr_pages) { +	if (nr_pinned < nr_pages) { /* Try to get

the remaining pages with get_user_pages */ - start += nr <<
PAGE_SHIFT; - pages += nr; + start += nr_pinned << PAGE_SHIFT; +
pages += nr_pinned;

mm/gup.c index e8aaa40..9df77b1 100644 --- renamed from mm/gup.c +0 -1

-		ret = __gup_longterm_unlocked(start, nr_pages - nr, +		ret =

__gup_longterm_unlocked(start, nr_pages - nr_pinned, gup_flags, pages);

/* Have to be a bit careful with return values */ - if (nr > 0) { +
if (nr_pinned > 0) { if (ret < 0) - ret = nr; + ret =
nr_pinned; else - ret += nr; + ret += nr_pinned; } }

-- 2.7.5

FOLL_LONGTERM is a special case of FOLL_PIN. It suggests a pin which is going to be given to hardware and can't move. It would truncate CMA permanently and should be excluded.

In gup slow path, slow path, where __gup_longterm_locked->check_and_migrate_cma_pages() handles FOLL_LONGTERM, but in fast path, there lacks such a check, which means a possible leak of CMA page to longterm pinned.

Place a check in try_grab_compound_head() in the fast path to fix the leak, and if FOLL_LONGTERM happens on CMA, it will fall back to slow path to migrate the page.

Some note about the check:
Huge page's subpages have the same migrate type due to either allocation from a free_list[] or alloc_contig_range() with param MIGRATE_MOVABLE. So it is enough to check on a single subpage by is_migrate_cma_page(subpage)

Signed-off-by Pingfan Liu <[email protected]>

Cc Ira Weiny <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Mike Rapoport <[email protected]>

Cc Dan Williams <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc John Hubbard <[email protected]>

Cc "Aneesh Kumar K.V" <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Shuah Khan <[email protected]>

Cc Jason Gunthorpe <[email protected]>

To: [email protected]
Cc: [email protected]

---
 mm/gup.c | 9 +++++++++
 1 file changed, 9 insertions(+)

mm/gup.c +0 -0

index 9df77b1..78132cf 100644

Wrong indentation. We either align two tabs for continuation statements, or after the opening brace of the previous line. With a likely or unlikely thrown in the former IMHO looks much better. E.g.

                if (unlikely(flags & FOLL_LONGTERM) &&
                                is_migrate_cma_page(page))
                        return NULL;

FOLL_LONGTERM is a special case of FOLL_PIN. It suggests a pin which is going to be given to hardware and can't move. It would truncate CMA permanently and should be excluded.

In gup slow path, slow path, where __gup_longterm_locked->check_and_migrate_cma_pages() handles FOLL_LONGTERM, but in fast path, there lacks such a check, which means a possible leak of CMA page to longterm pinned.

Place a check in try_grab_compound_head() in the fast path to fix the leak, and if FOLL_LONGTERM happens on CMA, it will fall back to slow path to migrate the page.

Some note about the check:
Huge page's subpages have the same migrate type due to either allocation from a free_list[] or alloc_contig_range() with param MIGRATE_MOVABLE. So it is enough to check on a single subpage by is_migrate_cma_page(subpage)

FOLL_LONGTERM is a special case of FOLL_PIN. It suggests a pin which is going to be given to hardware and can't move. It would truncate CMA permanently and should be excluded.

In gup slow path, slow path, where __gup_longterm_locked->check_and_migrate_cma_pages() handles FOLL_LONGTERM, but in fast path, there lacks such a check, which means a possible leak of CMA page to longterm pinned.

Place a check in try_grab_compound_head() in the fast path to fix the leak, and if FOLL_LONGTERM happens on CMA, it will fall back to slow path to migrate the page.

Some note about the check:
Huge page's subpages have the same migrate type due to either allocation from a free_list[] or alloc_contig_range() with param MIGRATE_MOVABLE. So it is enough to check on a single subpage by is_migrate_cma_page(subpage)

Signed-off-by Pingfan Liu <[email protected]>

Cc Ira Weiny <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Mike Rapoport <[email protected]>

Cc Dan Williams <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc John Hubbard <[email protected]>

Cc "Aneesh Kumar K.V" <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Shuah Khan <[email protected]>

Cc Jason Gunthorpe <[email protected]>

To: [email protected]
Cc: [email protected]
---
v6 -> v7: fix coding style issue

 mm/gup.c | 9 +++++++++
 1 file changed, 9 insertions(+)

FOLL_LONGTERM is a special case of FOLL_PIN. It suggests a pin which is going to be given to hardware and can't move. It would truncate CMA permanently and should be excluded.

In gup slow path, slow path, where

__gup_longterm_locked->check_and_migrate_cma_pages() handles FOLL_LONGTERM, but in fast path, there lacks such a check, which means a possible leak of CMA page to longterm pinned.

Place a check in try_grab_compound_head() in the fast path to fix the leak, and if FOLL_LONGTERM happens on CMA, it will fall back to slow path to migrate the page.

Some note about the check:
Huge page's subpages have the same migrate type due to either allocation from a free_list[] or alloc_contig_range() with param MIGRATE_MOVABLE. So it is enough to check on a single subpage by is_migrate_cma_page(subpage)

Signed-off-by Pingfan Liu <[email protected]>

Cc Ira Weiny <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Mike Rapoport <[email protected]>

Cc Dan Williams <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc John Hubbard <[email protected]>

Cc "Aneesh Kumar K.V" <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Shuah Khan <[email protected]>

Cc Jason Gunthorpe <[email protected]>

To: [email protected]
Cc: [email protected]
---
v6 -> v7: fix coding style issue

  mm/gup.c | 9 +++++++++
  1 file changed, 9 insertions(+)

mm/gup.c +0 -0

index 9df77b1..0a536d7 100644

mm/gup.c +5 -1

+		if (unlikely(flags & FOLL_LONGTERM) &&
+				is_migrate_cma_page(page))
+			return NULL;
+
+		/*
  		 * When pinning a compound page of order > 1 (which is what
  		 * hpage_pincount_available() checks for), use an exact count to
  		 * track it, via hpage_pincount_add/_sub().
--

2.7.5

Quoted 5 lines · Pingfan Liu

FOLL_LONGTERM is a special case of FOLL_PIN. It suggests a pin which is going to be given to hardware and can't move. It would truncate CMA permanently and should be excluded.

In gup slow path, slow path, where

s/slow path, slow path/slow path/

mm/gup.c static __maybe_unused struct page *try_grab_compound_head(struct page *page, · line 95

Quoted 6 lines

              /*

mm/gup.c +4 -0

+              * Huge page's subpages have the same migrate type due to either
+              * allocation from a free_list[] or alloc_contig_range() with
+              * param MIGRATE_MOVABLE. So it is enough to check on a subpage.
+              */

Urggh, this comment is fine in the commit description, but at this location in the code it is completely incomprehensible! Instead of an extremely far-removed tidbit about interactions between CMA and huge pages, this comment should be explaining why we bail out early in the specific case of FOLL_PIN + FOLL_LONGTERM. And we don't bail out for FOLL_GET + FOLL_LONGTERM...

I'm expect it is something like:

/*
* We can't do FOLL_LONGTERM + FOLL_PIN with CMA in the gup fast
* path, so fail and let the caller fall back to the slow path. */

...approximately. Right?

Introduce a PIN_FAST_LONGTERM_BENCHMARK ioctl to test longterm pin in gup fast path.

Signed-off-by Pingfan Liu <[email protected]>

Cc Ira Weiny <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Mike Rapoport <[email protected]>

Cc Dan Williams <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc John Hubbard <[email protected]>

Cc "Aneesh Kumar K.V" <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Shuah Khan <[email protected]>

Cc Alexander Duyck <[email protected]>

To: [email protected]
Cc: [email protected]

---
  mm/gup_benchmark.c                         | 7 +++++++
  tools/testing/selftests/vm/gup_benchmark.c | 6 +++++-
  2 files changed, 12 insertions(+), 1 deletion(-)

mm/gup_benchmark.c +0 -0

index be690fa..09fba20 100644

mm/gup_benchmark.c +1 -0

  		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;

tools/testing/selftests/vm/gup_benchmark.c +5 -2

index 43b4dfe..f024ff3 100644
@@ -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

Quoted 2 lines · Pingfan Liu

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.

Quoted 5 lines

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.

Quoted 7 lines

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: