Create 'old' ptes for faultaround mappings on arm64 with hardware access flag

mm/memory.c +0 -0

@@ -3978,8 +3994,17 @@ static vm_fault_t do_fault_around(struct vm_fault *vmf)
 
        /* check if the page fault is solved */
        vmf->pte -= (vmf->address >> PAGE_SHIFT) - (address >> PAGE_SHIFT);

Quoted 5 lines · Will Deacon

mm/memory.c +0 -0

@@ -3978,8 +3994,17 @@ static vm_fault_t do_fault_around(struct vm_fault *vmf)
 
        /* check if the page fault is solved */
        vmf->pte -= (vmf->address >> PAGE_SHIFT) - (address >> PAGE_SHIFT);

mm/memory.c static vm_fault_t do_fault_around(struct vm_fault *vmf) · line 4004

Quoted 10 lines

mm/memory.c +8 -2

-       if (!pte_none(*vmf->pte))
-               ret = VM_FAULT_NOPAGE;
+       if (pte_none(*vmf->pte))
+               goto out_unlock;
+
+       if (vmf->flags & FAULT_FLAG_PREFAULT_OLD) {
+               pte_t pte = pte_mkyoung(*vmf->pte);
+               if (ptep_set_access_flags(vmf->vma, address, vmf->pte, pte, 0))
+                       update_mmu_cache(vmf->vma, address, vmf->pte);
+       }

Oh, please dear God no.

First you incorrectly set it old, and then you conditionally make it young again and as a result force an atomic rwm update and another TLB flush for no good reason.

Just make sure that the FAULT_FLAG_PREFAULT_OLD never sets the *actual* address to old.

And yes, that probably means that you need to change "alloc_set_pte()" to actually pass in the real address, and leave "vmf->address" alone - so that it can know which ones are prefaulted and which one is real, but that sounds like a good idea anyway.

Then you can just make alloc_set_pte() do the right thing in the first place, instead of doing this nasty "lets do it wrong and fix it up later" horror.

Quoted 5 lines

And yes, that probably means that you need to change "alloc_set_pte()" to actually pass in the real address, and leave "vmf->address" alone - so that it can know which ones are prefaulted and which one is real, but that sounds like a good idea anyway.

Right, I deliberately avoided that based on the feedback from Jan on an older version [1], but I can certainly look at it again.

