mm: fix finish_fault() handling for large folios
Subsystems (3)
PATCH 1/1 mm: fix finish_fault() handling for large folios
When handling faults for anon shmem finish_fault() will attempt to install ptes for the entire folio. Unfortunately if it encounters a single non-pte_none entry in that range it will bail, even if the pte that triggered the fault is still pte_none. When this situation happens the fault will be retried endlessly never making forward progress.
This patch fixes this behavior and if it detects that a pte in the range is not pte_none it will fall back to setting just the pte for the address that triggered the fault.
--- mm/memory.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
mm/memory.c +16 -3
@@ -5258,9 +5258,22 @@ vm_fault_t finish_fault(struct vm_fault *vmf) ret =VM_FAULT_NOPAGE ; goto unlock; }else if (nr_pages >1 && !pte_range_none (vmf->pte, nr_pages)) { -update_mmu_tlb_range (vma, addr, vmf->pte, nr_pages); - ret =VM_FAULT_NOPAGE ; - goto unlock; +/* +* We encountered a set pte, let's just try to install the +* pte for the original fault if that pte is still pte none. +*/ +pgoff_t idx = (vmf->address - addr) /PAGE_SIZE ; + +if (!pte_none (ptep_get_lockless (vmf->pte + idx))) { +update_mmu_tlb_range (vma, addr, vmf->pte, nr_pages); + ret =VM_FAULT_NOPAGE ; + goto unlock; + } + + vmf->pte = vmf->pte + idx; + page =folio_page (folio, idx); + addr = vmf->address; + nr_pages =1 ; }folio_ref_add (folio, nr_pages -1 );
Signature · 1 line
2.48.1.711.g2feabab25a-goog
Quoted 9 lines · Brian Geffon
When handling faults for anon shmem
finish_fault()will attempt to install ptes for the entire folio. Unfortunately if it encounters a single non-pte_none entry in that range it will bail, even if the pte that triggered the fault is stillpte_none. When this situation happens the fault will be retried endlessly never making forward progress.This patch fixes this behavior and if it detects that a pte in the range is not
pte_noneit will fall back to setting just the pte for the address that triggered the fault.
Surely there's a similar problem in do_anonymous_page()?
At any rate, what a horrid function finish_fault() has become. Special cases all over the place. What we should be doing is deciding the range of PTEs to insert, bounded by the folio, the VMA and any non-none entries. Maybe I'll get a chance to fix this up.
Quoted 12 lines · Matthew Wilcox
Quoted 9 lines · Brian Geffon
When handling faults for anon shmem
finish_fault()will attempt to install ptes for the entire folio. Unfortunately if it encounters a single non-pte_none entry in that range it will bail, even if the pte that triggered the fault is stillpte_none. When this situation happens the fault will be retried endlessly never making forward progress.This patch fixes this behavior and if it detects that a pte in the range is not
pte_noneit will fall back to setting just the pte for the address that triggered the fault.
Quoted 6 lines
Surely there's a similar problem in
do_anonymous_page()?At any rate, what a horrid function
finish_fault()has become. Special cases all over the place. What we should be doing is deciding the range of PTEs to insert, bounded by the folio, the VMA and any non-none entries. Maybe I'll get a chance to fix this up.
I agree, I wasn't thrilled that the fix looked like this but I was trying to keep the change minimal to aid in backporting to stable kernels where this behavior is broken. With that being said, do you have a preference on a minimal way we can fix this before finish_fault() gets a proper cleanup?
Quoted 7 lines · Matthew Wilcox
Quoted 6 lines · Brian Geffon
When handling faults for anon shmem
finish_fault()will attempt to install ptes for the entire folio. Unfortunately if it encounters a single non-pte_none entry in that range it will bail, even if the pte that triggered the fault is stillpte_none. When this situation happens the fault will be retried endlessly never making forward progress.
Quoted 5 lines
Quoted 3 lines
This patch fixes this behavior and if it detects that a pte in the range is not
pte_noneit will fall back to setting just the pte for the address that triggered the fault.Surely there's a similar problem in
do_anonymous_page()?
I recall we handle it in there correctly the last time I stared at it.
We check pte_none to decide which folio size we can allocate (including basing the decision on other factors like VMA etc), and after retaking the PTL, we recheck vmf_pte_changed / pte_range_none() to make sure there were no races.
Signature · 3 lines
Cheers, David / dhildenb
Quoted 12 lines · David Hildenbrand
Quoted 11 lines · Matthew Wilcox
Quoted 9 lines · Brian Geffon
When handling faults for anon shmem
finish_fault()will attempt to install ptes for the entire folio. Unfortunately if it encounters a single non-pte_none entry in that range it will bail, even if the pte that triggered the fault is stillpte_none. When this situation happens the fault will be retried endlessly never making forward progress.This patch fixes this behavior and if it detects that a pte in the range is not
pte_noneit will fall back to setting just the pte for the address that triggered the fault.
Quoted 8 lines
Quoted 1 line
Surely there's a similar problem in
do_anonymous_page()?I recall we handle it in there correctly the last time I stared at it.
We check
pte_noneto decide which folio size we can allocate (including basing the decision on other factors like VMA etc), and after retaking the PTL, we recheckvmf_pte_changed/pte_range_none()to make sure there were no races.
Ah, so then we'll retry and allocate a folio of the right size the next time? Rather than the shmem case where the folio is already allocated and we can't change that?
Quoted 17 lines · Matthew Wilcox
Quoted 16 lines · David Hildenbrand
Quoted 7 lines · Matthew Wilcox
Quoted 6 lines · Brian Geffon
When handling faults for anon shmem
finish_fault()will attempt to install ptes for the entire folio. Unfortunately if it encounters a single non-pte_none entry in that range it will bail, even if the pte that triggered the fault is stillpte_none. When this situation happens the fault will be retried endlessly never making forward progress.Quoted 5 lines
Quoted 3 lines
This patch fixes this behavior and if it detects that a pte in the range is not
pte_noneit will fall back to setting just the pte for the address that triggered the fault.Surely there's a similar problem in
do_anonymous_page()?I recall we handle it in there correctly the last time I stared at it.
Quoted 7 lines
Quoted 4 lines
We check
pte_noneto decide which folio size we can allocate (including basing the decision on other factors like VMA etc), and after retaking the PTL, we recheckvmf_pte_changed/pte_range_none()to make sure there were no races.Ah, so then we'll retry and allocate a folio of the right size the next time?
IIRC we'll retry the fault in case we had a race. Likely, if we had a race, somebody else installed a (large) folio and we essentially have to second fault. If, for some reason, the race only touched parts of the PTEs we tried to modify, we'll get another fault and allocate something (smaller) that would fit into the new empty range.
So yes, we're more flexible because we're allocating the folios and don't have to take whatever folio size is in the pagecache in consideration.
Signature · 3 lines
Cheers, David / dhildenb
Quoted 9 lines · Brian Geffon
When handling faults for anon shmem
finish_fault()will attempt to install ptes for the entire folio. Unfortunately if it encounters a single non-pte_none entry in that range it will bail, even if the pte that triggered the fault is stillpte_none. When this situation happens the fault will be retried endlessly never making forward progress.This patch fixes this behavior and if it detects that a pte in the range is not
pte_noneit will fall back to setting just the pte for the address that triggered the fault.
Could you describe in detail how this situation occurs? How is the none pte inserted within the range of the large folio? Because we have checks in shmem to determine if a large folio is suitable.
Anyway, if we find the pte_range_none() is false, we can fallback to per-page fault as the following code shows (untested), which seems more simple?
mm/memory.c +11 -5
@@ -5219,7 +5219,12 @@ vm_fault_t finish_fault(struct vm_fault *vmf)bool is_cow = (vmf->flags &FAULT_FLAG_WRITE ) && !(vma->vm_flags &VM_SHARED );int type, nr_pages; -unsigned long addr = vmf->address; +unsigned long addr; +bool fallback_per_page = false; + + +fallback: + addr = vmf->address;/* Did we COW the page? */ if (is_cow)
@@ -5258,7 +5263,8 @@ vm_fault_t finish_fault(struct vm_fault *vmf)* approach also applies to non-anonymous-shmem faults to avoid * inflating the RSS of the process. */ -if (!vma_is_anon_shmem (vma) ||unlikely (userfaultfd_armed (vma))) { +if (!vma_is_anon_shmem (vma) ||unlikely (userfaultfd_armed (vma)) + ||unlikely (fallback_per_page)) { nr_pages =1 ; }else if (nr_pages >1 ) {pgoff_t idx =folio_page_idx (folio, page);
@@ -5294,9 +5300,9 @@ vm_fault_t finish_fault(struct vm_fault *vmf) ret =VM_FAULT_NOPAGE ; goto unlock; }else if (nr_pages >1 && !pte_range_none (vmf->pte, nr_pages)) { -update_mmu_tlb_range (vma, addr, vmf->pte, nr_pages); - ret =VM_FAULT_NOPAGE ; - goto unlock; + fallback_per_page = true; +pte_unmap_unlock (vmf->pte, vmf->ptl); + goto fallback; }folio_ref_add (folio, nr_pages -1 );
Quoted 41 lines
Cc Baolin Wang <[email protected]>Cc Hugh Dickins <[email protected]>Fixes 43e027e41423 ("mm: memory: extend finish_fault() to support large folio")Reported-by Marek Maslanka <[email protected]>Signed-off-by Brian Geffon <[email protected]>--- mm/memory.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)mm/memory.c +16
-3@@ -5258,9 +5258,22 @@ vm_fault_t finish_fault(struct vm_fault *vmf) ret =VM_FAULT_NOPAGE ; goto unlock; }else if (nr_pages >1 && !pte_range_none (vmf->pte, nr_pages)) { -update_mmu_tlb_range (vma, addr, vmf->pte, nr_pages); - ret =VM_FAULT_NOPAGE ; - goto unlock; +/* +* We encountered a set pte, let's just try to install the +* pte for the original fault if that pte is still pte none. +*/ +pgoff_t idx = (vmf->address - addr) /PAGE_SIZE ; + +if (!pte_none (ptep_get_lockless (vmf->pte + idx))) { +update_mmu_tlb_range (vma, addr, vmf->pte, nr_pages); + ret =VM_FAULT_NOPAGE ; + goto unlock; + } + + vmf->pte = vmf->pte + idx; + page =folio_page (folio, idx); + addr = vmf->address; + nr_pages =1 ; }folio_ref_add (folio, nr_pages -1 );
Quoted 10 lines · Baolin Wang
Quoted 6 lines · Brian Geffon
When handling faults for anon shmem
finish_fault()will attempt to install ptes for the entire folio. Unfortunately if it encounters a single non-pte_none entry in that range it will bail, even if the pte that triggered the fault is stillpte_none. When this situation happens the fault will be retried endlessly never making forward progress.
Quoted 7 lines
Quoted 3 lines
This patch fixes this behavior and if it detects that a pte in the range is not
pte_noneit will fall back to setting just the pte for the address that triggered the fault.Could you describe in detail how this situation occurs? How is the none pte inserted within the range of the large folio? Because we have checks in shmem to determine if a large folio is suitable.
We're seeing it because of racing shmem_undo_range() calls, basically we have a portion of the folio is zapped and that prevents finish_fault() from ever completing because it has an assumption that the entire range must be pte_none.
Quoted 4 lines
Anyway, if we find the
pte_range_none()is false, we can fallback to per-page fault as the following code shows (untested), which seems more simple?
Yah, I like this approach. I'll send a v2 with you as a Suggested-by.
Quoted 84 lines
mm/memory.c +11
-5@@ -5219,7 +5219,12 @@ vm_fault_t finish_fault(struct vm_fault *vmf)bool is_cow = (vmf->flags &FAULT_FLAG_WRITE ) && !(vma->vm_flags &VM_SHARED );int type, nr_pages; -unsigned long addr = vmf->address; +unsigned long addr; +bool fallback_per_page = false; + + +fallback: + addr = vmf->address;/* Did we COW the page? */ if (is_cow)@@ -5258,7 +5263,8 @@ vm_fault_t finish_fault(struct vm_fault *vmf)* approach also applies to non-anonymous-shmem faults to avoid * inflating the RSS of the process. */ -if (!vma_is_anon_shmem (vma) ||unlikely (userfaultfd_armed (vma))) { +if (!vma_is_anon_shmem (vma) ||unlikely (userfaultfd_armed (vma)) + ||unlikely (fallback_per_page)) { nr_pages =1 ; }else if (nr_pages >1 ) {pgoff_t idx =folio_page_idx (folio, page);@@ -5294,9 +5300,9 @@ vm_fault_t finish_fault(struct vm_fault *vmf) ret =VM_FAULT_NOPAGE ; goto unlock; }else if (nr_pages >1 && !pte_range_none (vmf->pte, nr_pages)) { -update_mmu_tlb_range (vma, addr, vmf->pte, nr_pages); - ret =VM_FAULT_NOPAGE ; - goto unlock; + fallback_per_page = true; +pte_unmap_unlock (vmf->pte, vmf->ptl); + goto fallback; }folio_ref_add (folio, nr_pages -1 );Quoted 42 lines
Cc Baolin Wang <[email protected]>Cc Hugh Dickins <[email protected]>Fixes 43e027e41423 ("mm: memory: extend finish_fault() to support large folio")Reported-by Marek Maslanka <[email protected]>Signed-off-by Brian Geffon <[email protected]>--- mm/memory.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)mm/memory.c +16
-3@@ -5258,9 +5258,22 @@ vm_fault_t finish_fault(struct vm_fault *vmf) ret =VM_FAULT_NOPAGE ; goto unlock; }else if (nr_pages >1 && !pte_range_none (vmf->pte, nr_pages)) { -update_mmu_tlb_range (vma, addr, vmf->pte, nr_pages); - ret =VM_FAULT_NOPAGE ; - goto unlock; +/* +* We encountered a set pte, let's just try to install the +* pte for the original fault if that pte is still pte none. +*/ +pgoff_t idx = (vmf->address - addr) /PAGE_SIZE ; + +if (!pte_none (ptep_get_lockless (vmf->pte + idx))) { +update_mmu_tlb_range (vma, addr, vmf->pte, nr_pages); + ret =VM_FAULT_NOPAGE ; + goto unlock; + } + + vmf->pte = vmf->pte + idx; + page =folio_page (folio, idx); + addr = vmf->address; + nr_pages =1 ; }folio_ref_add (folio, nr_pages -1 );
PATCH v2 1/1 mm: fix finish_fault() handling for large folios
In reply to Brian GeffonWhen handling faults for anon shmem finish_fault() will attempt to install ptes for the entire folio. Unfortunately if it encounters a single non-pte_none entry in that range it will bail, even if the pte that triggered the fault is still pte_none. When this situation happens the fault will be retried endlessly never making forward progress.
This patch fixes this behavior and if it detects that a pte in the range is not pte_none it will fall back to setting a single pte.
--- mm/memory.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
mm/memory.c +10 -5
@@ -5183,7 +5183,11 @@ vm_fault_t finish_fault(struct vm_fault *vmf)bool is_cow = (vmf->flags &FAULT_FLAG_WRITE ) && !(vma->vm_flags &VM_SHARED );int type, nr_pages; -unsigned long addr = vmf->address; +unsigned long addr; +bool needs_fallback = false; + +fallback: + addr = vmf->address;/* Did we COW the page? */ if (is_cow)
@@ -5222,7 +5226,8 @@ vm_fault_t finish_fault(struct vm_fault *vmf)* approach also applies to non-anonymous-shmem faults to avoid * inflating the RSS of the process. */ -if (!vma_is_anon_shmem (vma) ||unlikely (userfaultfd_armed (vma))) { +if (!vma_is_anon_shmem (vma) ||unlikely (userfaultfd_armed (vma)) || +unlikely (needs_fallback)) { nr_pages =1 ; }else if (nr_pages >1 ) {pgoff_t idx =folio_page_idx (folio, page);
@@ -5258,9 +5263,9 @@ vm_fault_t finish_fault(struct vm_fault *vmf) ret =VM_FAULT_NOPAGE ; goto unlock; }else if (nr_pages >1 && !pte_range_none (vmf->pte, nr_pages)) { -update_mmu_tlb_range (vma, addr, vmf->pte, nr_pages); - ret =VM_FAULT_NOPAGE ; - goto unlock; + needs_fallback = true; +pte_unmap_unlock (vmf->pte, vmf->ptl); + goto fallback; }folio_ref_add (folio, nr_pages -1 );
Signature · 1 line
2.48.1.711.g2feabab25a-goog
Quoted 37 lines · Brian Geffon
When handling faults for anon shmem
finish_fault()will attempt to install ptes for the entire folio. Unfortunately if it encounters a single non-pte_none entry in that range it will bail, even if the pte that triggered the fault is stillpte_none. When this situation happens the fault will be retried endlessly never making forward progress.This patch fixes this behavior and if it detects that a pte in the range is not
pte_noneit will fall back to setting a single pte.Cc Hugh Dickins <[email protected]>Fixes 43e027e41423 ("mm: memory: extend finish_fault() to support large folio")Suggested-by Baolin Wang <[email protected]>Reported-by Marek Maslanka <[email protected]>Signed-off-by Brian Geffon <[email protected]>--- mm/memory.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)mm/memory.c +5
-1@@ -5183,7 +5183,11 @@ vm_fault_t finish_fault(struct vm_fault *vmf)bool is_cow = (vmf->flags &FAULT_FLAG_WRITE ) && !(vma->vm_flags &VM_SHARED );int type, nr_pages; -unsigned long addr = vmf->address; +unsigned long addr; +bool needs_fallback = false; + +fallback: + addr = vmf->address;/* Did we COW the page? */ if (is_cow)
mm/memory.c vm_fault_t finish_fault(struct vm_fault *vmf) · line 5230
Quoted 6 lines
mm/memory.c +2
-1@@ -5222,7 +5226,8 @@ vm_fault_t finish_fault(struct vm_fault *vmf)* approach also applies to non-anonymous-shmem faults to avoid * inflating the RSS of the process. */ -if (!vma_is_anon_shmem (vma) ||unlikely (userfaultfd_armed (vma))) { +if (!vma_is_anon_shmem (vma) ||unlikely (userfaultfd_armed (vma)) || +unlikely (needs_fallback)) {
Nit: can you align the code? Otherwise look good to me.
Quoted 16 lines
mm/memory.c +3
-3nr_pages =1 ; }else if (nr_pages >1 ) {pgoff_t idx =folio_page_idx (folio, page);@@ -5258,9 +5263,9 @@ vm_fault_t finish_fault(struct vm_fault *vmf) ret =VM_FAULT_NOPAGE ; goto unlock; }else if (nr_pages >1 && !pte_range_none (vmf->pte, nr_pages)) { -update_mmu_tlb_range (vma, addr, vmf->pte, nr_pages); - ret =VM_FAULT_NOPAGE ; - goto unlock; + needs_fallback = true; +pte_unmap_unlock (vmf->pte, vmf->ptl); + goto fallback; }folio_ref_add (folio, nr_pages -1 );
Quoted 38 lines · Baolin Wang
Quoted 34 lines · Brian Geffon
When handling faults for anon shmem
finish_fault()will attempt to install ptes for the entire folio. Unfortunately if it encounters a single non-pte_none entry in that range it will bail, even if the pte that triggered the fault is stillpte_none. When this situation happens the fault will be retried endlessly never making forward progress.This patch fixes this behavior and if it detects that a pte in the range is not
pte_noneit will fall back to setting a single pte.Cc Hugh Dickins <[email protected]>Fixes 43e027e41423 ("mm: memory: extend finish_fault() to support large folio")Suggested-by Baolin Wang <[email protected]>Reported-by Marek Maslanka <[email protected]>Signed-off-by Brian Geffon <[email protected]>--- mm/memory.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)mm/memory.c +5
-1@@ -5183,7 +5183,11 @@ vm_fault_t finish_fault(struct vm_fault *vmf)bool is_cow = (vmf->flags &FAULT_FLAG_WRITE ) && !(vma->vm_flags &VM_SHARED );int type, nr_pages; -unsigned long addr = vmf->address; +unsigned long addr; +bool needs_fallback = false; + +fallback: + addr = vmf->address;
Quoted 11 lines
mm/memory.c vm_fault_t finish_fault(struct vm_fault *vmf) · line 5230
Quoted 9 lines
mm/memory.c +2
-1/* Did we COW the page? */ if (is_cow)@@ -5222,7 +5226,8 @@ vm_fault_t finish_fault(struct vm_fault *vmf)* approach also applies to non-anonymous-shmem faults to avoid * inflating the RSS of the process. */ -if (!vma_is_anon_shmem (vma) ||unlikely (userfaultfd_armed (vma))) { +if (!vma_is_anon_shmem (vma) ||unlikely (userfaultfd_armed (vma)) || +unlikely (needs_fallback)) {Nit: can you align the code? Otherwise look good to me.
I mailed a v3 with adjusted alignment, I'll let Andrew decide which variation he prefers.
Quoted 16 lines
Quoted 16 lines
mm/memory.c +3
-3nr_pages =1 ; }else if (nr_pages >1 ) {pgoff_t idx =folio_page_idx (folio, page);@@ -5258,9 +5263,9 @@ vm_fault_t finish_fault(struct vm_fault *vmf) ret =VM_FAULT_NOPAGE ; goto unlock; }else if (nr_pages >1 && !pte_range_none (vmf->pte, nr_pages)) { -update_mmu_tlb_range (vma, addr, vmf->pte, nr_pages); - ret =VM_FAULT_NOPAGE ; - goto unlock; + needs_fallback = true; +pte_unmap_unlock (vmf->pte, vmf->ptl); + goto fallback; }
Quoted 1 line
mm/memory.c vm_fault_t finish_fault(struct vm_fault *vmf) · line 5271
Quoted 1 line
mm/memory.c +0
-0folio_ref_add (folio, nr_pages -1 );
Thanks for looking
Brian
PATCH v3 1/1 mm: fix finish_fault() handling for large folios
In reply to Brian GeffonWhen handling faults for anon shmem finish_fault() will attempt to install ptes for the entire folio. Unfortunately if it encounters a single non-pte_none entry in that range it will bail, even if the pte that triggered the fault is still pte_none. When this situation happens the fault will be retried endlessly never making forward progress.
This patch fixes this behavior and if it detects that a pte in the range is not pte_none it will fall back to setting a single pte.
--- mm/memory.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
mm/memory.c +10 -5
@@ -5183,7 +5183,11 @@ vm_fault_t finish_fault(struct vm_fault *vmf)bool is_cow = (vmf->flags &FAULT_FLAG_WRITE ) && !(vma->vm_flags &VM_SHARED );int type, nr_pages; -unsigned long addr = vmf->address; +unsigned long addr; +bool needs_fallback = false; + +fallback: + addr = vmf->address;/* Did we COW the page? */ if (is_cow)
@@ -5222,7 +5226,8 @@ vm_fault_t finish_fault(struct vm_fault *vmf)* approach also applies to non-anonymous-shmem faults to avoid * inflating the RSS of the process. */ -if (!vma_is_anon_shmem (vma) ||unlikely (userfaultfd_armed (vma))) { +if (!vma_is_anon_shmem (vma) ||unlikely (userfaultfd_armed (vma)) || +unlikely (needs_fallback)) { nr_pages =1 ; }else if (nr_pages >1 ) {pgoff_t idx =folio_page_idx (folio, page);
@@ -5258,9 +5263,9 @@ vm_fault_t finish_fault(struct vm_fault *vmf) ret =VM_FAULT_NOPAGE ; goto unlock; }else if (nr_pages >1 && !pte_range_none (vmf->pte, nr_pages)) { -update_mmu_tlb_range (vma, addr, vmf->pte, nr_pages); - ret =VM_FAULT_NOPAGE ; - goto unlock; + needs_fallback = true; +pte_unmap_unlock (vmf->pte, vmf->ptl); + goto fallback; }folio_ref_add (folio, nr_pages -1 );
Signature · 1 line
2.48.1.711.g2feabab25a-goog