On Fri, Dec 18, 2020 at 4:36 AM Michal Hocko mhocko@suse.com wrote:

On Thu 17-12-20 13:52:36, Pavel Tatashin wrote: [..]

...

diff --git a/mm/vmscan.c b/mm/vmscan.c index 469016222cdb..d9546f5897f4 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -3234,11 +3234,12 @@ static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist, unsigned long try_to_free_pages(struct zonelist *zonelist, int order, gfp_t gfp_mask, nodemask_t *nodemask) {

• gfp_t current_gfp_mask = current_gfp_context(gfp_mask);
  unsigned long nr_reclaimed;
  struct scan_control sc = {
          .nr_to_reclaim = SWAP_CLUSTER_MAX,
  

•         .gfp_mask = current_gfp_context(gfp_mask),
  

•         .reclaim_idx = gfp_zone(gfp_mask),
  

•         .gfp_mask = current_gfp_mask,
  

•         .reclaim_idx = gfp_zone(current_gfp_mask),
          .order = order,
          .nodemask = nodemask,
          .priority = DEF_PRIORITY,
  

@@ -4158,17 +4159,18 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in { /* Minimum pages needed in order to stay on node */ const unsigned long nr_pages = 1 << order;

• gfp_t current_gfp_mask = current_gfp_context(gfp_mask);
  struct task_struct *p = current;
  unsigned int noreclaim_flag;
  struct scan_control sc = {
          .nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
  

•         .gfp_mask = current_gfp_context(gfp_mask),
  

•         .gfp_mask = current_gfp_mask,
          .order = order,
          .priority = NODE_RECLAIM_PRIORITY,
          .may_writepage = !!(node_reclaim_mode & RECLAIM_WRITE),
          .may_unmap = !!(node_reclaim_mode & RECLAIM_UNMAP),
          .may_swap = 1,
  

•         .reclaim_idx = gfp_zone(gfp_mask),
  

•         .reclaim_idx = gfp_zone(current_gfp_mask),
  };
  
  trace_mm_vmscan_node_reclaim_begin(pgdat->node_id, order,
  

I was hoping we had agreed these are not necessary and they shouldn't be touched in the patch.

Thank you for noticing, I was sure I removed these changes, not sure what happened :( They will be gone in the next version.

Thank you, Pasha

-- Michal Hocko SUSE Labs

On Thu 17-12-20 13:52:39, Pavel Tatashin wrote:

Document the special handling of page pinning when ZONE_MOVABLE present.

Signed-off-by: Pavel Tatashin pasha.tatashin@soleen.com Suggested-by: David Hildenbrand david@redhat.com

Acked-by: Michal Hocko mhocko@suse.com

---

Documentation/admin-guide/mm/memory-hotplug.rst | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/Documentation/admin-guide/mm/memory-hotplug.rst b/Documentation/admin-guide/mm/memory-hotplug.rst index 5c4432c96c4b..c6618f99f765 100644 --- a/Documentation/admin-guide/mm/memory-hotplug.rst +++ b/Documentation/admin-guide/mm/memory-hotplug.rst @@ -357,6 +357,15 @@ creates ZONE_MOVABLE as following. Unfortunately, there is no information to show which memory block belongs to ZONE_MOVABLE. This is TBD. +.. note::

• Techniques that rely on long-term pinnings of memory (especially, RDMA and
• vfio) are fundamentally problematic with ZONE_MOVABLE and, therefore, memory
• hot remove. Pinned pages cannot reside on ZONE_MOVABLE, to guarantee that
• memory can still get hot removed - be aware that pinning can fail even if
• there is plenty of free memory in ZONE_MOVABLE. In addition, using
• ZONE_MOVABLE might make page pinning more expensive, because pages have to be
• migrated off that zone first.

.. _memory_hotplug_how_to_offline_memory: How to offline memory -- 2.25.1

On Thu 17-12-20 13:52:40, Pavel Tatashin wrote:

In __get_user_pages_locked() i counts number of pages which should be long.

Do we know of any caller who would like to pin so many pages it wouldn't fit into an int? I suspect this is more to sync types of nr_pages and the iterator right. It would be better to be explicit about this in the changelog.

Signed-off-by: Pavel Tatashin pasha.tatashin@soleen.com

Acked-by: Michal Hocko mhocko@suse.com

---

mm/gup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/gup.c b/mm/gup.c index 591d8e2dfc70..1ebb7cc2fbe4 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1481,7 +1481,7 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start, { struct vm_area_struct *vma; unsigned long vm_flags;

• int i;

• long i;

/* calculate required read or write permissions. * If FOLL_FORCE is set, we only require the "MAY" flags. -- 2.25.1

Hi Jason,

Thank you for your comments. My replies below.

On Thu, Dec 17, 2020 at 3:50 PM Jason Gunthorpe jgg@ziepe.ca wrote:

On Thu, Dec 17, 2020 at 01:52:41PM -0500, Pavel Tatashin wrote:

...

+/*

• • Verify that there are no unpinnable (movable) pages, if so return true.
• • Otherwise an unpinnable pages is found return false, and unpin all pages.
• */

+static bool check_and_unpin_pages(unsigned long nr_pages, struct page **pages,

•                           unsigned int gup_flags)
  

+{

• unsigned long i, step;
  

• for (i = 0; i < nr_pages; i += step) {
  

•         struct page *head = compound_head(pages[i]);
  

•         step = compound_nr(head) - (pages[i] - head);
  

You can't assume that all of a compound head is in the pages array, this assumption would only work inside the page walkers if the page was found in a PMD or something.

I am not sure I understand your comment. The compound head is not taken from the pages array, and not assumed to be in it. It is exactly the same logic as that we currently have: https://soleen.com/source/xref/linux/mm/gup.c?r=a00cda3f#1565

...

• if (gup_flags & FOLL_PIN) {
  

•         unpin_user_pages(pages, nr_pages);
  

So we throw everything away? Why? That isn't how the old algorithm worked

It is exactly like the old algorithm worked: if there are pages to be migrated (not pinnable pages) we unpinned everything. See here: https://soleen.com/source/xref/linux/mm/gup.c?r=a00cda3f#1603

If cma_pages_list is not empty unpin everything. The list is not empty if we isolated some pages, we isolated some pages if there are some pages which are not pinnable. Now, we do exactly the same thing, but cleaner, and handle errors. We must unpin everything because if we fail, no pages should stay pinned, and also if we migrated some pages, the pages array must be updated, so we need to call __get_user_pages_locked() pin and repopulated pages array.

...

@@ -1654,22 +1664,55 @@ static long __gup_longterm_locked(struct mm_struct *mm, struct vm_area_struct **vmas, unsigned int gup_flags) {

• unsigned long flags = 0;
  

• int migrate_retry = 0;
  

• int isolate_retry = 0;
  

• unsigned int flags;
  long rc;
  
  

• if (gup_flags & FOLL_LONGTERM)
  

•         flags = memalloc_pin_save();
  

• if (!(gup_flags & FOLL_LONGTERM))
  

•         return __get_user_pages_locked(mm, start, nr_pages, pages, vmas,
  

•                                        NULL, gup_flags);
  
  

• rc = __get_user_pages_locked(mm, start, nr_pages, pages, vmas, NULL,
  

•                              gup_flags);
  

• /*
  

•  * Without FOLL_WRITE fault handler may return zero page, which can
  

•  * be in a movable zone, and also will fail to isolate during migration,
  

•  * thus the longterm pin will fail.
  

•  */
  

• gup_flags &= FOLL_WRITE;
  

Is &= what you mean here? |= right?

Right, I meant |=.

Seems like we've ended up in a weird place if FOLL_LONGTERM always includes FOLL_WRITE. Putting the zero page in ZONE_MOVABLE seems like a bad idea, no?

I am not sure, I just found this problem during testing, and this is the solution I am proposing. I am worried about limiting the zero page to a non movable zone, but let's see what others think about this.

...

• /*
  

•  * Migration may fail, we retry before giving up. Also, because after
  

•  * migration pages[] becomes outdated, we unpin and repin all pages
  

•  * in the range, so pages array is repopulated with new values.
  

•  * Also, because of this we cannot retry migration failures in a loop
  

•  * without pinning/unpinnig pages.
  

•  */
  

The old algorithm made continuous forward progress and only went back to the first migration point.

That is not right, the old code went back to the beginning. Making continuous progress is possible, but we won't see any performance betnefit from it, because migration failures is already exception scenarios where machine is under memory stress. The truth is if we fail to migrate it is unlikely will succeed if we retry right away, so giving some time between retries may be even beneficial. Also with continious progress we need to take care of some corner cases where we need to unpin already succeeded pages in case if forward progress is not possible. Also, adjust pages array, start address etc.

...

• for (; ; ) {
  

while (true)?

Hm, the same thing? :)

...

•         rc = __get_user_pages_locked(mm, start, nr_pages, pages, vmas,
  

•                                      NULL, gup_flags);
  

...

•         /* Return if error or if all pages are pinnable */
  

•         if (rc <= 0 || check_and_unpin_pages(rc, pages, gup_flags))
  

•                 break;
  

So we sweep the pages list twice now?

Yes, but O(N) is the same. No new operation is added. Before we had something like this:

while (npages) check if pinnable isolate while (npages) unpin migrate while (npages) pin

Now: while(npages) check if pinnable while(npages) unpin while(npages) isolate migrate pin

...

•         /* Some pages are not pinnable, migrate them */
  

•         rc = migrate_movable_pages(rc, pages);
  

•         /*
  

•          * If there is an error, and we tried maximum number of times
  

•          * bail out. Notice: we return an error code, and all pages are
  

•          * unpinned
  

•          */
  

•         if (rc < 0 && migrate_retry++ >= PINNABLE_MIGRATE_MAX) {
  

•                 break;
  

•         } else if (rc > 0 && isolate_retry++ >= PINNABLE_ISOLATE_MAX) {
  

•                 rc = -EBUSY;
  

I don't like this at all. It shouldn't be so flakey

Can you do migration without the LRU?

I do not think it is possible, we must isolate pages before migration.

Pasha

On 12/17/20 10:52 AM, Pavel Tatashin wrote:

In gup_test both gup_flags and test_flags use the same flags field. This is broken, because gup_flags can be passed as raw value (via -F hex), which can overwrite all the test flags.

Thanks for finding and fixing the "stuck at 0x1" bug!

The test is not quite as broken as you think, though, because you're not looking at the history and intent of the FOLL_WRITE, and how that is used. And that is leading you to make wider "fixes" than I'd recommend.

The new -F command line switch is the latest, newest (and therefore flakiest) addition, and *that* part is worth fixing up, because in addition to being stuck at 0x1 as you noticed, it also misleads everyone into thinking that it's intended for general gup flags. It's not.

It really was meant to be a subtest control flag (controlling the behavior of the dump pages sub-test), as you can see in the commit log and documentation:

For example:

./gup_test -ct -F 1 0 19 0x1000

Meaning: -c: dump pages sub-test -t: use THP pages -F 1: use pin_user_pages() instead of get_user_pages() 0 19 0x1000: dump pages 0, 19, and 4096

Farther, in the actual gup_test.c all the passed gup_flags are erased and unconditionally replaced with FOLL_WRITE.

This is still desirable; Kirill's original intention of only allowing FOLL_WRITE or nothing is still what we want. There is no desire to pass in general gup_flags.

Which means that test_flags are ignored, and code like this always performs pin dump test:

155 if (gup->flags & GUP_TEST_FLAG_DUMP_PAGES_USE_PIN) 156 nr = pin_user_pages(addr, nr, gup->flags, 157 pages + i, NULL); 158 else 159 nr = get_user_pages(addr, nr, gup->flags, 160 pages + i, NULL); 161 break;

Add a new test_flags field, to allow raw gup_flags to work.

I think .test_control_flags field would be a good name, to make it very clear that it's not destined for gup_flags. Just .test_flags is not quite as clear a distinction from .gup_flags, as .test_control_flags is, IMHO.

Add a new subcommand for DUMP_USER_PAGES_TEST to specify that pin test should be performed.

It's arguably better to just keep using the -c option instead, plus the new .test_control_flags field. Otherwise you get a multiplication of possibilities (and therefore, of command line options).

Remove unconditional overwriting of gup_flags via FOLL_WRITE. But,

Nope, let's not do that.

preserve the previous behaviour where FOLL_WRITE was the default flag, and add a new option "-W" to unset FOLL_WRITE.

Or that either.

With the fix, dump works like this:

root@virtme:/# gup_test -c ---- page #0, starting from user virt addr: 0x7f8acb9e4000 page:00000000d3d2ee27 refcount:2 mapcount:1 mapping:0000000000000000 index:0x0 pfn:0x100bcf anon flags: 0x300000000080016(referenced|uptodate|lru|swapbacked) raw: 0300000000080016 ffffd0e204021608 ffffd0e208df2e88 ffff8ea04243ec61 raw: 0000000000000000 0000000000000000 0000000200000000 0000000000000000 page dumped because: gup_test: dump_pages() test DUMP_USER_PAGES_TEST: done

root@virtme:/# gup_test -c -p ---- page #0, starting from user virt addr: 0x7fd19701b000 page:00000000baed3c7d refcount:1025 mapcount:1 mapping:0000000000000000 index:0x0 pfn:0x108008 anon flags: 0x300000000080014(uptodate|lru|swapbacked) raw: 0300000000080014 ffffd0e204200188 ffffd0e205e09088 ffff8ea04243ee71 raw: 0000000000000000 0000000000000000 0000040100000000 0000000000000000 page dumped because: gup_test: dump_pages() test DUMP_USER_PAGES_TEST: done

Refcount shows the difference between pin vs no-pin case. Also change type of nr from int to long, as it counts number of pages.

Why? Is there a bug? gup only handles int sized amounts of pages so this is just adding to the churn, yes?

Signed-off-by: Pavel Tatashin pasha.tatashin@soleen.com

mm/gup_test.c | 9 +++------ mm/gup_test.h | 1 + tools/testing/selftests/vm/gup_test.c | 11 +++++++++-- 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/mm/gup_test.c b/mm/gup_test.c index e3cf78e5873e..24c70c5814ba 100644 --- a/mm/gup_test.c +++ b/mm/gup_test.c @@ -94,7 +94,7 @@ static int __gup_test_ioctl(unsigned int cmd, { ktime_t start_time, end_time; unsigned long i, nr_pages, addr, next;

• int nr;

• long nr; struct page **pages; int ret = 0; bool needs_mmap_lock =

@@ -126,9 +126,6 @@ static int __gup_test_ioctl(unsigned int cmd, nr = (next - addr) / PAGE_SIZE; }

• /* Filter out most gup flags: only allow a tiny subset here: */
  

• gup->flags &= FOLL_WRITE;
  

• switch (cmd) { case GUP_FAST_BENCHMARK: nr = get_user_pages_fast(addr, nr, gup->flags,

@@ -152,7 +149,7 @@ static int __gup_test_ioctl(unsigned int cmd, pages + i, NULL); break; case DUMP_USER_PAGES_TEST:

• 	if (gup->flags & GUP_TEST_FLAG_DUMP_PAGES_USE_PIN)
  

• 	if (gup->test_flags & GUP_TEST_FLAG_DUMP_PAGES_USE_PIN)
  	nr = pin_user_pages(addr, nr, gup->flags,
  			    pages + i, NULL);
  else
  

@@ -187,7 +184,7 @@ static int __gup_test_ioctl(unsigned int cmd, start_time = ktime_get();

• put_back_pages(cmd, pages, nr_pages, gup->flags);

• put_back_pages(cmd, pages, nr_pages, gup->test_flags);

end_time = ktime_get(); gup->put_delta_usec = ktime_us_delta(end_time, start_time); diff --git a/mm/gup_test.h b/mm/gup_test.h index 90a6713d50eb..b1b376b5d3b2 100644 --- a/mm/gup_test.h +++ b/mm/gup_test.h @@ -22,6 +22,7 @@ struct gup_test { __u64 size; __u32 nr_pages_per_call; __u32 flags;

• __u32 test_flags; /*
  • Each non-zero entry is the number of the page (1-based: first page is
  • page 1, so that zero entries mean "do nothing") from the .addr base.

diff --git a/tools/testing/selftests/vm/gup_test.c b/tools/testing/selftests/vm/gup_test.c index 6c6336dd3b7f..42c71483729f 100644 --- a/tools/testing/selftests/vm/gup_test.c +++ b/tools/testing/selftests/vm/gup_test.c @@ -37,13 +37,13 @@ int main(int argc, char **argv) { struct gup_test gup = { 0 }; unsigned long size = 128 * MB;

• int i, fd, filed, opt, nr_pages = 1, thp = -1, repeats = 1, write = 0;

• int i, fd, filed, opt, nr_pages = 1, thp = -1, repeats = 1, write = 1;

Let's leave that alone, as noted above.

unsigned long cmd = GUP_FAST_BENCHMARK; int flags = MAP_PRIVATE; char *file = "/dev/zero"; char *p;

• while ((opt = getopt(argc, argv, "m:r:n:F:f:abctTLUuwSH")) != -1) {

• while ((opt = getopt(argc, argv, "m:r:n:F:f:abctTLUuwWSHp")) != -1) {

And also as noted above I don't think we need this or any of the remaining hunks below.

switch (opt) {
case 'a':
	cmd = PIN_FAST_BENCHMARK;

@@ -65,6 +65,10 @@ int main(int argc, char **argv) */ gup.which_pages[0] = 1; break;

• case 'p':
  

• 	/* works only with DUMP_USER_PAGES_TEST */
  

• 	gup.test_flags |= GUP_TEST_FLAG_DUMP_PAGES_USE_PIN;
  

• 	break;
  

  case 'F': /* strtol, so you can pass flags in hex form */ gup.flags = strtol(optarg, 0, 0);

@@ -93,6 +97,9 @@ int main(int argc, char **argv) case 'w': write = 1; break;

• case 'W':
  

• 	write = 0;
  

• 	break;
  

  case 'f': file = optarg; break;

thanks,

On Sat, Dec 19, 2020 at 12:57 AM John Hubbard jhubbard@nvidia.com wrote:

On 12/17/20 10:52 AM, Pavel Tatashin wrote:

...

Hi Pavel,

This all looks good pretty good to me, with just a couple of minor doubts interleaved with the documentation tweaks:

a) I'm not yet sure if the is_pinnable_page() concept is a keeper. If it's not for some reason, then we should revisit this patch.

b) I don't yet understand why FOLL_TOUCH from gup/pup is a critical part of the test.

...

When pages are pinned they can be faulted in userland and migrated, and they can be faulted right in kernel without migration.

In either case, the pinned pages must end-up being pinnable (not movable).

Let's delete the above two sentences, which are confusing as currently worded, and just keep approximately the last sentence below.

Sure.

...

Add a new test without touching pages in userland, and use FOLL_TOUCH instead. Also, verify that pinned pages are pinnable.

Maybe this instead:

Add a new test to gup_test, to verify that only "pinnable" pages are pinned. Also, use gup/pup + FOLL_TOUCH to fault in the pages, rather than faulting them in from user space.

OK

? But I don't know why that second point is important. Is it actually important in order to have a valid test? If so, why?

It is crucial because when pages are faulted from userland they may end up in a movable zone, we spend time migrating them during pinning: performance suffers. When pages are faulted from the kernel, they do not need to be migrated as they should end-up in the right zone from the beginning, and performance should be fast.

Here is sample run to pin 512M 4096 pages at a time:

Before my changes: Fault in userland: get:4787 put:1093 Fault in kernel: get:80577 put:1187 In both cases pages in movable zone were pinned. The fault in kernel is slower compared to userland, because pages need to be faulted and zeroed before returning.

With my changes: Fault in userland: get:150793 put:1111 Fault in kernel: get:81066 put:1091 In both cases pages were pinned in correct zone. The fault in kernel is faster than userland, because userland pages needed to be migrated from the movable zone.

...

Signed-off-by: Pavel Tatashin pasha.tatashin@soleen.com

mm/gup_test.c | 6 ++++++ tools/testing/selftests/vm/gup_test.c | 17 +++++++++++++---- 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/mm/gup_test.c b/mm/gup_test.c index 24c70c5814ba..24fd542091ee 100644 --- a/mm/gup_test.c +++ b/mm/gup_test.c @@ -52,6 +52,12 @@ static void verify_dma_pinned(unsigned int cmd, struct page **pages,

                          dump_page(page, "gup_test failure");
                          break;

•                 } else if (cmd == PIN_LONGTERM_BENCHMARK &&
  

•                         WARN(!is_pinnable_page(page),
  

•                              "pages[%lu] is NOT pinnable but pinned\n",
  

•                              i)) {
  

•                         dump_page(page, "gup_test failure");
  

•                         break;
                  }
          }
          break;
  

diff --git a/tools/testing/selftests/vm/gup_test.c b/tools/testing/selftests/vm/gup_test.c index 42c71483729f..f08cc97d424d 100644 --- a/tools/testing/selftests/vm/gup_test.c +++ b/tools/testing/selftests/vm/gup_test.c @@ -13,6 +13,7 @@

/* Just the flags we need, copied from mm.h: */ #define FOLL_WRITE 0x01 /* check pte is writable */ +#define FOLL_TOUCH 0x02 /* mark page accessed */

Aha, now I see why you wanted to pass other GUP flags, in the previous patch. I think it's OK to pass this set of possible flags (as .gup_flags) through ioctl, yes.

Sure, and I will rename .flags to .gup_flags as you previously suggested.

However (this is about the previous patch), I *think* we're better off leaving the gup_test behavior as: "default is read-only pages, but you can pass in -w to specify FOLL_WRITE". As opposed to passing in raw flags from the command line. And yes, I realize that my -F option seemed to recommand the latter...I'm regretting that -F approach now.

But, that would reverse the current default behaviour where FOLL_WRITE is always set. I introduced "-W" not to break backward compatibility where the "-w" option was already available, but since "-w" is the default it basically does nothing, where "-W '' removes the FOLL_WRITE. Do you want to reverse the current behaviour?

The other direction to go might be to stop doing that, and shift over to just let the user specify FOLL_* flags directly on the command line, but IMHO there's no need for that (yet), and it's a little less error-prone to constrain it.

This leads to: change the "-F 1", to some other better-named option, perhaps. Open to suggestion there.

Perhaps disable "-F n" and print a warning about the ignored flag until it is found to be useful?

...

static char *cmd_to_str(unsigned long cmd) { @@ -39,11 +40,11 @@ int main(int argc, char **argv) unsigned long size = 128 * MB; int i, fd, filed, opt, nr_pages = 1, thp = -1, repeats = 1, write = 1; unsigned long cmd = GUP_FAST_BENCHMARK;

• int flags = MAP_PRIVATE;
  

• int flags = MAP_PRIVATE, touch = 0;
  

Silly nit, can we put it on its own line? This pre-existing mess of declarations makes it hard to read everything. One item per line is easier on the reader, who is often just looking for a single item at a time. Actually why not rename it slightly while we're here (see below), maybe to this:

    int use_foll_touch = 0;

Sure, I will move it. I also prefer one initialization per-line, but tried to keep the current style. I can also correct the other initializations in this function.

...

  char *file = "/dev/zero";
  char *p;

• while ((opt = getopt(argc, argv, "m:r:n:F:f:abctTLUuwWSHp")) != -1) {
  

• while ((opt = getopt(argc, argv, "m:r:n:F:f:abctTLUuwWSHpz")) != -1) {
  

Yes, this seems worth its own command line option.

...

          switch (opt) {
          case 'a':
                  cmd = PIN_FAST_BENCHMARK;

@@ -110,6 +111,10 @@ int main(int argc, char **argv) case 'H': flags |= (MAP_HUGETLB | MAP_ANONYMOUS); break;

•         case 'z':
  

•                 /* fault pages in gup, do not fault in userland */
  

How about: /* * Use gup/pup(FOLL_TOUCH), *instead* of faulting * pages in from user space. */ use_foll_touch = 1;

Sure.

...

•                 touch = 1;
  

•                 break;
          default:
                  return -1;
          }
  

@@ -167,8 +172,12 @@ int main(int argc, char **argv) else if (thp == 0) madvise(p, size, MADV_NOHUGEPAGE);

• for (; (unsigned long)p < gup.addr + size; p += PAGE_SIZE)
  

•         p[0] = 0;
  

• if (touch) {
  

•         gup.flags |= FOLL_TOUCH;
  

• } else {
  

•         for (; (unsigned long)p < gup.addr + size; p += PAGE_SIZE)
  

•                 p[0] = 0;
  

• }
  

OK.

Thank you for your review.

Pahsa