In particular, it made it a nightmare to read what do_fault_around()
does: it does that odd

        if (pmd_none(*vmf->pmd)) {
                vmf->prealloc_pte = pte_alloc_one(vmf->vma->vm_mm);

and then it calls ->map_pages() (which is always filemap_map_pages(), except for xfs, where it is also always filemap_map_pages but it takes a lock first).

If a filesystem has put an Uptodate page into the page cache, the rest of the kernel can read it without telling the filesystem.

Kirill - I think you know this code best. Would you be willing to look at splitting out that "allocate and map" from alloc_set_pte() and into the callers?

At that point, I think the current very special and odd do_fault_around() pre-allocation could be made into just a _regular_ "allocate the pmd if it doesn't exist". And then the pte locking could be moved into filemap_map_pages(), and suddenly the semantics and rules around all that would be a whole lot more obvious.

See lightly tested patch below. Is it something you had in mind?

Quoted 5 lines

At that point, I think the current very special and odd do_fault_around() pre-allocation could be made into just a _regular_ "allocate the pmd if it doesn't exist". And then the pte locking could be moved into filemap_map_pages(), and suddenly the semantics and rules around all that would be a whole lot more obvious.

No. It would stop faultaround code from mapping huge pages. We had to defer pte page table mapping until we know we don't have huge pages in page cache.

Can we please move that part to the callers too - possibly with a separate helper function?

Here it is. Still barely tested.

I expected to hate it more, but it looks reasonable. Opencoded xas_for_each() smells bad, but...

Quoted 2 lines

I expected to hate it more, but it looks reasonable. Opencoded xas_for_each() smells bad, but...

I think the open-coded xas_for_each() per se isn't a problem, but I agree that the startup condition is a bit ugly. And I'm actually personally more confused by why xas_retry() is needed here, bit not in many other places. That is perhaps more obvious now that it shows up twice.

Adding Willy to the cc in case he has comments on that, and can explain it to me in small words.

[ https://lore.kernel.org/lkml/20201214160724.ewhjqoi32chheone@box/
for context ]

Could we have a "filemap_map_pmd()" that does it all, and then the filemap_map_pages() logic would be more along the lines of

    if (filemap_map_pmd(vmf, xas)) {
            rcu_read_unlock();
            return;
    }

    ... then handle pte's ...

Hmm?

If this looks fine, I'll submit a proper patch.

Quoted 2 lines · Kirill A. Shutemov

If this looks fine, I'll submit a proper patch.

That patch looks good to me.

It would be good if somebody else looked it through - maybe I like it just because I got to pee in the snow and make my mark. But i think that filemap_map_pages() now looks a lot more understandable, and having that pte_offset_map_lock() outside the loop should be good.

Also if the range doesn't have a mappable page we would setup a page table into the PMD entry. It means we cannot have huge page mapped there later. It may be a bummer: getting the page table out of page table tree requires mmap_write_lock().

We also take ptl for cold page cache. It may increase ptl contention, but it should be negligible with split-ptl.

(quoted diff) +9 -0

+       head = xas_find(&xas, end_pgoff);
+       for (; ; head = xas_next_entry(&xas, end_pgoff)) {
+               if (!head) {
+                       rcu_read_unlock();
+                       return;
+               }
+               if (likely(!xas_retry(&xas, head))
+                       break;
+       }

instead. So that if we don't find any cached entries, we won't do anything, rather than take locks and then not do anything.

Then that second loop very naturally becomes a "do { } while ()" one.

This should do. See below.

Quoted 1 line

Then that second loop very naturally becomes a "do { } while ()" one.

I don't see it. I haven't found a reasonable way to rework it do-while.

No?

Okay, but we only win the NULL check. xas_retry() and xa_is_value() has to be repeated in the beginning of the loop.

mm/filemap.c +0 -0

@@ -2884,19 +2966,18 @@ void filemap_map_pages(struct vm_fault *vmf,
                if (vmf->pte)
                        vmf->pte += xas.xa_index - last_pgoff;

Quoted 4 lines · Kirill A. Shutemov

(quoted diff) +0 -0

@@ -2884,19 +2966,18 @@ void filemap_map_pages(struct vm_fault *vmf,
                if (vmf->pte)
                        vmf->pte += xas.xa_index - last_pgoff;

Review void filemap_map_pages(struct vm_fault *vmf, · line 2974

Quoted 10 lines

(quoted diff) +5 -3

                last_pgoff = xas.xa_index;
-               if (alloc_set_pte(vmf, page))
-                       goto unlock;
+               if (pte_none(*vmf->pte))
+                       do_set_pte(vmf, page);
+               /* no need to invalidate: a not-present page won't be cached */
+               update_mmu_cache(vma, vmf->address, vmf->pte);
                unlock_page(head);
-               goto next;
+               continue;

This can't be right.

Look at what happens if "pte_none()" is not true.. It won't install the new pte, but it also won't drop the ref to the page.

So I think it needs to be

(quoted diff) +2 -1

-               if (alloc_set_pte(vmf, page))
+               if (!pte_none(*vmf->pte))
                        goto unlock;
+               do_set_pte(vmf, page);

instead, so that the "if somebody else already filled the page table" case gets handled right.

Updated patch is below.

From 0ec1bc1fe95587350ac4f4c866d6482383740b36 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <[email protected]>
Date: Sat, 19 Dec 2020 15:19:23 +0300
Subject: [PATCH] mm: Cleanup faultaround and finish_fault() codepaths

alloc_set_pte() has two users with different requirements: in the faultaround code, it called from an atomic context and PTE page table has to be preallocated. finish_fault() can sleep and allocate page table as needed.

PTL locking rules are also strange, hard to follow and overkill for finish_fault().

Let's untangle the mess. alloc_set_pte() has gone now. All locking is explicit.

The price is some code duplication to handle huge pages in faultaround path, but it should be fine, having overall improvement in readability.

Signed-off-by Kirill A. Shutemov <[email protected]>

mm/filemap.c +2 -0

index 0b2067b3c328..f8fdbe079375 100644
@@ -2831,10 +2832,74 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
 }
 EXPORT_SYMBOL(filemap_fault);
 
+static bool filemap_map_pmd(struct vm_fault *vmf, struct page *page,
+				  struct xa_state *xas)

mm/filemap.c +13 -0

+
+	if (!page_cache_get_speculative(page))
+		goto nohuge;
+
+	if (page != xas_reload(xas))
+		goto unref;
+
+	if (!PageTransHuge(page))
+		goto unref;
+
+	if (!PageUptodate(page) || PageReadahead(page) || PageHWPoison(page))
+		goto unref;
+

mm/filemap.c +4 -0

+	unlock_page(page);
+	return true;
+unlock:
+	unlock_page(page);

mm/filemap.c +7 -0

+
+	/* See comment in handle_pte_fault() */
+	if (pmd_devmap_trans_unstable(vmf->pmd))
+		return true;
+
+	return false;
+}

It's not ready yet.

The new helper next_page() returns a stablized page, so filemap_map_pmd() can clearly decide if we should set up a page table or a huge page.

Because not only does that get rid of the "if (page)" test, I think it would make things a bit clearer. When I read the patch first, the initial "next_page()" call confused me.

Quoted 3 lines · Linus Torvalds

Because not only does that get rid of the "if (page)" test, I think it would make things a bit clearer. When I read the patch first, the initial "next_page()" call confused me.

Agreed. Here we go:

From d12dea4abe94dbc24b7945329b191ad7d29e213a Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <[email protected]>
Date: Sat, 19 Dec 2020 15:19:23 +0300
Subject: [PATCH] mm: Cleanup faultaround and finish_fault() codepaths

alloc_set_pte() has two users with different requirements: in the faultaround code, it called from an atomic context and PTE page table has to be preallocated. finish_fault() can sleep and allocate page table as needed.

PTL locking rules are also strange, hard to follow and overkill for finish_fault().

Let's untangle the mess. alloc_set_pte() has gone now. All locking is explicit.

The price is some code duplication to handle huge pages in faultaround path, but it should be fine, having overall improvement in readability.

Signed-off-by Kirill A. Shutemov <[email protected]>

Hold on. I guess this one will suffer from the same bug as the previous. I was about to report back, after satisfactory overnight testing of that version - provided that one big little bug is fixed:

Quoted 9 lines · Hugh Dickins

Hold on. I guess this one will suffer from the same bug as the previous. I was about to report back, after satisfactory overnight testing of that version - provided that one big little bug is fixed:

mm/filemap.c static bool filemap_map_pmd(struct vm_fa · line 2925

Quoted 8 lines

mm/filemap.c +1 -1

@@ -2919,7 +2919,7 @@ static bool filemap_map_pmd(struct vm_fa
 
        if (pmd_none(*vmf->pmd) &&
            PageTransHuge(page) &&
-           do_set_pmd(vmf, page)) {
+           do_set_pmd(vmf, page) == 0) {
                unlock_page(page);
                return true;
        }

I missed that entirely, because when just reading the patch it looks fine and I didn't look at what do_set_pmd() function returns outside the patch.

And maybe it would be better to write it as

        if (pmd_none(*vmf->pmd) && PageTransHuge(page)) {
                vm_fault_t ret = do_set_pmd(vmf, page);
                if (!ret) {
                        ...

instead to make it a bit more explicit about how that return value is a vm_fault_t there...

And see my other email about how I suspect there is still a leak in that patch for the previous test-case.

Here's the fixup I have so far. It doesn't blow up immediately, but please take a closer look. Who knows what stupid mistake I did this time. :/

Quoted 3 lines · Kirill A. Shutemov

Here's the fixup I have so far. It doesn't blow up immediately, but please take a closer look. Who knows what stupid mistake I did this time. :/

It's been running fine on x86_64 for a couple of hours (but of course my testing is deficient, in not detecting the case Linus spotted).

But I just thought I'd try it on i386 (hadn't tried previous versions) and this has a new disappointment: crashes when booting, in the "check if the page fault is solved" in do_fault_around(). I imagine a highmem issue with kmap of the pte address, but I'm reporting now before looking into it further (but verified that current linux.git i386 boots up fine).

Maybe easily fixed: but does suggest this needs exposure in linux-next.

Hugh

This patch (like its antecedents) moves the pte_unmap_unlock() from after do_fault_around()'s "check if the page fault is solved" into filemap_map_pages() itself (which apparently does not NULLify vmf->pte after unmapping it, which is poor, but good for revealing this issue). That looks cleaner, but of course there was a very good reason for its original positioning.

Maybe you want to change the ->map_pages prototype, to pass down the requested address too, so that it can report whether the requested address was resolved or not. Or it could be left to __do_fault(), or even to a repeated fault; but those would be less efficient.

Quoted 7 lines · Hugh Dickins

This patch (like its antecedents) moves the pte_unmap_unlock() from after do_fault_around()'s "check if the page fault is solved" into filemap_map_pages() itself (which apparently does not NULLify vmf->pte after unmapping it, which is poor, but good for revealing this issue). That looks cleaner, but of course there was a very good reason for its original positioning.

Good catch.

Quoted 4 lines

Maybe you want to change the ->map_pages prototype, to pass down the requested address too, so that it can report whether the requested address was resolved or not. Or it could be left to __do_fault(), or even to a repeated fault; but those would be less efficient.

Let's keep the old really odd "let's unlock in the caller" for now, and minimize the changes.

Adding a big big comment at the end of filemap_map_pages() to note the odd delayed page table unlocking.

Here's an updated patch that combines Kirill's original patch, his additional incremental patch, and the fix for the pte lock oddity into one thing.

Does this finally pass your testing?

Linus

From 4d221d934d112aa40c3f4978460be098fc9ce831 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <[email protected]>
Date: Sat, 19 Dec 2020 15:19:23 +0300
Subject: [PATCH] mm: Cleanup faultaround and finish_fault() codepaths

alloc_set_pte() has two users with different requirements: in the faultaround code, it called from an atomic context and PTE page table has to be preallocated. finish_fault() can sleep and allocate page table as needed.

PTL locking rules are also strange, hard to follow and overkill for finish_fault().

Let's untangle the mess. alloc_set_pte() has gone now. All locking is explicit.

The price is some code duplication to handle huge pages in faultaround path, but it should be fine, having overall improvement in readability.

Signed-off-by Kirill A. Shutemov <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

---
 include/linux/mm.h      |   8 +-
 include/linux/pgtable.h |  11 +++
 mm/filemap.c            | 168 ++++++++++++++++++++++++++++++----------
 mm/memory.c             | 161 +++++++++++---------------------------
 4 files changed, 192 insertions(+), 156 deletions(-)

include/linux/mm.h +5 -3

index 5299b90a6c40..c0643a0ad5ff 100644
@@ -535,8 +535,8 @@ struct vm_fault {
 					 * is not NULL, otherwise pmd.
 					 */
 	pgtable_t prealloc_pte;		/* Pre-allocated pte page table.
-					 * vm_ops->map_pages() calls
-					 * alloc_set_pte() from atomic context.
+					 * vm_ops->map_pages() sets up a page
+					 * table from from atomic context.
 					 * do_fault_around() pre-allocates
 					 * page table to avoid allocation from
 					 * atomic context.

@@ -981,7 +981,9 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
 	return pte;
 }
 
-vm_fault_t alloc_set_pte(struct vm_fault *vmf, struct page *page);
+vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page);
+void do_set_pte(struct vm_fault *vmf, struct page *page);
+
 vm_fault_t finish_fault(struct vm_fault *vmf);
 vm_fault_t finish_mkwrite_fault(struct vm_fault *vmf);
 #endif

include/linux/pgtable.h +11 -0

index 8fcdfa52eb4b..36eb748f3c97 100644
@@ -1314,6 +1314,17 @@ static inline int pmd_trans_unstable(pmd_t *pmd)
 #endif
 }
 
+/*
+ * the ordering of these checks is important for pmds with _page_devmap set.
+ * if we check pmd_trans_unstable() first we will trip the bad_pmd() check
+ * inside of pmd_none_or_trans_huge_or_clear_bad(). this will end up correctly
+ * returning 1 but not before it spams dmesg with the pmd_clear_bad() output.
+ */
+static inline int pmd_devmap_trans_unstable(pmd_t *pmd)
+{
+	return pmd_devmap(*pmd) || pmd_trans_unstable(pmd);
+}
+
 #ifndef CONFIG_NUMA_BALANCING
 /*
  * Technically a PTE can be PROTNONE even when not doing NUMA balancing but

mm/filemap.c +129 -39

index 5c9d564317a5..dbc2eda92a53 100644
@@ -42,6 +42,7 @@
 #include <linux/psi.h>
 #include <linux/ramfs.h>
 #include <linux/page_idle.h>
+#include <asm/pgalloc.h>
 #include "internal.h"
 
 #define CREATE_TRACE_POINTS

@@ -2911,50 +2912,133 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
 }
 EXPORT_SYMBOL(filemap_fault);
 
+static bool filemap_map_pmd(struct vm_fault *vmf, struct page *page)
+{
+	struct mm_struct *mm = vmf->vma->vm_mm;
+
+	/* Huge page is mapped? No need to proceed. */
+	if (pmd_trans_huge(*vmf->pmd)) {
+		unlock_page(page);
+		put_page(page);
+		return true;
+	}
+
+	if (pmd_none(*vmf->pmd) && PageTransHuge(page)) {
+	    vm_fault_t ret = do_set_pmd(vmf, page);
+	    if (!ret) {
+		    /* The page is mapped successfully, reference consumed. */
+		    unlock_page(page);
+		    return true;
+	    }
+	}
+
+	if (pmd_none(*vmf->pmd)) {
+		vmf->ptl = pmd_lock(mm, vmf->pmd);
+		if (likely(pmd_none(*vmf->pmd))) {
+			mm_inc_nr_ptes(mm);
+			pmd_populate(mm, vmf->pmd, vmf->prealloc_pte);
+			vmf->prealloc_pte = NULL;
+		}
+		spin_unlock(vmf->ptl);
+	}
+
+	/* See comment in handle_pte_fault() */
+	if (pmd_devmap_trans_unstable(vmf->pmd)) {
+		unlock_page(page);
+		put_page(page);
+		return true;
+	}
+
+	return false;
+}
+
+static struct page *next_uptodate_page(struct page *page, struct vm_fault *vmf,
+				     struct xa_state *xas, pgoff_t end_pgoff)
+{
+	struct address_space *mapping = vmf->vma->vm_file->f_mapping;
+	unsigned long max_idx;
+
+	do {
+		if (!page)
+			return NULL;
+		if (xas_retry(xas, page))
+			continue;
+		if (xa_is_value(page))
+			continue;
+		if (PageLocked(page))
+			continue;
+		if (!page_cache_get_speculative(page))
+			continue;
+		/* Has the page moved or been split? */
+		if (unlikely(page != xas_reload(xas)))
+			goto skip;
+		if (!PageUptodate(page) || PageReadahead(page))
+			goto skip;
+		if (PageHWPoison(page))
+			goto skip;
+		if (!trylock_page(page))
+			goto skip;
+		if (page->mapping != mapping)
+			goto unlock;
+		if (!PageUptodate(page))
+			goto unlock;
+		max_idx = DIV_ROUND_UP(i_size_read(mapping->host), PAGE_SIZE);
+		if (xas->xa_index >= max_idx)
+			goto unlock;
+		return page;
+unlock:
+		unlock_page(page);
+skip:
+		put_page(page);
+	} while ((page = xas_next_entry(xas, end_pgoff)) != NULL);
+
+	return NULL;
+}
+
+static inline struct page *first_map_page(struct vm_fault *vmf,
+					  struct xa_state *xas,
+					  pgoff_t end_pgoff)
+{
+	return next_uptodate_page(xas_find(xas, end_pgoff),
+				  vmf, xas, end_pgoff);
+}
+
+static inline struct page *next_map_page(struct vm_fault *vmf,
+					  struct xa_state *xas,
+					  pgoff_t end_pgoff)
+{
+	return next_uptodate_page(xas_next_entry(xas, end_pgoff),
+				  vmf, xas, end_pgoff);
+}
+
 void filemap_map_pages(struct vm_fault *vmf,
 		pgoff_t start_pgoff, pgoff_t end_pgoff)
 {
-	struct file *file = vmf->vma->vm_file;
+	struct vm_area_struct *vma = vmf->vma;
+	struct file *file = vma->vm_file;
 	struct address_space *mapping = file->f_mapping;
 	pgoff_t last_pgoff = start_pgoff;
-	unsigned long max_idx;
 	XA_STATE(xas, &mapping->i_pages, start_pgoff);
 	struct page *head, *page;
 	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
 
 	rcu_read_lock();
-	xas_for_each(&xas, head, end_pgoff) {
-		if (xas_retry(&xas, head))
-			continue;
-		if (xa_is_value(head))
-			goto next;
+	head = first_map_page(vmf, &xas, end_pgoff);
+	if (!head) {
+		rcu_read_unlock();
+		return;
+	}
 
-		/*
-		 * Check for a locked page first, as a speculative
-		 * reference may adversely influence page migration.
-		 */
-		if (PageLocked(head))
-			goto next;
-		if (!page_cache_get_speculative(head))
-			goto next;
+	if (filemap_map_pmd(vmf, head)) {
+		rcu_read_unlock();
+		return;
+	}
 
-		/* Has the page moved or been split? */
-		if (unlikely(head != xas_reload(&xas)))
-			goto skip;
+	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
+				       vmf->address, &vmf->ptl);
+	do {
 		page = find_subpage(head, xas.xa_index);
-
-		if (!PageUptodate(head) ||
-				PageReadahead(page) ||
-				PageHWPoison(page))
-			goto skip;
-		if (!trylock_page(head))
-			goto skip;
-
-		if (head->mapping != mapping || !PageUptodate(head))
-			goto unlock;
-
-		max_idx = DIV_ROUND_UP(i_size_read(mapping->host), PAGE_SIZE);
-		if (xas.xa_index >= max_idx)
+		if (PageHWPoison(page))
 			goto unlock;
 
 		if (mmap_miss > 0)

@@ -2964,19 +3048,25 @@ void filemap_map_pages(struct vm_fault *vmf,
 		if (vmf->pte)
 			vmf->pte += xas.xa_index - last_pgoff;
 		last_pgoff = xas.xa_index;
-		if (alloc_set_pte(vmf, page))
+
+		if (!pte_none(*vmf->pte))
 			goto unlock;
+
+		do_set_pte(vmf, page);
+		/* no need to invalidate: a not-present page won't be cached */
+		update_mmu_cache(vma, vmf->address, vmf->pte);
 		unlock_page(head);
-		goto next;
+		continue;
 unlock:
 		unlock_page(head);
-skip:
 		put_page(head);
-next:
-		/* Huge page is mapped? No need to proceed. */
-		if (pmd_trans_huge(*vmf->pmd))
-			break;
-	}
+	} while ((head = next_map_page(vmf, &xas, end_pgoff)) != NULL);
+
+	/*
+	 * NOTE! We return with the pte still locked! It is unlocked
+	 * by do_fault_around() after it has tested whether the target
+	 * address got filled in.
+	 */
 	rcu_read_unlock();
 	WRITE_ONCE(file->f_ra.mmap_miss, mmap_miss);
 }

mm/memory.c +47 -114

index 7d608765932b..07a408c7d38b 100644
@@ -3501,7 +3501,7 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
 	if (pte_alloc(vma->vm_mm, vmf->pmd))
 		return VM_FAULT_OOM;
 
-	/* See the comment in pte_alloc_one_map() */
+	/* See comment in handle_pte_fault() */
 	if (unlikely(pmd_trans_unstable(vmf->pmd)))
 		return 0;
 

@@ -3641,66 +3641,6 @@ static vm_fault_t __do_fault(struct vm_fault *vmf)
 	return ret;
 }
 
-/*
- * The ordering of these checks is important for pmds with _PAGE_DEVMAP set.
- * If we check pmd_trans_unstable() first we will trip the bad_pmd() check
- * inside of pmd_none_or_trans_huge_or_clear_bad(). This will end up correctly
- * returning 1 but not before it spams dmesg with the pmd_clear_bad() output.
- */
-static int pmd_devmap_trans_unstable(pmd_t *pmd)
-{
-	return pmd_devmap(*pmd) || pmd_trans_unstable(pmd);
-}
-
-static vm_fault_t pte_alloc_one_map(struct vm_fault *vmf)
-{
-	struct vm_area_struct *vma = vmf->vma;
-
-	if (!pmd_none(*vmf->pmd))
-		goto map_pte;
-	if (vmf->prealloc_pte) {
-		vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
-		if (unlikely(!pmd_none(*vmf->pmd))) {
-			spin_unlock(vmf->ptl);
-			goto map_pte;
-		}
-
-		mm_inc_nr_ptes(vma->vm_mm);
-		pmd_populate(vma->vm_mm, vmf->pmd, vmf->prealloc_pte);
-		spin_unlock(vmf->ptl);
-		vmf->prealloc_pte = NULL;
-	} else if (unlikely(pte_alloc(vma->vm_mm, vmf->pmd))) {
-		return VM_FAULT_OOM;
-	}
-map_pte:
-	/*
-	 * If a huge pmd materialized under us just retry later.  Use
-	 * pmd_trans_unstable() via pmd_devmap_trans_unstable() instead of
-	 * pmd_trans_huge() to ensure the pmd didn't become pmd_trans_huge
-	 * under us and then back to pmd_none, as a result of MADV_DONTNEED
-	 * running immediately after a huge pmd fault in a different thread of
-	 * this mm, in turn leading to a misleading pmd_trans_huge() retval.
-	 * All we have to ensure is that it is a regular pmd that we can walk
-	 * with pte_offset_map() and we can do that through an atomic read in
-	 * C, which is what pmd_trans_unstable() provides.
-	 */
-	if (pmd_devmap_trans_unstable(vmf->pmd))
-		return VM_FAULT_NOPAGE;
-
-	/*
-	 * At this point we know that our vmf->pmd points to a page of ptes
-	 * and it cannot become pmd_none(), pmd_devmap() or pmd_trans_huge()
-	 * for the duration of the fault.  If a racing MADV_DONTNEED runs and
-	 * we zap the ptes pointed to by our vmf->pmd, the vmf->ptl will still
-	 * be valid and we will re-check to make sure the vmf->pte isn't
-	 * pte_none() under vmf->ptl protection when we return to
-	 * alloc_set_pte().
-	 */
-	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
-			&vmf->ptl);
-	return 0;
-}
-
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 static void deposit_prealloc_pte(struct vm_fault *vmf)
 {

@@ -3715,7 +3655,7 @@ static void deposit_prealloc_pte(struct vm_fault *vmf)
 	vmf->prealloc_pte = NULL;
 }
 
-static vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page)
+vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page)
 {
 	struct vm_area_struct *vma = vmf->vma;
 	bool write = vmf->flags & FAULT_FLAG_WRITE;

@@ -3780,45 +3720,11 @@ static vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page)
 }
 #endif
 
-/**
- * alloc_set_pte - setup new PTE entry for given page and add reverse page
- * mapping. If needed, the function allocates page table or use pre-allocated.
- *
- * @vmf: fault environment
- * @page: page to map
- *
- * Caller must take care of unlocking vmf->ptl, if vmf->pte is non-NULL on
- * return.
- *
- * Target users are page handler itself and implementations of
- * vm_ops->map_pages.
- *
- * Return: %0 on success, %VM_FAULT_ code in case of error.
- */
-vm_fault_t alloc_set_pte(struct vm_fault *vmf, struct page *page)
+void do_set_pte(struct vm_fault *vmf, struct page *page)
 {
 	struct vm_area_struct *vma = vmf->vma;
 	bool write = vmf->flags & FAULT_FLAG_WRITE;
 	pte_t entry;
-	vm_fault_t ret;
-
-	if (pmd_none(*vmf->pmd) && PageTransCompound(page)) {
-		ret = do_set_pmd(vmf, page);
-		if (ret != VM_FAULT_FALLBACK)
-			return ret;
-	}
-
-	if (!vmf->pte) {
-		ret = pte_alloc_one_map(vmf);
-		if (ret)
-			return ret;
-	}
-
-	/* Re-check under ptl */
-	if (unlikely(!pte_none(*vmf->pte))) {
-		update_mmu_tlb(vma, vmf->address, vmf->pte);
-		return VM_FAULT_NOPAGE;
-	}
 
 	flush_icache_page(vma, page);
 	entry = mk_pte(page, vma->vm_page_prot);

@@ -3835,14 +3741,8 @@ vm_fault_t alloc_set_pte(struct vm_fault *vmf, struct page *page)
 		page_add_file_rmap(page, false);
 	}
 	set_pte_at(vma->vm_mm, vmf->address, vmf->pte, entry);
-
-	/* no need to invalidate: a not-present page won't be cached */
-	update_mmu_cache(vma, vmf->address, vmf->pte);
-
-	return 0;
 }
 
-
 /**
  * finish_fault - finish page fault once we have prepared the page to fault
  *

@@ -3860,12 +3760,12 @@ vm_fault_t alloc_set_pte(struct vm_fault *vmf, struct page *page)
  */
 vm_fault_t finish_fault(struct vm_fault *vmf)
 {
+	struct vm_area_struct *vma = vmf->vma;
 	struct page *page;
-	vm_fault_t ret = 0;
+	vm_fault_t ret;
 
 	/* Did we COW the page? */
-	if ((vmf->flags & FAULT_FLAG_WRITE) &&
-	    !(vmf->vma->vm_flags & VM_SHARED))
+	if ((vmf->flags & FAULT_FLAG_WRITE) && !(vma->vm_flags & VM_SHARED))
 		page = vmf->cow_page;
 	else
 		page = vmf->page;

@@ -3874,13 +3774,35 @@ vm_fault_t finish_fault(struct vm_fault *vmf)
 	 * check even for read faults because we might have lost our CoWed
 	 * page
 	 */
-	if (!(vmf->vma->vm_flags & VM_SHARED))
-		ret = check_stable_address_space(vmf->vma->vm_mm);
-	if (!ret)
-		ret = alloc_set_pte(vmf, page);
-	if (vmf->pte)
-		pte_unmap_unlock(vmf->pte, vmf->ptl);
-	return ret;
+	if (!(vma->vm_flags & VM_SHARED))
+		ret = check_stable_address_space(vma->vm_mm);
+	if (ret)
+		return ret;
+
+	if (pmd_none(*vmf->pmd)) {
+		if (PageTransCompound(page)) {
+			ret = do_set_pmd(vmf, page);
+			if (ret != VM_FAULT_FALLBACK)
+				return ret;
+		}
+
+		if (unlikely(pte_alloc(vma->vm_mm, vmf->pmd)))
+			return VM_FAULT_OOM;
+	}
+
+	/* See comment in handle_pte_fault() */
+	if (pmd_devmap_trans_unstable(vmf->pmd))
+		return 0;
+
+	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
+				      vmf->address, &vmf->ptl);
+	/* Re-check under ptl */
+	if (likely(pte_none(*vmf->pte)))
+		do_set_pte(vmf, page);
+
+	update_mmu_tlb(vma, vmf->address, vmf->pte);
+	pte_unmap_unlock(vmf->pte, vmf->ptl);
+	return 0;
 }
 
 static unsigned long fault_around_bytes __read_mostly =

@@ -4351,7 +4273,18 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
 		 */
 		vmf->pte = NULL;
 	} else {
-		/* See comment in pte_alloc_one_map() */
+		/*
+		 * If a huge pmd materialized under us just retry later.  Use
+		 * pmd_trans_unstable() via pmd_devmap_trans_unstable() instead
+		 * of pmd_trans_huge() to ensure the pmd didn't become
+		 * pmd_trans_huge under us and then back to pmd_none, as a
+		 * result of MADV_DONTNEED running immediately after a huge pmd
+		 * fault in a different thread of this mm, in turn leading to a
+		 * misleading pmd_trans_huge() retval. All we have to ensure is
+		 * that it is a regular pmd that we can walk with
+		 * pte_offset_map() and we can do that through an atomic read
+		 * in C, which is what pmd_trans_unstable() provides.
+		 */
 		if (pmd_devmap_trans_unstable(vmf->pmd))
 			return 0;
 		/*

Signature · 1 line

2.29.2.157.g1d47791a39

Quoted 19 lines · Linus Torvalds

Quoted 7 lines · Hugh Dickins

This patch (like its antecedents) moves the pte_unmap_unlock() from after do_fault_around()'s "check if the page fault is solved" into filemap_map_pages() itself (which apparently does not NULLify vmf->pte after unmapping it, which is poor, but good for revealing this issue). That looks cleaner, but of course there was a very good reason for its original positioning.

Good catch.

Quoted 4 lines

Maybe you want to change the ->map_pages prototype, to pass down the requested address too, so that it can report whether the requested address was resolved or not. Or it could be left to __do_fault(), or even to a repeated fault; but those would be less efficient.

Let's keep the old really odd "let's unlock in the caller" for now, and minimize the changes.

Quoted 8 lines

Adding a big big comment at the end of filemap_map_pages() to note the odd delayed page table unlocking.

Here's an updated patch that combines Kirill's original patch, his additional incremental patch, and the fix for the pte lock oddity into one thing.

Does this finally pass your testing?

Quoted 2 lines

Linus

Hello together,

when i try to build this patch, i got the following error:

CC arch/x86/kernel/cpu/mce/threshold.o
mm/memory.c:3716:19: error: static declaration of ‘do_set_pmd’ follows non-static declaration
static vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page)

                   ^~~~~~~~~~
In file included from mm/memory.c:43:
./include/linux/mm.h:984:12: note: previous declaration of ‘do_set_pmd’ was here
 vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page);
            ^~~~~~~~~~

make[3]: *** [scripts/Makefile.build:279: mm/memory.o] Error 1 make[2]: *** [Makefile:1805: mm] Error 2
make[2]: *** Waiting for unfinished jobs....
CC arch/x86/kernel/cpu/mce/therm_throt.o

Yes, this one passes my testing on x86_64 and on i386.

But...

... Damian very helpfully reports that it does not build when CONFIG_TRANSPARENT_HUGEPAGE is not set, since the "static " has not been removed from the alternative definition of do_set_pmd().

Ok, your fix for that folded in, and here's yet another version.

I think Kirill was intending to move the "if (ret)" up into the path that sets it, IOW something like

(quoted diff) +5 -0

+       if (!(vma->vm_flags & VM_SHARED)) {
+               ret = check_stable_address_space(vma->vm_mm);
+               if (ret)
+                       return ret;
+       }

instead. But that patch as-is is broken.

Kirill?

Quoted 7 lines · Hugh Dickins

This patch (like its antecedents) moves the pte_unmap_unlock() from after do_fault_around()'s "check if the page fault is solved" into filemap_map_pages() itself (which apparently does not NULLify vmf->pte after unmapping it, which is poor, but good for revealing this issue). That looks cleaner, but of course there was a very good reason for its original positioning.

Good catch.

Quoted 4 lines

Maybe you want to change the ->map_pages prototype, to pass down the requested address too, so that it can report whether the requested address was resolved or not. Or it could be left to __do_fault(), or even to a repeated fault; but those would be less efficient.

Let's keep the old really odd "let's unlock in the caller" for now, and minimize the changes.

I did what Hugh proposed and it got clear to my eyes. It gets somewhat large, but take a look.

Quoted 3 lines · Kirill A. Shutemov

I did what Hugh proposed and it got clear to my eyes. It gets somewhat large, but take a look.

Ok, it's not that much bigger, and the end result is certainly much clearer wrt locking.

So that last version of yours with the fix for the uninitialized 'ret' variable looks good to me.

Of course, I've said that before, and have been wrong. So ...

Quoted 5 lines · Linus Torvalds

Quoted 3 lines · Kirill A. Shutemov

I did what Hugh proposed and it got clear to my eyes. It gets somewhat large, but take a look.

Quoted 7 lines

Ok, it's not that much bigger, and the end result is certainly much clearer wrt locking.

So that last version of yours with the fix for the uninitialized 'ret' variable looks good to me.

Of course, I've said that before, and have been wrong. So ...

And guess what... it's broken.

I folded it into testing rc1: segfault on cc1, systemd "Journal file corrupted, rotating", seen on more than one machine.

I've backed it out, rc1 itself seems fine, I'll leave rc1 under load overnight, then come back to the faultaround patch tomorrow; won't glance at it tonight, but maybe Kirill will guess what's wrong.

So far I only found one more pin leak and always-true check. I don't see how can it lead to crash or corruption. Keep looking.

I personally think it's wrong to update vmf->pte at all. We should just have a local 'ptep' pointer that we update as we walk along. But that requires another change to the calling convention, namely to "do_set_pte()".

Quoted 5 lines · Linus Torvalds

I personally think it's wrong to update vmf->pte at all. We should just have a local 'ptep' pointer that we update as we walk along. But that requires another change to the calling convention, namely to "do_set_pte()".

Actually, I think we should not use do_set_pte() at all.

About half of do_set_pte() is about the FAULT_FLAG_WRITE case, which the fault-around code never has set (and would be wrong if it did).

So I think do_set_pte() should be made local to mm/memory.c, and the filemap_map_pages() code should do it's own simplified version that just does the non-writable case, and that just gets passed the address and the pte pointer.

At that point, there would no longer be any need to update the address/pte fields in the vmf struct, and in fact I think it could be made a "const" pointer in this cal chain.

This is all just from looking at the code, I haven't tried to write a patch to do this, so I might be missing some case.

Quoted 3 lines

At that point, there would no longer be any need to update the address/pte fields in the vmf struct, and in fact I think it could be made a "const" pointer in this cal chain.

Unfortunately, we would still need to NULLify vmf->prealloc_pte once it's consumed. It kills idea with const.

I reworked do_fault_around() so it doesn't touch vmf->address and pass original address down to ->map_pages(). No need in the new argument in ->map_pages. filemap_map_pages() calculates address based on pgoff of the page handled.

Quoted 3 lines · Kirill A. Shutemov

So far I only found one more pin leak and always-true check. I don't see how can it lead to crash or corruption. Keep looking.

Well, I noticed that the nommu.c version of filemap_map_pages() needs fixing, but that's obviously not the case Hugh sees.

No,m I think the problem is the

        pte_unmap_unlock(vmf->pte, vmf->ptl);

at the end of filemap_map_pages().

Why?

Because we've been updating vmf->pte as we go along:

                vmf->pte += xas.xa_index - last_pgoff;

and I think that by the time we get to that "pte_unmap_unlock()", vmf->pte potentially points to past the edge of the page directory.

Quoted 5 lines · Kirill A. Shutemov

Quoted 4 lines · Linus Torvalds

Quoted 3 lines · Kirill A. Shutemov

So far I only found one more pin leak and always-true check. I don't see how can it lead to crash or corruption. Keep looking.

Quoted 16 lines

Quoted 16 lines

Well, I noticed that the nommu.c version of filemap_map_pages() needs fixing, but that's obviously not the case Hugh sees.

No,m I think the problem is the

        pte_unmap_unlock(vmf->pte, vmf->ptl);

at the end of filemap_map_pages().

Why?

Because we've been updating vmf->pte as we go along:

                vmf->pte += xas.xa_index - last_pgoff;

Quoted 8 lines

Quoted 2 lines

and I think that by the time we get to that "pte_unmap_unlock()", vmf->pte potentially points to past the edge of the page directory.

Well, if it's true we have bigger problem: we set up an pte entry without relevant PTL.

But I *think* we should be fine here: do_fault_around() limits start_pgoff and end_pgoff to stay within the page table.

mm/memory.c

Quoted 6 lines

It made mw looking at the code around pte_unmap_unlock() and I think that the bug is that we have to reset vmf->address and NULLify vmf->pte once we are done with faultaround:

mm/memory.c +0 -0

Ugh.. Wrong place. Need to sleep.

I'll look into your idea tomorrow.

mm/filemap.c +0 -0

index 87671284de62..e4daab80ed81 100644

But I *think* we should be fine here: do_fault_around() limits start_pgoff and end_pgoff to stay within the page table.

arch/arm64/include/asm/cpufeature.h +1 -0

index da250e4741bd..3424f5881390 100644
@@ -764,6 +764,18 @@ static inline bool cpu_has_hw_af(void)
 						ID_AA64MMFR1_HADBS_SHIFT);
 }
 
+static inline bool system_has_hw_af(void)

Quoted 8 lines · Will Deacon

arch/arm64/include/asm/cpufeature.h +0 -0

index da250e4741bd..3424f5881390 100644
@@ -764,6 +764,18 @@ static inline bool cpu_has_hw_af(void)
 						ID_AA64MMFR1_HADBS_SHIFT);
 }
 

arch/arm64/include/asm/cpufeature.h static inline bool cpu_has_hw_af(void) · line 777

Quoted 11 lines

arch/arm64/include/asm/cpufeature.h +11 -0

+static inline bool system_has_hw_af(void)
+{
+	u64 mmfr1;
+
+	if (!IS_ENABLED(CONFIG_ARM64_HW_AFDBM))
+		return false;
+
+	mmfr1 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
+	return cpuid_feature_extract_unsigned_field(mmfr1,
+						ID_AA64MMFR1_HADBS_SHIFT);
+}

Could we not add a new system-wide cpu feature that checks for hardware AF? This read_sanitised_ftr_reg() does a binary search on each invocation.