[PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Zi Yan]

When handling a numa page fault, task_numa_fault() should be called by a
process that restores the page table of the faulted folio to avoid
duplicated stats counting. Commit b99a342d4f11 ("NUMA balancing: reduce
TLB flush via delaying mapping on hint page fault") restructured
do_numa_page() and do_huge_pmd_numa_page() and did not avoid
task_numa_fault() call in the second page table check after a numa
migration failure. Fix it by making all !pte_same()/!pmd_same() return
immediately.

This issue can cause task_numa_fault() being called more than necessary
and lead to unexpected numa balancing results (It is hard to tell whether
the issue will cause positive or negative performance impact due to
duplicated numa fault counting).

Reported-by: "Huang, Ying" <ying.huang@intel.com>
Closes: https://lore.kernel.org/linux-mm/87zfqfw0yw.fsf@yhuang6-desk2.ccr.corp.intel.com/
Fixes: b99a342d4f11 ("NUMA balancing: reduce TLB flush via delaying mapping on hint page fault")
Cc: <stable@vger.kernel.org>
Signed-off-by: Zi Yan <ziy@nvidia.com>
---
 mm/huge_memory.c | 5 +++--
 mm/memory.c      | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 0024266dea0a..a3c018f2b554 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1734,10 +1734,11 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 		goto out_map;
 	}
 
-out:
+count_fault:
 	if (nid != NUMA_NO_NODE)
 		task_numa_fault(last_cpupid, nid, HPAGE_PMD_NR, flags);
 
+out:
 	return 0;
 
 out_map:
@@ -1749,7 +1750,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	set_pmd_at(vma->vm_mm, haddr, vmf->pmd, pmd);
 	update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
 	spin_unlock(vmf->ptl);
-	goto out;
+	goto count_fault;
 }
 
 /*
diff --git a/mm/memory.c b/mm/memory.c
index 67496dc5064f..503d493263df 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5536,9 +5536,10 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 		goto out_map;
 	}
 
-out:
+count_fault:
 	if (nid != NUMA_NO_NODE)
 		task_numa_fault(last_cpupid, nid, nr_pages, flags);
+out:
 	return 0;
 out_map:
 	/*
@@ -5552,7 +5553,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 		numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
 					    writable);
 	pte_unmap_unlock(vmf->pte, vmf->ptl);
-	goto out;
+	goto count_fault;
 }
 
 static inline vm_fault_t create_huge_pmd(struct vm_fault *vmf)
-- 
2.43.0

[PATCH 2/2] mm/migrate: move common code to numa_migrate_check (was numa_migrate_prep)

[Zi Yan]

do_numa_page() and do_huge_pmd_numa_page() share a lot of common code. To
reduce redundancy, move common code to numa_migrate_prep() and rename
the function to numa_migrate_check() to reflect its functionality.

Now do_huge_pmd_numa_page() also checks shared folios to set TNF_SHARED
flag.

Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Zi Yan <ziy@nvidia.com>
---
 mm/huge_memory.c | 14 ++-------
 mm/internal.h    |  5 ++--
 mm/memory.c      | 76 ++++++++++++++++++++++++------------------------
 3 files changed, 44 insertions(+), 51 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a3c018f2b554..9b312cae6775 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1699,18 +1699,10 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	if (!folio)
 		goto out_map;
 
-	/* See similar comment in do_numa_page for explanation */
-	if (!writable)
-		flags |= TNF_NO_GROUP;
-
 	nid = folio_nid(folio);
-	/*
-	 * For memory tiering mode, cpupid of slow memory page is used
-	 * to record page access time.  So use default value.
-	 */
-	if (!folio_use_access_time(folio))
-		last_cpupid = folio_last_cpupid(folio);
-	target_nid = numa_migrate_prep(folio, vmf, haddr, nid, &flags);
+
+	target_nid = numa_migrate_check(folio, vmf, haddr, &flags, writable,
+					&last_cpupid);
 	if (target_nid == NUMA_NO_NODE)
 		goto out_map;
 	if (migrate_misplaced_folio_prepare(folio, vma, target_nid)) {
diff --git a/mm/internal.h b/mm/internal.h
index 52f7fc4e8ac3..fb16e18c9761 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1191,8 +1191,9 @@ void vunmap_range_noflush(unsigned long start, unsigned long end);
 
 void __vunmap_range_noflush(unsigned long start, unsigned long end);
 
-int numa_migrate_prep(struct folio *folio, struct vm_fault *vmf,
-		      unsigned long addr, int page_nid, int *flags);
+int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
+		      unsigned long addr, int *flags, bool writable,
+		      int *last_cpupid);
 
 void free_zone_device_folio(struct folio *folio);
 int migrate_device_coherent_page(struct page *page);
diff --git a/mm/memory.c b/mm/memory.c
index 503d493263df..b093df652c11 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5368,16 +5368,43 @@ static vm_fault_t do_fault(struct vm_fault *vmf)
 	return ret;
 }
 
-int numa_migrate_prep(struct folio *folio, struct vm_fault *vmf,
-		      unsigned long addr, int page_nid, int *flags)
+int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
+		      unsigned long addr, int *flags,
+		      bool writable, int *last_cpupid)
 {
 	struct vm_area_struct *vma = vmf->vma;
 
+	/*
+	 * Avoid grouping on RO pages in general. RO pages shouldn't hurt as
+	 * much anyway since they can be in shared cache state. This misses
+	 * the case where a mapping is writable but the process never writes
+	 * to it but pte_write gets cleared during protection updates and
+	 * pte_dirty has unpredictable behaviour between PTE scan updates,
+	 * background writeback, dirty balancing and application behaviour.
+	 */
+	if (!writable)
+		*flags |= TNF_NO_GROUP;
+
+	/*
+	 * Flag if the folio is shared between multiple address spaces. This
+	 * is later used when determining whether to group tasks together
+	 */
+	if (folio_likely_mapped_shared(folio) && (vma->vm_flags & VM_SHARED))
+		*flags |= TNF_SHARED;
+	/*
+	 * For memory tiering mode, cpupid of slow memory page is used
+	 * to record page access time.  So use default value.
+	 */
+	if (folio_use_access_time(folio))
+		*last_cpupid = (-1 & LAST_CPUPID_MASK);
+	else
+		*last_cpupid = folio_last_cpupid(folio);
+
 	/* Record the current PID acceesing VMA */
 	vma_set_access_pid_bit(vma);
 
 	count_vm_numa_event(NUMA_HINT_FAULTS);
-	if (page_nid == numa_node_id()) {
+	if (folio_nid(folio) == numa_node_id()) {
 		count_vm_numa_event(NUMA_HINT_FAULTS_LOCAL);
 		*flags |= TNF_FAULT_LOCAL;
 	}
@@ -5442,13 +5469,13 @@ static void numa_rebuild_large_mapping(struct vm_fault *vmf, struct vm_area_stru
 static vm_fault_t do_numa_page(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
+	pte_t old_pte = vmf->orig_pte;
+	pte_t pte;
 	struct folio *folio = NULL;
 	int nid = NUMA_NO_NODE;
 	bool writable = false, ignore_writable = false;
 	bool pte_write_upgrade = vma_wants_manual_pte_write_upgrade(vma);
-	int last_cpupid;
-	int target_nid;
-	pte_t pte, old_pte;
+	int target_nid, last_cpupid = (-1 & LAST_CPUPID_MASK);
 	int flags = 0, nr_pages;
 
 	/*
@@ -5456,10 +5483,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 	 * table lock, that its contents have not changed during fault handling.
 	 */
 	spin_lock(vmf->ptl);
-	/* Read the live PTE from the page tables: */
-	old_pte = ptep_get(vmf->pte);
-
-	if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
+	if (unlikely(!pte_same(old_pte, ptep_get(vmf->pte)))) {
 		pte_unmap_unlock(vmf->pte, vmf->ptl);
 		goto out;
 	}
@@ -5479,35 +5503,11 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 	if (!folio || folio_is_zone_device(folio))
 		goto out_map;
 
-	/*
-	 * Avoid grouping on RO pages in general. RO pages shouldn't hurt as
-	 * much anyway since they can be in shared cache state. This misses
-	 * the case where a mapping is writable but the process never writes
-	 * to it but pte_write gets cleared during protection updates and
-	 * pte_dirty has unpredictable behaviour between PTE scan updates,
-	 * background writeback, dirty balancing and application behaviour.
-	 */
-	if (!writable)
-		flags |= TNF_NO_GROUP;
-
-	/*
-	 * Flag if the folio is shared between multiple address spaces. This
-	 * is later used when determining whether to group tasks together
-	 */
-	if (folio_likely_mapped_shared(folio) && (vma->vm_flags & VM_SHARED))
-		flags |= TNF_SHARED;
-
 	nid = folio_nid(folio);
 	nr_pages = folio_nr_pages(folio);
-	/*
-	 * For memory tiering mode, cpupid of slow memory page is used
-	 * to record page access time.  So use default value.
-	 */
-	if (folio_use_access_time(folio))
-		last_cpupid = (-1 & LAST_CPUPID_MASK);
-	else
-		last_cpupid = folio_last_cpupid(folio);
-	target_nid = numa_migrate_prep(folio, vmf, vmf->address, nid, &flags);
+
+	target_nid = numa_migrate_check(folio, vmf, vmf->address, &flags,
+					writable, &last_cpupid);
 	if (target_nid == NUMA_NO_NODE)
 		goto out_map;
 	if (migrate_misplaced_folio_prepare(folio, vma, target_nid)) {
@@ -5529,7 +5529,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 					       vmf->address, &vmf->ptl);
 		if (unlikely(!vmf->pte))
 			goto out;
-		if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
+		if (unlikely(!pte_same(old_pte, ptep_get(vmf->pte)))) {
 			pte_unmap_unlock(vmf->pte, vmf->ptl);
 			goto out;
 		}
-- 
2.43.0

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Baolin Wang]

On 2024/8/8 02:47, Zi Yan wrote:
> When handling a numa page fault, task_numa_fault() should be called by a
> process that restores the page table of the faulted folio to avoid
> duplicated stats counting. Commit b99a342d4f11 ("NUMA balancing: reduce
> TLB flush via delaying mapping on hint page fault") restructured
> do_numa_page() and do_huge_pmd_numa_page() and did not avoid
> task_numa_fault() call in the second page table check after a numa
> migration failure. Fix it by making all !pte_same()/!pmd_same() return
> immediately.
> 
> This issue can cause task_numa_fault() being called more than necessary
> and lead to unexpected numa balancing results (It is hard to tell whether
> the issue will cause positive or negative performance impact due to
> duplicated numa fault counting).
> 
> Reported-by: "Huang, Ying" <ying.huang@intel.com>
> Closes: https://lore.kernel.org/linux-mm/87zfqfw0yw.fsf@yhuang6-desk2.ccr.corp.intel.com/
> Fixes: b99a342d4f11 ("NUMA balancing: reduce TLB flush via delaying mapping on hint page fault")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Zi Yan <ziy@nvidia.com>

The fix looks reasonable to me. Feel free to add:
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

(Nit: These goto labels are a bit confusing and might need some cleanup 
in the future.)

> ---
>   mm/huge_memory.c | 5 +++--
>   mm/memory.c      | 5 +++--
>   2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 0024266dea0a..a3c018f2b554 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1734,10 +1734,11 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
>   		goto out_map;
>   	}
>   
> -out:
> +count_fault:
>   	if (nid != NUMA_NO_NODE)
>   		task_numa_fault(last_cpupid, nid, HPAGE_PMD_NR, flags);
>   
> +out:
>   	return 0;
>   
>   out_map:
> @@ -1749,7 +1750,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
>   	set_pmd_at(vma->vm_mm, haddr, vmf->pmd, pmd);
>   	update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
>   	spin_unlock(vmf->ptl);
> -	goto out;
> +	goto count_fault;
>   }
>   
>   /*
> diff --git a/mm/memory.c b/mm/memory.c
> index 67496dc5064f..503d493263df 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5536,9 +5536,10 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>   		goto out_map;
>   	}
>   
> -out:
> +count_fault:
>   	if (nid != NUMA_NO_NODE)
>   		task_numa_fault(last_cpupid, nid, nr_pages, flags);
> +out:
>   	return 0;
>   out_map:
>   	/*
> @@ -5552,7 +5553,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>   		numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
>   					    writable);
>   	pte_unmap_unlock(vmf->pte, vmf->ptl);
> -	goto out;
> +	goto count_fault;
>   }
>   
>   static inline vm_fault_t create_huge_pmd(struct vm_fault *vmf)

Re: [PATCH 2/2] mm/migrate: move common code to numa_migrate_check (was numa_migrate_prep)

[Huang, Ying]

Zi Yan <ziy@nvidia.com> writes:

> do_numa_page() and do_huge_pmd_numa_page() share a lot of common code. To
> reduce redundancy, move common code to numa_migrate_prep() and rename
> the function to numa_migrate_check() to reflect its functionality.
>
> Now do_huge_pmd_numa_page() also checks shared folios to set TNF_SHARED
> flag.
>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
>  mm/huge_memory.c | 14 ++-------
>  mm/internal.h    |  5 ++--
>  mm/memory.c      | 76 ++++++++++++++++++++++++------------------------
>  3 files changed, 44 insertions(+), 51 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index a3c018f2b554..9b312cae6775 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1699,18 +1699,10 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
>  	if (!folio)
>  		goto out_map;
>  
> -	/* See similar comment in do_numa_page for explanation */
> -	if (!writable)
> -		flags |= TNF_NO_GROUP;
> -
>  	nid = folio_nid(folio);

NITPICK: It appears that we can remove "nid" local variable.

> -	/*
> -	 * For memory tiering mode, cpupid of slow memory page is used
> -	 * to record page access time.  So use default value.
> -	 */
> -	if (!folio_use_access_time(folio))
> -		last_cpupid = folio_last_cpupid(folio);
> -	target_nid = numa_migrate_prep(folio, vmf, haddr, nid, &flags);
> +
> +	target_nid = numa_migrate_check(folio, vmf, haddr, &flags, writable,
> +					&last_cpupid);
>  	if (target_nid == NUMA_NO_NODE)
>  		goto out_map;
>  	if (migrate_misplaced_folio_prepare(folio, vma, target_nid)) {
> diff --git a/mm/internal.h b/mm/internal.h
> index 52f7fc4e8ac3..fb16e18c9761 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1191,8 +1191,9 @@ void vunmap_range_noflush(unsigned long start, unsigned long end);
>  
>  void __vunmap_range_noflush(unsigned long start, unsigned long end);
>  
> -int numa_migrate_prep(struct folio *folio, struct vm_fault *vmf,
> -		      unsigned long addr, int page_nid, int *flags);
> +int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
> +		      unsigned long addr, int *flags, bool writable,
> +		      int *last_cpupid);
>  
>  void free_zone_device_folio(struct folio *folio);
>  int migrate_device_coherent_page(struct page *page);
> diff --git a/mm/memory.c b/mm/memory.c
> index 503d493263df..b093df652c11 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5368,16 +5368,43 @@ static vm_fault_t do_fault(struct vm_fault *vmf)
>  	return ret;
>  }
>  
> -int numa_migrate_prep(struct folio *folio, struct vm_fault *vmf,
> -		      unsigned long addr, int page_nid, int *flags)
> +int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
> +		      unsigned long addr, int *flags,
> +		      bool writable, int *last_cpupid)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
>  
> +	/*
> +	 * Avoid grouping on RO pages in general. RO pages shouldn't hurt as
> +	 * much anyway since they can be in shared cache state. This misses
> +	 * the case where a mapping is writable but the process never writes
> +	 * to it but pte_write gets cleared during protection updates and
> +	 * pte_dirty has unpredictable behaviour between PTE scan updates,
> +	 * background writeback, dirty balancing and application behaviour.
> +	 */
> +	if (!writable)
> +		*flags |= TNF_NO_GROUP;
> +
> +	/*
> +	 * Flag if the folio is shared between multiple address spaces. This
> +	 * is later used when determining whether to group tasks together
> +	 */
> +	if (folio_likely_mapped_shared(folio) && (vma->vm_flags & VM_SHARED))
> +		*flags |= TNF_SHARED;
> +	/*
> +	 * For memory tiering mode, cpupid of slow memory page is used
> +	 * to record page access time.  So use default value.
> +	 */
> +	if (folio_use_access_time(folio))
> +		*last_cpupid = (-1 & LAST_CPUPID_MASK);
> +	else
> +		*last_cpupid = folio_last_cpupid(folio);
> +
>  	/* Record the current PID acceesing VMA */
>  	vma_set_access_pid_bit(vma);
>  
>  	count_vm_numa_event(NUMA_HINT_FAULTS);
> -	if (page_nid == numa_node_id()) {
> +	if (folio_nid(folio) == numa_node_id()) {
>  		count_vm_numa_event(NUMA_HINT_FAULTS_LOCAL);
>  		*flags |= TNF_FAULT_LOCAL;
>  	}
> @@ -5442,13 +5469,13 @@ static void numa_rebuild_large_mapping(struct vm_fault *vmf, struct vm_area_stru
>  static vm_fault_t do_numa_page(struct vm_fault *vmf)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
> +	pte_t old_pte = vmf->orig_pte;

The usage of old_pte is different from other use cases.  Where,

old_pte = *pte;
check old_pte and orig_pte
generate new_pte from old_pte
set new_pte

We have used this before in do_numa_page(), but not do that now.  But I
still think that it's better to follow the convention partly if
possible.  This makes code easier to be read.  I notices that we don't
follow it in do_huge_pmd_numa_page(), we may change that?

> +	pte_t pte;
>  	struct folio *folio = NULL;
>  	int nid = NUMA_NO_NODE;
>  	bool writable = false, ignore_writable = false;
>  	bool pte_write_upgrade = vma_wants_manual_pte_write_upgrade(vma);
> -	int last_cpupid;
> -	int target_nid;
> -	pte_t pte, old_pte;
> +	int target_nid, last_cpupid = (-1 & LAST_CPUPID_MASK);

Because we will initialize last_cpupid in numa_migrate_check(), we don't
need to initialize it here?

>  	int flags = 0, nr_pages;
>  
>  	/*
> @@ -5456,10 +5483,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>  	 * table lock, that its contents have not changed during fault handling.
>  	 */
>  	spin_lock(vmf->ptl);
> -	/* Read the live PTE from the page tables: */
> -	old_pte = ptep_get(vmf->pte);
> -
> -	if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
> +	if (unlikely(!pte_same(old_pte, ptep_get(vmf->pte)))) {
>  		pte_unmap_unlock(vmf->pte, vmf->ptl);
>  		goto out;
>  	}

[snip]

--
Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[David Hildenbrand]

On 08.08.24 05:19, Baolin Wang wrote:
> 
> 
> On 2024/8/8 02:47, Zi Yan wrote:
>> When handling a numa page fault, task_numa_fault() should be called by a
>> process that restores the page table of the faulted folio to avoid
>> duplicated stats counting. Commit b99a342d4f11 ("NUMA balancing: reduce
>> TLB flush via delaying mapping on hint page fault") restructured
>> do_numa_page() and do_huge_pmd_numa_page() and did not avoid
>> task_numa_fault() call in the second page table check after a numa
>> migration failure. Fix it by making all !pte_same()/!pmd_same() return
>> immediately.
>>
>> This issue can cause task_numa_fault() being called more than necessary
>> and lead to unexpected numa balancing results (It is hard to tell whether
>> the issue will cause positive or negative performance impact due to
>> duplicated numa fault counting).
>>
>> Reported-by: "Huang, Ying" <ying.huang@intel.com>
>> Closes: https://lore.kernel.org/linux-mm/87zfqfw0yw.fsf@yhuang6-desk2.ccr.corp.intel.com/
>> Fixes: b99a342d4f11 ("NUMA balancing: reduce TLB flush via delaying mapping on hint page fault")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
> 
> The fix looks reasonable to me. Feel free to add:
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> 
> (Nit: These goto labels are a bit confusing and might need some cleanup
> in the future.)

Agreed, maybe we should simply handle that right away and replace the "goto out;" users by "return 0;".

Then, just copy the 3 LOC.

For mm/memory.c that would be:

diff --git a/mm/memory.c b/mm/memory.c
index 67496dc5064f..410ba50ca746 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
  
         if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
                 pte_unmap_unlock(vmf->pte, vmf->ptl);
-               goto out;
+               return 0;
         }
  
         pte = pte_modify(old_pte, vma->vm_page_prot);
@@ -5528,15 +5528,14 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
                 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
                                                vmf->address, &vmf->ptl);
                 if (unlikely(!vmf->pte))
-                       goto out;
+                       return 0;
                 if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
                         pte_unmap_unlock(vmf->pte, vmf->ptl);
-                       goto out;
+                       return 0;
                 }
                 goto out_map;
         }
  
-out:
         if (nid != NUMA_NO_NODE)
                 task_numa_fault(last_cpupid, nid, nr_pages, flags);
         return 0;
@@ -5552,7 +5551,9 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
                 numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
                                             writable);
         pte_unmap_unlock(vmf->pte, vmf->ptl);
-       goto out;
+       if (nid != NUMA_NO_NODE)
+               task_numa_fault(last_cpupid, nid, nr_pages, flags);
+       return 0;
  }


-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Baolin Wang]

On 2024/8/8 16:22, David Hildenbrand wrote:
> On 08.08.24 05:19, Baolin Wang wrote:
>>
>>
>> On 2024/8/8 02:47, Zi Yan wrote:
>>> When handling a numa page fault, task_numa_fault() should be called by a
>>> process that restores the page table of the faulted folio to avoid
>>> duplicated stats counting. Commit b99a342d4f11 ("NUMA balancing: reduce
>>> TLB flush via delaying mapping on hint page fault") restructured
>>> do_numa_page() and do_huge_pmd_numa_page() and did not avoid
>>> task_numa_fault() call in the second page table check after a numa
>>> migration failure. Fix it by making all !pte_same()/!pmd_same() return
>>> immediately.
>>>
>>> This issue can cause task_numa_fault() being called more than necessary
>>> and lead to unexpected numa balancing results (It is hard to tell 
>>> whether
>>> the issue will cause positive or negative performance impact due to
>>> duplicated numa fault counting).
>>>
>>> Reported-by: "Huang, Ying" <ying.huang@intel.com>
>>> Closes: 
>>> https://lore.kernel.org/linux-mm/87zfqfw0yw.fsf@yhuang6-desk2.ccr.corp.intel.com/
>>> Fixes: b99a342d4f11 ("NUMA balancing: reduce TLB flush via delaying 
>>> mapping on hint page fault")
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>
>> The fix looks reasonable to me. Feel free to add:
>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>
>> (Nit: These goto labels are a bit confusing and might need some cleanup
>> in the future.)
> 
> Agreed, maybe we should simply handle that right away and replace the 
> "goto out;" users by "return 0;".
> 
> Then, just copy the 3 LOC.
> 
> For mm/memory.c that would be:
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 67496dc5064f..410ba50ca746 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
> 
>          if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
>                  pte_unmap_unlock(vmf->pte, vmf->ptl);
> -               goto out;
> +               return 0;
>          }
> 
>          pte = pte_modify(old_pte, vma->vm_page_prot);
> @@ -5528,15 +5528,14 @@ static vm_fault_t do_numa_page(struct vm_fault 
> *vmf)
>                  vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>                                                 vmf->address, &vmf->ptl);
>                  if (unlikely(!vmf->pte))
> -                       goto out;
> +                       return 0;
>                  if (unlikely(!pte_same(ptep_get(vmf->pte), 
> vmf->orig_pte))) {
>                          pte_unmap_unlock(vmf->pte, vmf->ptl);
> -                       goto out;
> +                       return 0;
>                  }
>                  goto out_map;
>          }
> 
> -out:
>          if (nid != NUMA_NO_NODE)
>                  task_numa_fault(last_cpupid, nid, nr_pages, flags);
>          return 0;
> @@ -5552,7 +5551,9 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>                  numa_rebuild_single_mapping(vmf, vma, vmf->address, 
> vmf->pte,
>                                              writable);
>          pte_unmap_unlock(vmf->pte, vmf->ptl);
> -       goto out;
> +       if (nid != NUMA_NO_NODE)
> +               task_numa_fault(last_cpupid, nid, nr_pages, flags);
> +       return 0;
>   }

Thanks. Looks better:)

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 3493 bytes --]

On 8 Aug 2024, at 4:22, David Hildenbrand wrote:

> On 08.08.24 05:19, Baolin Wang wrote:
>>
>>
>> On 2024/8/8 02:47, Zi Yan wrote:
>>> When handling a numa page fault, task_numa_fault() should be called by a
>>> process that restores the page table of the faulted folio to avoid
>>> duplicated stats counting. Commit b99a342d4f11 ("NUMA balancing: reduce
>>> TLB flush via delaying mapping on hint page fault") restructured
>>> do_numa_page() and do_huge_pmd_numa_page() and did not avoid
>>> task_numa_fault() call in the second page table check after a numa
>>> migration failure. Fix it by making all !pte_same()/!pmd_same() return
>>> immediately.
>>>
>>> This issue can cause task_numa_fault() being called more than necessary
>>> and lead to unexpected numa balancing results (It is hard to tell whether
>>> the issue will cause positive or negative performance impact due to
>>> duplicated numa fault counting).
>>>
>>> Reported-by: "Huang, Ying" <ying.huang@intel.com>
>>> Closes: https://lore.kernel.org/linux-mm/87zfqfw0yw.fsf@yhuang6-desk2.ccr.corp.intel.com/
>>> Fixes: b99a342d4f11 ("NUMA balancing: reduce TLB flush via delaying mapping on hint page fault")
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>
>> The fix looks reasonable to me. Feel free to add:
>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>
>> (Nit: These goto labels are a bit confusing and might need some cleanup
>> in the future.)
>
> Agreed, maybe we should simply handle that right away and replace the "goto out;" users by "return 0;".
>
> Then, just copy the 3 LOC.
>
> For mm/memory.c that would be:
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 67496dc5064f..410ba50ca746 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>          if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
>                 pte_unmap_unlock(vmf->pte, vmf->ptl);
> -               goto out;
> +               return 0;
>         }
>          pte = pte_modify(old_pte, vma->vm_page_prot);
> @@ -5528,15 +5528,14 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>                 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>                                                vmf->address, &vmf->ptl);
>                 if (unlikely(!vmf->pte))
> -                       goto out;
> +                       return 0;
>                 if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
>                         pte_unmap_unlock(vmf->pte, vmf->ptl);
> -                       goto out;
> +                       return 0;
>                 }
>                 goto out_map;
>         }
>  -out:
>         if (nid != NUMA_NO_NODE)
>                 task_numa_fault(last_cpupid, nid, nr_pages, flags);
>         return 0;
> @@ -5552,7 +5551,9 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>                 numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
>                                             writable);
>         pte_unmap_unlock(vmf->pte, vmf->ptl);
> -       goto out;
> +       if (nid != NUMA_NO_NODE)
> +               task_numa_fault(last_cpupid, nid, nr_pages, flags);
> +       return 0;
>  }

Looks good to me. Thanks.

Hi Andrew,

Should I resend this for an easy back porting? Or you want to fold David’s
changes in directly?

Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[David Hildenbrand]

On 08.08.24 16:13, Zi Yan wrote:
> On 8 Aug 2024, at 4:22, David Hildenbrand wrote:
> 
>> On 08.08.24 05:19, Baolin Wang wrote:
>>>
>>>
>>> On 2024/8/8 02:47, Zi Yan wrote:
>>>> When handling a numa page fault, task_numa_fault() should be called by a
>>>> process that restores the page table of the faulted folio to avoid
>>>> duplicated stats counting. Commit b99a342d4f11 ("NUMA balancing: reduce
>>>> TLB flush via delaying mapping on hint page fault") restructured
>>>> do_numa_page() and do_huge_pmd_numa_page() and did not avoid
>>>> task_numa_fault() call in the second page table check after a numa
>>>> migration failure. Fix it by making all !pte_same()/!pmd_same() return
>>>> immediately.
>>>>
>>>> This issue can cause task_numa_fault() being called more than necessary
>>>> and lead to unexpected numa balancing results (It is hard to tell whether
>>>> the issue will cause positive or negative performance impact due to
>>>> duplicated numa fault counting).
>>>>
>>>> Reported-by: "Huang, Ying" <ying.huang@intel.com>
>>>> Closes: https://lore.kernel.org/linux-mm/87zfqfw0yw.fsf@yhuang6-desk2.ccr.corp.intel.com/
>>>> Fixes: b99a342d4f11 ("NUMA balancing: reduce TLB flush via delaying mapping on hint page fault")
>>>> Cc: <stable@vger.kernel.org>
>>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>>
>>> The fix looks reasonable to me. Feel free to add:
>>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>>
>>> (Nit: These goto labels are a bit confusing and might need some cleanup
>>> in the future.)
>>
>> Agreed, maybe we should simply handle that right away and replace the "goto out;" users by "return 0;".
>>
>> Then, just copy the 3 LOC.
>>
>> For mm/memory.c that would be:
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 67496dc5064f..410ba50ca746 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>           if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
>>                  pte_unmap_unlock(vmf->pte, vmf->ptl);
>> -               goto out;
>> +               return 0;
>>          }
>>           pte = pte_modify(old_pte, vma->vm_page_prot);
>> @@ -5528,15 +5528,14 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>                  vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>>                                                 vmf->address, &vmf->ptl);
>>                  if (unlikely(!vmf->pte))
>> -                       goto out;
>> +                       return 0;
>>                  if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
>>                          pte_unmap_unlock(vmf->pte, vmf->ptl);
>> -                       goto out;
>> +                       return 0;
>>                  }
>>                  goto out_map;
>>          }
>>   -out:
>>          if (nid != NUMA_NO_NODE)
>>                  task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>          return 0;
>> @@ -5552,7 +5551,9 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>                  numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
>>                                              writable);
>>          pte_unmap_unlock(vmf->pte, vmf->ptl);
>> -       goto out;
>> +       if (nid != NUMA_NO_NODE)
>> +               task_numa_fault(last_cpupid, nid, nr_pages, flags);
>> +       return 0;
>>   }
> 
> Looks good to me. Thanks.
> 
> Hi Andrew,
> 
> Should I resend this for an easy back porting? Or you want to fold David’s
> changes in directly?

Note that I didn't touch huge_memory.c. So maybe just send a fixup on top?

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Zi Yan]

[-- Attachment #1.1: Type: text/plain, Size: 3886 bytes --]

On 8 Aug 2024, at 10:14, David Hildenbrand wrote:

> On 08.08.24 16:13, Zi Yan wrote:
>> On 8 Aug 2024, at 4:22, David Hildenbrand wrote:
>>
>>> On 08.08.24 05:19, Baolin Wang wrote:
>>>>
>>>>
>>>> On 2024/8/8 02:47, Zi Yan wrote:
>>>>> When handling a numa page fault, task_numa_fault() should be called by a
>>>>> process that restores the page table of the faulted folio to avoid
>>>>> duplicated stats counting. Commit b99a342d4f11 ("NUMA balancing: reduce
>>>>> TLB flush via delaying mapping on hint page fault") restructured
>>>>> do_numa_page() and do_huge_pmd_numa_page() and did not avoid
>>>>> task_numa_fault() call in the second page table check after a numa
>>>>> migration failure. Fix it by making all !pte_same()/!pmd_same() return
>>>>> immediately.
>>>>>
>>>>> This issue can cause task_numa_fault() being called more than necessary
>>>>> and lead to unexpected numa balancing results (It is hard to tell whether
>>>>> the issue will cause positive or negative performance impact due to
>>>>> duplicated numa fault counting).
>>>>>
>>>>> Reported-by: "Huang, Ying" <ying.huang@intel.com>
>>>>> Closes: https://lore.kernel.org/linux-mm/87zfqfw0yw.fsf@yhuang6-desk2.ccr.corp.intel.com/
>>>>> Fixes: b99a342d4f11 ("NUMA balancing: reduce TLB flush via delaying mapping on hint page fault")
>>>>> Cc: <stable@vger.kernel.org>
>>>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>>>
>>>> The fix looks reasonable to me. Feel free to add:
>>>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>>>
>>>> (Nit: These goto labels are a bit confusing and might need some cleanup
>>>> in the future.)
>>>
>>> Agreed, maybe we should simply handle that right away and replace the "goto out;" users by "return 0;".
>>>
>>> Then, just copy the 3 LOC.
>>>
>>> For mm/memory.c that would be:
>>>
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index 67496dc5064f..410ba50ca746 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>           if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
>>>                  pte_unmap_unlock(vmf->pte, vmf->ptl);
>>> -               goto out;
>>> +               return 0;
>>>          }
>>>           pte = pte_modify(old_pte, vma->vm_page_prot);
>>> @@ -5528,15 +5528,14 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>                  vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>>>                                                 vmf->address, &vmf->ptl);
>>>                  if (unlikely(!vmf->pte))
>>> -                       goto out;
>>> +                       return 0;
>>>                  if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
>>>                          pte_unmap_unlock(vmf->pte, vmf->ptl);
>>> -                       goto out;
>>> +                       return 0;
>>>                  }
>>>                  goto out_map;
>>>          }
>>>   -out:
>>>          if (nid != NUMA_NO_NODE)
>>>                  task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>>          return 0;
>>> @@ -5552,7 +5551,9 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>                  numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
>>>                                              writable);
>>>          pte_unmap_unlock(vmf->pte, vmf->ptl);
>>> -       goto out;
>>> +       if (nid != NUMA_NO_NODE)
>>> +               task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>> +       return 0;
>>>   }
>>
>> Looks good to me. Thanks.
>>
>> Hi Andrew,
>>
>> Should I resend this for an easy back porting? Or you want to fold David’s
>> changes in directly?
>
> Note that I didn't touch huge_memory.c. So maybe just send a fixup on top?

Got it. The fixup is attached.

Best Regards,
Yan, Zi

[-- Attachment #1.2: 0001-fixup-mm-numa-no-task_numa_fault-call-if-page-table-.patch --]
[-- Type: text/plain, Size: 2864 bytes --]

From c0494d569e77291f7f51abb16c2ceff0976371f4 Mon Sep 17 00:00:00 2001
From: Zi Yan <ziy@nvidia.com>
Date: Thu, 8 Aug 2024 10:18:42 -0400
Subject: [PATCH] fixup! mm/numa: no task_numa_fault() call if page table is
 changed

---
 mm/huge_memory.c | 11 +++++------
 mm/memory.c      | 12 ++++++------
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a3c018f2b554..4e8746769a97 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1681,7 +1681,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
 	if (unlikely(!pmd_same(oldpmd, *vmf->pmd))) {
 		spin_unlock(vmf->ptl);
-		goto out;
+		return 0;
 	}
 
 	pmd = pmd_modify(oldpmd, vma->vm_page_prot);
@@ -1729,16 +1729,13 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 		vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
 		if (unlikely(!pmd_same(oldpmd, *vmf->pmd))) {
 			spin_unlock(vmf->ptl);
-			goto out;
+			return 0;
 		}
 		goto out_map;
 	}
 
-count_fault:
 	if (nid != NUMA_NO_NODE)
 		task_numa_fault(last_cpupid, nid, HPAGE_PMD_NR, flags);
-
-out:
 	return 0;
 
 out_map:
@@ -1750,7 +1747,9 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	set_pmd_at(vma->vm_mm, haddr, vmf->pmd, pmd);
 	update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
 	spin_unlock(vmf->ptl);
-	goto count_fault;
+	if (nid != NUMA_NO_NODE)
+		task_numa_fault(last_cpupid, nid, HPAGE_PMD_NR, flags);
+	return 0;
 }
 
 /*
diff --git a/mm/memory.c b/mm/memory.c
index 503d493263df..410ba50ca746 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 
 	if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
 		pte_unmap_unlock(vmf->pte, vmf->ptl);
-		goto out;
+		return 0;
 	}
 
 	pte = pte_modify(old_pte, vma->vm_page_prot);
@@ -5528,18 +5528,16 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 		vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
 					       vmf->address, &vmf->ptl);
 		if (unlikely(!vmf->pte))
-			goto out;
+			return 0;
 		if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
 			pte_unmap_unlock(vmf->pte, vmf->ptl);
-			goto out;
+			return 0;
 		}
 		goto out_map;
 	}
 
-count_fault:
 	if (nid != NUMA_NO_NODE)
 		task_numa_fault(last_cpupid, nid, nr_pages, flags);
-out:
 	return 0;
 out_map:
 	/*
@@ -5553,7 +5551,9 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 		numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
 					    writable);
 	pte_unmap_unlock(vmf->pte, vmf->ptl);
-	goto count_fault;
+	if (nid != NUMA_NO_NODE)
+		task_numa_fault(last_cpupid, nid, nr_pages, flags);
+	return 0;
 }
 
 static inline vm_fault_t create_huge_pmd(struct vm_fault *vmf)
-- 
2.43.0


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Kefeng Wang]

On 2024/8/8 22:21, Zi Yan wrote:
> On 8 Aug 2024, at 10:14, David Hildenbrand wrote:
> 
>> On 08.08.24 16:13, Zi Yan wrote:
>>> On 8 Aug 2024, at 4:22, David Hildenbrand wrote:
>>>
>>>> On 08.08.24 05:19, Baolin Wang wrote:
>>>>>
>>>>>
...
>>>> Agreed, maybe we should simply handle that right away and replace the "goto out;" users by "return 0;".
>>>>
>>>> Then, just copy the 3 LOC.
>>>>
>>>> For mm/memory.c that would be:
>>>>
>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>> index 67496dc5064f..410ba50ca746 100644
>>>> --- a/mm/memory.c
>>>> +++ b/mm/memory.c
>>>> @@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>            if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
>>>>                   pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>> -               goto out;
>>>> +               return 0;
>>>>           }
>>>>            pte = pte_modify(old_pte, vma->vm_page_prot);
>>>> @@ -5528,15 +5528,14 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>                   vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>>>>                                                  vmf->address, &vmf->ptl);
>>>>                   if (unlikely(!vmf->pte))
>>>> -                       goto out;
>>>> +                       return 0;
>>>>                   if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
>>>>                           pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>> -                       goto out;
>>>> +                       return 0;
>>>>                   }
>>>>                   goto out_map;
>>>>           }
>>>>    -out:
>>>>           if (nid != NUMA_NO_NODE)
>>>>                   task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>>>           return 0;

Maybe drop this part too,

diff --git a/mm/memory.c b/mm/memory.c
index 410ba50ca746..07343c1469e0 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5523,6 +5523,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
         if (!migrate_misplaced_folio(folio, vma, target_nid)) {
                 nid = target_nid;
                 flags |= TNF_MIGRATED;
+               goto out;
         } else {
                 flags |= TNF_MIGRATE_FAIL;
                 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
@@ -5533,12 +5534,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
                         pte_unmap_unlock(vmf->pte, vmf->ptl);
                         return 0;
                 }
-               goto out_map;
         }

-       if (nid != NUMA_NO_NODE)
-               task_numa_fault(last_cpupid, nid, nr_pages, flags);
-       return 0;
  out_map:
         /*
          * Make it present again, depending on how arch implements
@@ -5551,6 +5548,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
                 numa_rebuild_single_mapping(vmf, vma, vmf->address, 
vmf->pte,
                                             writable);
         pte_unmap_unlock(vmf->pte, vmf->ptl);
+out:
         if (nid != NUMA_NO_NODE)
                 task_numa_fault(last_cpupid, nid, nr_pages, flags);
         return 0;


>>>> @@ -5552,7 +5551,9 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>                   numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
>>>>                                               writable);
>>>>           pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>> -       goto out;
>>>> +       if (nid != NUMA_NO_NODE)
>>>> +               task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>>> +       return 0;
>>>>    }
>>>
>>> Looks good to me. Thanks.
>>>
>>> Hi Andrew,
>>>
>>> Should I resend this for an easy back porting? Or you want to fold David’s
>>> changes in directly?
>>
>> Note that I didn't touch huge_memory.c. So maybe just send a fixup on top?
> 
> Got it. The fixup is attached.
> 
> Best Regards,
> Yan, Zi

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Zi Yan]

[-- Attachment #1.1: Type: text/plain, Size: 3398 bytes --]

On 8 Aug 2024, at 10:36, Kefeng Wang wrote:

> On 2024/8/8 22:21, Zi Yan wrote:
>> On 8 Aug 2024, at 10:14, David Hildenbrand wrote:
>>
>>> On 08.08.24 16:13, Zi Yan wrote:
>>>> On 8 Aug 2024, at 4:22, David Hildenbrand wrote:
>>>>
>>>>> On 08.08.24 05:19, Baolin Wang wrote:
>>>>>>
>>>>>>
> ...
>>>>> Agreed, maybe we should simply handle that right away and replace the "goto out;" users by "return 0;".
>>>>>
>>>>> Then, just copy the 3 LOC.
>>>>>
>>>>> For mm/memory.c that would be:
>>>>>
>>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>>> index 67496dc5064f..410ba50ca746 100644
>>>>> --- a/mm/memory.c
>>>>> +++ b/mm/memory.c
>>>>> @@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>            if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
>>>>>                   pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>> -               goto out;
>>>>> +               return 0;
>>>>>           }
>>>>>            pte = pte_modify(old_pte, vma->vm_page_prot);
>>>>> @@ -5528,15 +5528,14 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>                   vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>>>>>                                                  vmf->address, &vmf->ptl);
>>>>>                   if (unlikely(!vmf->pte))
>>>>> -                       goto out;
>>>>> +                       return 0;
>>>>>                   if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
>>>>>                           pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>> -                       goto out;
>>>>> +                       return 0;
>>>>>                   }
>>>>>                   goto out_map;
>>>>>           }
>>>>>    -out:
>>>>>           if (nid != NUMA_NO_NODE)
>>>>>                   task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>>>>           return 0;
>
> Maybe drop this part too,
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 410ba50ca746..07343c1469e0 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5523,6 +5523,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>         if (!migrate_misplaced_folio(folio, vma, target_nid)) {
>                 nid = target_nid;
>                 flags |= TNF_MIGRATED;
> +               goto out;
>         } else {
>                 flags |= TNF_MIGRATE_FAIL;
>                 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
> @@ -5533,12 +5534,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>                         pte_unmap_unlock(vmf->pte, vmf->ptl);
>                         return 0;
>                 }
> -               goto out_map;
>         }
>
> -       if (nid != NUMA_NO_NODE)
> -               task_numa_fault(last_cpupid, nid, nr_pages, flags);
> -       return 0;
>  out_map:
>         /*
>          * Make it present again, depending on how arch implements
> @@ -5551,6 +5548,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>                 numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
>                                             writable);
>         pte_unmap_unlock(vmf->pte, vmf->ptl);
> +out:
>         if (nid != NUMA_NO_NODE)
>                 task_numa_fault(last_cpupid, nid, nr_pages, flags);
>         return 0;

Even better. Thanks. The updated fixup is attached.



Best Regards,
Yan, Zi

[-- Attachment #1.2: 0001-fixup-mm-numa-no-task_numa_fault-call-if-page-table-.patch --]
[-- Type: text/plain, Size: 3359 bytes --]

From d761a4bb6cdf3277f1af1d129bf01583962c53b3 Mon Sep 17 00:00:00 2001
From: Zi Yan <ziy@nvidia.com>
Date: Thu, 8 Aug 2024 10:18:42 -0400
Subject: [PATCH] fixup! mm/numa: no task_numa_fault() call if page table is
 changed

---
 mm/huge_memory.c | 18 +++++++-----------
 mm/memory.c      | 19 ++++++++-----------
 2 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a3c018f2b554..1e22801a9d6e 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1681,7 +1681,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
 	if (unlikely(!pmd_same(oldpmd, *vmf->pmd))) {
 		spin_unlock(vmf->ptl);
-		goto out;
+		return 0;
 	}
 
 	pmd = pmd_modify(oldpmd, vma->vm_page_prot);
@@ -1724,23 +1724,16 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	if (!migrate_misplaced_folio(folio, vma, target_nid)) {
 		flags |= TNF_MIGRATED;
 		nid = target_nid;
+		goto out:
 	} else {
 		flags |= TNF_MIGRATE_FAIL;
 		vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
 		if (unlikely(!pmd_same(oldpmd, *vmf->pmd))) {
 			spin_unlock(vmf->ptl);
-			goto out;
+			return 0;
 		}
-		goto out_map;
 	}
 
-count_fault:
-	if (nid != NUMA_NO_NODE)
-		task_numa_fault(last_cpupid, nid, HPAGE_PMD_NR, flags);
-
-out:
-	return 0;
-
 out_map:
 	/* Restore the PMD */
 	pmd = pmd_modify(oldpmd, vma->vm_page_prot);
@@ -1750,7 +1743,10 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	set_pmd_at(vma->vm_mm, haddr, vmf->pmd, pmd);
 	update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
 	spin_unlock(vmf->ptl);
-	goto count_fault;
+out:
+	if (nid != NUMA_NO_NODE)
+		task_numa_fault(last_cpupid, nid, HPAGE_PMD_NR, flags);
+	return 0;
 }
 
 /*
diff --git a/mm/memory.c b/mm/memory.c
index 503d493263df..d9b1dff9dc57 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 
 	if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
 		pte_unmap_unlock(vmf->pte, vmf->ptl);
-		goto out;
+		return 0;
 	}
 
 	pte = pte_modify(old_pte, vma->vm_page_prot);
@@ -5523,24 +5523,18 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 	if (!migrate_misplaced_folio(folio, vma, target_nid)) {
 		nid = target_nid;
 		flags |= TNF_MIGRATED;
+		goto out;
 	} else {
 		flags |= TNF_MIGRATE_FAIL;
 		vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
 					       vmf->address, &vmf->ptl);
 		if (unlikely(!vmf->pte))
-			goto out;
+			return 0;
 		if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
 			pte_unmap_unlock(vmf->pte, vmf->ptl);
-			goto out;
+			return 0;
 		}
-		goto out_map;
 	}
-
-count_fault:
-	if (nid != NUMA_NO_NODE)
-		task_numa_fault(last_cpupid, nid, nr_pages, flags);
-out:
-	return 0;
 out_map:
 	/*
 	 * Make it present again, depending on how arch implements
@@ -5553,7 +5547,10 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 		numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
 					    writable);
 	pte_unmap_unlock(vmf->pte, vmf->ptl);
-	goto count_fault;
+out:
+	if (nid != NUMA_NO_NODE)
+		task_numa_fault(last_cpupid, nid, nr_pages, flags);
+	return 0;
 }
 
 static inline vm_fault_t create_huge_pmd(struct vm_fault *vmf)
-- 
2.43.0


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 2/2] mm/migrate: move common code to numa_migrate_check (was numa_migrate_prep)

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 6157 bytes --]

On 8 Aug 2024, at 0:14, Huang, Ying wrote:

> Zi Yan <ziy@nvidia.com> writes:
>
>> do_numa_page() and do_huge_pmd_numa_page() share a lot of common code. To
>> reduce redundancy, move common code to numa_migrate_prep() and rename
>> the function to numa_migrate_check() to reflect its functionality.
>>
>> Now do_huge_pmd_numa_page() also checks shared folios to set TNF_SHARED
>> flag.
>>
>> Suggested-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>> ---
>>  mm/huge_memory.c | 14 ++-------
>>  mm/internal.h    |  5 ++--
>>  mm/memory.c      | 76 ++++++++++++++++++++++++------------------------
>>  3 files changed, 44 insertions(+), 51 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index a3c018f2b554..9b312cae6775 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1699,18 +1699,10 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
>>  	if (!folio)
>>  		goto out_map;
>>
>> -	/* See similar comment in do_numa_page for explanation */
>> -	if (!writable)
>> -		flags |= TNF_NO_GROUP;
>> -
>>  	nid = folio_nid(folio);
>
> NITPICK: It appears that we can remove "nid" local variable.

I thought about it, but

1. if folio is NULL from vm_normal_folio_pmd(), nid remains NUMA_NO_NODE,
2. if migration succeeds, nid is changed to target_nid,
3. if migration fails, nid is the folio node id,

all three are consumed by if (nid != NUMA_NO_NODE) before task_numa_fault().

I will give it a try in next version and see if I can remove it.

>
>> -	/*
>> -	 * For memory tiering mode, cpupid of slow memory page is used
>> -	 * to record page access time.  So use default value.
>> -	 */
>> -	if (!folio_use_access_time(folio))
>> -		last_cpupid = folio_last_cpupid(folio);
>> -	target_nid = numa_migrate_prep(folio, vmf, haddr, nid, &flags);
>> +
>> +	target_nid = numa_migrate_check(folio, vmf, haddr, &flags, writable,
>> +					&last_cpupid);
>>  	if (target_nid == NUMA_NO_NODE)
>>  		goto out_map;
>>  	if (migrate_misplaced_folio_prepare(folio, vma, target_nid)) {
>> diff --git a/mm/internal.h b/mm/internal.h
>> index 52f7fc4e8ac3..fb16e18c9761 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -1191,8 +1191,9 @@ void vunmap_range_noflush(unsigned long start, unsigned long end);
>>
>>  void __vunmap_range_noflush(unsigned long start, unsigned long end);
>>
>> -int numa_migrate_prep(struct folio *folio, struct vm_fault *vmf,
>> -		      unsigned long addr, int page_nid, int *flags);
>> +int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
>> +		      unsigned long addr, int *flags, bool writable,
>> +		      int *last_cpupid);
>>
>>  void free_zone_device_folio(struct folio *folio);
>>  int migrate_device_coherent_page(struct page *page);
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 503d493263df..b093df652c11 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -5368,16 +5368,43 @@ static vm_fault_t do_fault(struct vm_fault *vmf)
>>  	return ret;
>>  }
>>
>> -int numa_migrate_prep(struct folio *folio, struct vm_fault *vmf,
>> -		      unsigned long addr, int page_nid, int *flags)
>> +int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
>> +		      unsigned long addr, int *flags,
>> +		      bool writable, int *last_cpupid)
>>  {
>>  	struct vm_area_struct *vma = vmf->vma;
>>
>> +	/*
>> +	 * Avoid grouping on RO pages in general. RO pages shouldn't hurt as
>> +	 * much anyway since they can be in shared cache state. This misses
>> +	 * the case where a mapping is writable but the process never writes
>> +	 * to it but pte_write gets cleared during protection updates and
>> +	 * pte_dirty has unpredictable behaviour between PTE scan updates,
>> +	 * background writeback, dirty balancing and application behaviour.
>> +	 */
>> +	if (!writable)
>> +		*flags |= TNF_NO_GROUP;
>> +
>> +	/*
>> +	 * Flag if the folio is shared between multiple address spaces. This
>> +	 * is later used when determining whether to group tasks together
>> +	 */
>> +	if (folio_likely_mapped_shared(folio) && (vma->vm_flags & VM_SHARED))
>> +		*flags |= TNF_SHARED;
>> +	/*
>> +	 * For memory tiering mode, cpupid of slow memory page is used
>> +	 * to record page access time.  So use default value.
>> +	 */
>> +	if (folio_use_access_time(folio))
>> +		*last_cpupid = (-1 & LAST_CPUPID_MASK);
>> +	else
>> +		*last_cpupid = folio_last_cpupid(folio);
>> +
>>  	/* Record the current PID acceesing VMA */
>>  	vma_set_access_pid_bit(vma);
>>
>>  	count_vm_numa_event(NUMA_HINT_FAULTS);
>> -	if (page_nid == numa_node_id()) {
>> +	if (folio_nid(folio) == numa_node_id()) {
>>  		count_vm_numa_event(NUMA_HINT_FAULTS_LOCAL);
>>  		*flags |= TNF_FAULT_LOCAL;
>>  	}
>> @@ -5442,13 +5469,13 @@ static void numa_rebuild_large_mapping(struct vm_fault *vmf, struct vm_area_stru
>>  static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>  {
>>  	struct vm_area_struct *vma = vmf->vma;
>> +	pte_t old_pte = vmf->orig_pte;
>
> The usage of old_pte is different from other use cases.  Where,
>
> old_pte = *pte;
> check old_pte and orig_pte
> generate new_pte from old_pte
> set new_pte
>
> We have used this before in do_numa_page(), but not do that now.  But I
> still think that it's better to follow the convention partly if
> possible.  This makes code easier to be read.  I notices that we don't
> follow it in do_huge_pmd_numa_page(), we may change that?

Sure, since I am trying to make do_numa_page() and do_huge_pmd_numa_page()
use the same pattern.

>
>> +	pte_t pte;
>>  	struct folio *folio = NULL;
>>  	int nid = NUMA_NO_NODE;
>>  	bool writable = false, ignore_writable = false;
>>  	bool pte_write_upgrade = vma_wants_manual_pte_write_upgrade(vma);
>> -	int last_cpupid;
>> -	int target_nid;
>> -	pte_t pte, old_pte;
>> +	int target_nid, last_cpupid = (-1 & LAST_CPUPID_MASK);
>
> Because we will initialize last_cpupid in numa_migrate_check(), we don't
> need to initialize it here?

Will remove it.

Thanks for the review.

Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Zi Yan]

[-- Attachment #1.1: Type: text/plain, Size: 3641 bytes --]

On 8 Aug 2024, at 10:57, Zi Yan wrote:

> On 8 Aug 2024, at 10:36, Kefeng Wang wrote:
>
>> On 2024/8/8 22:21, Zi Yan wrote:
>>> On 8 Aug 2024, at 10:14, David Hildenbrand wrote:
>>>
>>>> On 08.08.24 16:13, Zi Yan wrote:
>>>>> On 8 Aug 2024, at 4:22, David Hildenbrand wrote:
>>>>>
>>>>>> On 08.08.24 05:19, Baolin Wang wrote:
>>>>>>>
>>>>>>>
>> ...
>>>>>> Agreed, maybe we should simply handle that right away and replace the "goto out;" users by "return 0;".
>>>>>>
>>>>>> Then, just copy the 3 LOC.
>>>>>>
>>>>>> For mm/memory.c that would be:
>>>>>>
>>>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>>>> index 67496dc5064f..410ba50ca746 100644
>>>>>> --- a/mm/memory.c
>>>>>> +++ b/mm/memory.c
>>>>>> @@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>>            if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
>>>>>>                   pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>>> -               goto out;
>>>>>> +               return 0;
>>>>>>           }
>>>>>>            pte = pte_modify(old_pte, vma->vm_page_prot);
>>>>>> @@ -5528,15 +5528,14 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>>                   vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>>>>>>                                                  vmf->address, &vmf->ptl);
>>>>>>                   if (unlikely(!vmf->pte))
>>>>>> -                       goto out;
>>>>>> +                       return 0;
>>>>>>                   if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
>>>>>>                           pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>>> -                       goto out;
>>>>>> +                       return 0;
>>>>>>                   }
>>>>>>                   goto out_map;
>>>>>>           }
>>>>>>    -out:
>>>>>>           if (nid != NUMA_NO_NODE)
>>>>>>                   task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>>>>>           return 0;
>>
>> Maybe drop this part too,
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 410ba50ca746..07343c1469e0 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -5523,6 +5523,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>         if (!migrate_misplaced_folio(folio, vma, target_nid)) {
>>                 nid = target_nid;
>>                 flags |= TNF_MIGRATED;
>> +               goto out;
>>         } else {
>>                 flags |= TNF_MIGRATE_FAIL;
>>                 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>> @@ -5533,12 +5534,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>                         pte_unmap_unlock(vmf->pte, vmf->ptl);
>>                         return 0;
>>                 }
>> -               goto out_map;
>>         }
>>
>> -       if (nid != NUMA_NO_NODE)
>> -               task_numa_fault(last_cpupid, nid, nr_pages, flags);
>> -       return 0;
>>  out_map:
>>         /*
>>          * Make it present again, depending on how arch implements
>> @@ -5551,6 +5548,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>                 numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
>>                                             writable);
>>         pte_unmap_unlock(vmf->pte, vmf->ptl);
>> +out:
>>         if (nid != NUMA_NO_NODE)
>>                 task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>         return 0;
>
> Even better. Thanks. The updated fixup is attached.
Update the fixup, if Andrew wants to fold it in instead of a resend, again to fix a typo causing compilation failure.

Best Regards,
Yan, Zi

[-- Attachment #1.2: 0001-fixup-mm-numa-no-task_numa_fault-call-if-page-table-.patch --]
[-- Type: text/plain, Size: 3359 bytes --]

From b42f0e90ed0b4117139cf66de2d6f83e3d8bcf8d Mon Sep 17 00:00:00 2001
From: Zi Yan <ziy@nvidia.com>
Date: Thu, 8 Aug 2024 10:18:42 -0400
Subject: [PATCH] fixup! mm/numa: no task_numa_fault() call if page table is
 changed

---
 mm/huge_memory.c | 18 +++++++-----------
 mm/memory.c      | 19 ++++++++-----------
 2 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a3c018f2b554..4e4364a17e6d 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1681,7 +1681,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
 	if (unlikely(!pmd_same(oldpmd, *vmf->pmd))) {
 		spin_unlock(vmf->ptl);
-		goto out;
+		return 0;
 	}
 
 	pmd = pmd_modify(oldpmd, vma->vm_page_prot);
@@ -1724,23 +1724,16 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	if (!migrate_misplaced_folio(folio, vma, target_nid)) {
 		flags |= TNF_MIGRATED;
 		nid = target_nid;
+		goto out;
 	} else {
 		flags |= TNF_MIGRATE_FAIL;
 		vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
 		if (unlikely(!pmd_same(oldpmd, *vmf->pmd))) {
 			spin_unlock(vmf->ptl);
-			goto out;
+			return 0;
 		}
-		goto out_map;
 	}
 
-count_fault:
-	if (nid != NUMA_NO_NODE)
-		task_numa_fault(last_cpupid, nid, HPAGE_PMD_NR, flags);
-
-out:
-	return 0;
-
 out_map:
 	/* Restore the PMD */
 	pmd = pmd_modify(oldpmd, vma->vm_page_prot);
@@ -1750,7 +1743,10 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	set_pmd_at(vma->vm_mm, haddr, vmf->pmd, pmd);
 	update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
 	spin_unlock(vmf->ptl);
-	goto count_fault;
+out:
+	if (nid != NUMA_NO_NODE)
+		task_numa_fault(last_cpupid, nid, HPAGE_PMD_NR, flags);
+	return 0;
 }
 
 /*
diff --git a/mm/memory.c b/mm/memory.c
index 503d493263df..d9b1dff9dc57 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 
 	if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
 		pte_unmap_unlock(vmf->pte, vmf->ptl);
-		goto out;
+		return 0;
 	}
 
 	pte = pte_modify(old_pte, vma->vm_page_prot);
@@ -5523,24 +5523,18 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 	if (!migrate_misplaced_folio(folio, vma, target_nid)) {
 		nid = target_nid;
 		flags |= TNF_MIGRATED;
+		goto out;
 	} else {
 		flags |= TNF_MIGRATE_FAIL;
 		vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
 					       vmf->address, &vmf->ptl);
 		if (unlikely(!vmf->pte))
-			goto out;
+			return 0;
 		if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
 			pte_unmap_unlock(vmf->pte, vmf->ptl);
-			goto out;
+			return 0;
 		}
-		goto out_map;
 	}
-
-count_fault:
-	if (nid != NUMA_NO_NODE)
-		task_numa_fault(last_cpupid, nid, nr_pages, flags);
-out:
-	return 0;
 out_map:
 	/*
 	 * Make it present again, depending on how arch implements
@@ -5553,7 +5547,10 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 		numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
 					    writable);
 	pte_unmap_unlock(vmf->pte, vmf->ptl);
-	goto count_fault;
+out:
+	if (nid != NUMA_NO_NODE)
+		task_numa_fault(last_cpupid, nid, nr_pages, flags);
+	return 0;
 }
 
 static inline vm_fault_t create_huge_pmd(struct vm_fault *vmf)
-- 
2.43.0


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 2/2] mm/migrate: move common code to numa_migrate_check (was numa_migrate_prep)

[Zi Yan]

[-- Attachment #1.1: Type: text/plain, Size: 6451 bytes --]

On 8 Aug 2024, at 10:59, Zi Yan wrote:

> On 8 Aug 2024, at 0:14, Huang, Ying wrote:
>
>> Zi Yan <ziy@nvidia.com> writes:
>>
>>> do_numa_page() and do_huge_pmd_numa_page() share a lot of common code. To
>>> reduce redundancy, move common code to numa_migrate_prep() and rename
>>> the function to numa_migrate_check() to reflect its functionality.
>>>
>>> Now do_huge_pmd_numa_page() also checks shared folios to set TNF_SHARED
>>> flag.
>>>
>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>> ---
>>>  mm/huge_memory.c | 14 ++-------
>>>  mm/internal.h    |  5 ++--
>>>  mm/memory.c      | 76 ++++++++++++++++++++++++------------------------
>>>  3 files changed, 44 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>> index a3c018f2b554..9b312cae6775 100644
>>> --- a/mm/huge_memory.c
>>> +++ b/mm/huge_memory.c
>>> @@ -1699,18 +1699,10 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
>>>  	if (!folio)
>>>  		goto out_map;
>>>
>>> -	/* See similar comment in do_numa_page for explanation */
>>> -	if (!writable)
>>> -		flags |= TNF_NO_GROUP;
>>> -
>>>  	nid = folio_nid(folio);
>>
>> NITPICK: It appears that we can remove "nid" local variable.
>
> I thought about it, but
>
> 1. if folio is NULL from vm_normal_folio_pmd(), nid remains NUMA_NO_NODE,
> 2. if migration succeeds, nid is changed to target_nid,
> 3. if migration fails, nid is the folio node id,
>
> all three are consumed by if (nid != NUMA_NO_NODE) before task_numa_fault().
>
> I will give it a try in next version and see if I can remove it.
>
>>
>>> -	/*
>>> -	 * For memory tiering mode, cpupid of slow memory page is used
>>> -	 * to record page access time.  So use default value.
>>> -	 */
>>> -	if (!folio_use_access_time(folio))
>>> -		last_cpupid = folio_last_cpupid(folio);
>>> -	target_nid = numa_migrate_prep(folio, vmf, haddr, nid, &flags);
>>> +
>>> +	target_nid = numa_migrate_check(folio, vmf, haddr, &flags, writable,
>>> +					&last_cpupid);
>>>  	if (target_nid == NUMA_NO_NODE)
>>>  		goto out_map;
>>>  	if (migrate_misplaced_folio_prepare(folio, vma, target_nid)) {
>>> diff --git a/mm/internal.h b/mm/internal.h
>>> index 52f7fc4e8ac3..fb16e18c9761 100644
>>> --- a/mm/internal.h
>>> +++ b/mm/internal.h
>>> @@ -1191,8 +1191,9 @@ void vunmap_range_noflush(unsigned long start, unsigned long end);
>>>
>>>  void __vunmap_range_noflush(unsigned long start, unsigned long end);
>>>
>>> -int numa_migrate_prep(struct folio *folio, struct vm_fault *vmf,
>>> -		      unsigned long addr, int page_nid, int *flags);
>>> +int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
>>> +		      unsigned long addr, int *flags, bool writable,
>>> +		      int *last_cpupid);
>>>
>>>  void free_zone_device_folio(struct folio *folio);
>>>  int migrate_device_coherent_page(struct page *page);
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index 503d493263df..b093df652c11 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -5368,16 +5368,43 @@ static vm_fault_t do_fault(struct vm_fault *vmf)
>>>  	return ret;
>>>  }
>>>
>>> -int numa_migrate_prep(struct folio *folio, struct vm_fault *vmf,
>>> -		      unsigned long addr, int page_nid, int *flags)
>>> +int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
>>> +		      unsigned long addr, int *flags,
>>> +		      bool writable, int *last_cpupid)
>>>  {
>>>  	struct vm_area_struct *vma = vmf->vma;
>>>
>>> +	/*
>>> +	 * Avoid grouping on RO pages in general. RO pages shouldn't hurt as
>>> +	 * much anyway since they can be in shared cache state. This misses
>>> +	 * the case where a mapping is writable but the process never writes
>>> +	 * to it but pte_write gets cleared during protection updates and
>>> +	 * pte_dirty has unpredictable behaviour between PTE scan updates,
>>> +	 * background writeback, dirty balancing and application behaviour.
>>> +	 */
>>> +	if (!writable)
>>> +		*flags |= TNF_NO_GROUP;
>>> +
>>> +	/*
>>> +	 * Flag if the folio is shared between multiple address spaces. This
>>> +	 * is later used when determining whether to group tasks together
>>> +	 */
>>> +	if (folio_likely_mapped_shared(folio) && (vma->vm_flags & VM_SHARED))
>>> +		*flags |= TNF_SHARED;
>>> +	/*
>>> +	 * For memory tiering mode, cpupid of slow memory page is used
>>> +	 * to record page access time.  So use default value.
>>> +	 */
>>> +	if (folio_use_access_time(folio))
>>> +		*last_cpupid = (-1 & LAST_CPUPID_MASK);
>>> +	else
>>> +		*last_cpupid = folio_last_cpupid(folio);
>>> +
>>>  	/* Record the current PID acceesing VMA */
>>>  	vma_set_access_pid_bit(vma);
>>>
>>>  	count_vm_numa_event(NUMA_HINT_FAULTS);
>>> -	if (page_nid == numa_node_id()) {
>>> +	if (folio_nid(folio) == numa_node_id()) {
>>>  		count_vm_numa_event(NUMA_HINT_FAULTS_LOCAL);
>>>  		*flags |= TNF_FAULT_LOCAL;
>>>  	}
>>> @@ -5442,13 +5469,13 @@ static void numa_rebuild_large_mapping(struct vm_fault *vmf, struct vm_area_stru
>>>  static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>  {
>>>  	struct vm_area_struct *vma = vmf->vma;
>>> +	pte_t old_pte = vmf->orig_pte;
>>
>> The usage of old_pte is different from other use cases.  Where,
>>
>> old_pte = *pte;
>> check old_pte and orig_pte
>> generate new_pte from old_pte
>> set new_pte
>>
>> We have used this before in do_numa_page(), but not do that now.  But I
>> still think that it's better to follow the convention partly if
>> possible.  This makes code easier to be read.  I notices that we don't
>> follow it in do_huge_pmd_numa_page(), we may change that?
>
> Sure, since I am trying to make do_numa_page() and do_huge_pmd_numa_page()
> use the same pattern.
>
>>
>>> +	pte_t pte;
>>>  	struct folio *folio = NULL;
>>>  	int nid = NUMA_NO_NODE;
>>>  	bool writable = false, ignore_writable = false;
>>>  	bool pte_write_upgrade = vma_wants_manual_pte_write_upgrade(vma);
>>> -	int last_cpupid;
>>> -	int target_nid;
>>> -	pte_t pte, old_pte;
>>> +	int target_nid, last_cpupid = (-1 & LAST_CPUPID_MASK);
>>
>> Because we will initialize last_cpupid in numa_migrate_check(), we don't
>> need to initialize it here?
>
> Will remove it.
>
> Thanks for the review.

I attached my fixup and updated patch 2.

Let me know how it looks.

--
Best Regards,
Yan, Zi

[-- Attachment #1.2: 0001-mm-migrate-move-common-code-to-numa_migrate_check-wa.patch --]
[-- Type: text/plain, Size: 7341 bytes --]

From 9d2f9fc7a3e7d21af820c91aa239b6a2511d7840 Mon Sep 17 00:00:00 2001
From: Zi Yan <ziy@nvidia.com>
Date: Thu, 8 Aug 2024 11:17:03 -0400
Subject: [PATCH] mm/migrate: move common code to numa_migrate_check (was
 numa_migrate_prep)

do_numa_page() and do_huge_pmd_numa_page() share a lot of common code. To
reduce redundancy, move common code to numa_migrate_prep() and rename
the function to numa_migrate_check() to reflect its functionality.

Now do_huge_pmd_numa_page() also checks shared folios to set TNF_SHARED
flag.

Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Zi Yan <ziy@nvidia.com>
---
 mm/huge_memory.c | 29 +++++++++-------------
 mm/internal.h    |  5 ++--
 mm/memory.c      | 63 +++++++++++++++++++++++++-----------------------
 3 files changed, 47 insertions(+), 50 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 4e4364a17e6d..96a52e71d167 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1669,22 +1669,23 @@ static inline bool can_change_pmd_writable(struct vm_area_struct *vma,
 vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
-	pmd_t oldpmd = vmf->orig_pmd;
-	pmd_t pmd;
 	struct folio *folio;
 	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
 	int nid = NUMA_NO_NODE;
-	int target_nid, last_cpupid = (-1 & LAST_CPUPID_MASK);
+	int target_nid, last_cpupid;
+	pmd_t pmd, old_pmd;
 	bool writable = false;
 	int flags = 0;
 
 	vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
-	if (unlikely(!pmd_same(oldpmd, *vmf->pmd))) {
+	old_pmd = pmdp_get(vmf->pmd);
+
+	if (unlikely(!pmd_same(old_pmd, vmf->orig_pmd))) {
 		spin_unlock(vmf->ptl);
 		return 0;
 	}
 
-	pmd = pmd_modify(oldpmd, vma->vm_page_prot);
+	pmd = pmd_modify(old_pmd, vma->vm_page_prot);
 
 	/*
 	 * Detect now whether the PMD could be writable; this information
@@ -1699,18 +1700,10 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	if (!folio)
 		goto out_map;
 
-	/* See similar comment in do_numa_page for explanation */
-	if (!writable)
-		flags |= TNF_NO_GROUP;
-
 	nid = folio_nid(folio);
-	/*
-	 * For memory tiering mode, cpupid of slow memory page is used
-	 * to record page access time.  So use default value.
-	 */
-	if (!folio_use_access_time(folio))
-		last_cpupid = folio_last_cpupid(folio);
-	target_nid = numa_migrate_prep(folio, vmf, haddr, nid, &flags);
+
+	target_nid = numa_migrate_check(folio, vmf, haddr, &flags, writable,
+					&last_cpupid);
 	if (target_nid == NUMA_NO_NODE)
 		goto out_map;
 	if (migrate_misplaced_folio_prepare(folio, vma, target_nid)) {
@@ -1728,7 +1721,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	} else {
 		flags |= TNF_MIGRATE_FAIL;
 		vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
-		if (unlikely(!pmd_same(oldpmd, *vmf->pmd))) {
+		if (unlikely(!pmd_same(pmdp_get(vmf->pmd), vmf->orig_pmd))) {
 			spin_unlock(vmf->ptl);
 			return 0;
 		}
@@ -1736,7 +1729,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 
 out_map:
 	/* Restore the PMD */
-	pmd = pmd_modify(oldpmd, vma->vm_page_prot);
+	pmd = pmd_modify(pmdp_get(vmf->pmd), vma->vm_page_prot);
 	pmd = pmd_mkyoung(pmd);
 	if (writable)
 		pmd = pmd_mkwrite(pmd, vma);
diff --git a/mm/internal.h b/mm/internal.h
index 52f7fc4e8ac3..fb16e18c9761 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1191,8 +1191,9 @@ void vunmap_range_noflush(unsigned long start, unsigned long end);
 
 void __vunmap_range_noflush(unsigned long start, unsigned long end);
 
-int numa_migrate_prep(struct folio *folio, struct vm_fault *vmf,
-		      unsigned long addr, int page_nid, int *flags);
+int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
+		      unsigned long addr, int *flags, bool writable,
+		      int *last_cpupid);
 
 void free_zone_device_folio(struct folio *folio);
 int migrate_device_coherent_page(struct page *page);
diff --git a/mm/memory.c b/mm/memory.c
index d9b1dff9dc57..3441f60d54ef 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5368,16 +5368,43 @@ static vm_fault_t do_fault(struct vm_fault *vmf)
 	return ret;
 }
 
-int numa_migrate_prep(struct folio *folio, struct vm_fault *vmf,
-		      unsigned long addr, int page_nid, int *flags)
+int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
+		      unsigned long addr, int *flags,
+		      bool writable, int *last_cpupid)
 {
 	struct vm_area_struct *vma = vmf->vma;
 
+	/*
+	 * Avoid grouping on RO pages in general. RO pages shouldn't hurt as
+	 * much anyway since they can be in shared cache state. This misses
+	 * the case where a mapping is writable but the process never writes
+	 * to it but pte_write gets cleared during protection updates and
+	 * pte_dirty has unpredictable behaviour between PTE scan updates,
+	 * background writeback, dirty balancing and application behaviour.
+	 */
+	if (!writable)
+		*flags |= TNF_NO_GROUP;
+
+	/*
+	 * Flag if the folio is shared between multiple address spaces. This
+	 * is later used when determining whether to group tasks together
+	 */
+	if (folio_likely_mapped_shared(folio) && (vma->vm_flags & VM_SHARED))
+		*flags |= TNF_SHARED;
+	/*
+	 * For memory tiering mode, cpupid of slow memory page is used
+	 * to record page access time.  So use default value.
+	 */
+	if (folio_use_access_time(folio))
+		*last_cpupid = (-1 & LAST_CPUPID_MASK);
+	else
+		*last_cpupid = folio_last_cpupid(folio);
+
 	/* Record the current PID acceesing VMA */
 	vma_set_access_pid_bit(vma);
 
 	count_vm_numa_event(NUMA_HINT_FAULTS);
-	if (page_nid == numa_node_id()) {
+	if (folio_nid(folio) == numa_node_id()) {
 		count_vm_numa_event(NUMA_HINT_FAULTS_LOCAL);
 		*flags |= TNF_FAULT_LOCAL;
 	}
@@ -5479,35 +5506,11 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 	if (!folio || folio_is_zone_device(folio))
 		goto out_map;
 
-	/*
-	 * Avoid grouping on RO pages in general. RO pages shouldn't hurt as
-	 * much anyway since they can be in shared cache state. This misses
-	 * the case where a mapping is writable but the process never writes
-	 * to it but pte_write gets cleared during protection updates and
-	 * pte_dirty has unpredictable behaviour between PTE scan updates,
-	 * background writeback, dirty balancing and application behaviour.
-	 */
-	if (!writable)
-		flags |= TNF_NO_GROUP;
-
-	/*
-	 * Flag if the folio is shared between multiple address spaces. This
-	 * is later used when determining whether to group tasks together
-	 */
-	if (folio_likely_mapped_shared(folio) && (vma->vm_flags & VM_SHARED))
-		flags |= TNF_SHARED;
-
 	nid = folio_nid(folio);
 	nr_pages = folio_nr_pages(folio);
-	/*
-	 * For memory tiering mode, cpupid of slow memory page is used
-	 * to record page access time.  So use default value.
-	 */
-	if (folio_use_access_time(folio))
-		last_cpupid = (-1 & LAST_CPUPID_MASK);
-	else
-		last_cpupid = folio_last_cpupid(folio);
-	target_nid = numa_migrate_prep(folio, vmf, vmf->address, nid, &flags);
+
+	target_nid = numa_migrate_check(folio, vmf, vmf->address, &flags,
+					writable, &last_cpupid);
 	if (target_nid == NUMA_NO_NODE)
 		goto out_map;
 	if (migrate_misplaced_folio_prepare(folio, vma, target_nid)) {
-- 
2.43.0


[-- Attachment #1.3: patch2_fixup.patch --]
[-- Type: text/plain, Size: 3198 bytes --]

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 5610289865a7..96a52e71d167 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1669,22 +1669,23 @@ static inline bool can_change_pmd_writable(struct vm_area_struct *vma,
 vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
-	pmd_t oldpmd = vmf->orig_pmd;
-	pmd_t pmd;
 	struct folio *folio;
 	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
 	int nid = NUMA_NO_NODE;
-	int target_nid, last_cpupid = (-1 & LAST_CPUPID_MASK);
+	int target_nid, last_cpupid;
+	pmd_t pmd, old_pmd;
 	bool writable = false;
 	int flags = 0;
 
 	vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
-	if (unlikely(!pmd_same(oldpmd, *vmf->pmd))) {
+	old_pmd = pmdp_get(vmf->pmd);
+
+	if (unlikely(!pmd_same(old_pmd, vmf->orig_pmd))) {
 		spin_unlock(vmf->ptl);
 		return 0;
 	}
 
-	pmd = pmd_modify(oldpmd, vma->vm_page_prot);
+	pmd = pmd_modify(old_pmd, vma->vm_page_prot);
 
 	/*
 	 * Detect now whether the PMD could be writable; this information
@@ -1720,7 +1721,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	} else {
 		flags |= TNF_MIGRATE_FAIL;
 		vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
-		if (unlikely(!pmd_same(oldpmd, *vmf->pmd))) {
+		if (unlikely(!pmd_same(pmdp_get(vmf->pmd), vmf->orig_pmd))) {
 			spin_unlock(vmf->ptl);
 			return 0;
 		}
@@ -1728,7 +1729,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 
 out_map:
 	/* Restore the PMD */
-	pmd = pmd_modify(oldpmd, vma->vm_page_prot);
+	pmd = pmd_modify(pmdp_get(vmf->pmd), vma->vm_page_prot);
 	pmd = pmd_mkyoung(pmd);
 	if (writable)
 		pmd = pmd_mkwrite(pmd, vma);
diff --git a/mm/memory.c b/mm/memory.c
index f186a8d8c992..3441f60d54ef 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5469,13 +5469,13 @@ static void numa_rebuild_large_mapping(struct vm_fault *vmf, struct vm_area_stru
 static vm_fault_t do_numa_page(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
-	pte_t old_pte = vmf->orig_pte;
-	pte_t pte;
 	struct folio *folio = NULL;
 	int nid = NUMA_NO_NODE;
 	bool writable = false, ignore_writable = false;
 	bool pte_write_upgrade = vma_wants_manual_pte_write_upgrade(vma);
-	int target_nid, last_cpupid = (-1 & LAST_CPUPID_MASK);
+	int last_cpupid;
+	int target_nid;
+	pte_t pte, old_pte;
 	int flags = 0, nr_pages;
 
 	/*
@@ -5483,7 +5483,10 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 	 * table lock, that its contents have not changed during fault handling.
 	 */
 	spin_lock(vmf->ptl);
-	if (unlikely(!pte_same(old_pte, ptep_get(vmf->pte)))) {
+	/* Read the live PTE from the page tables: */
+	old_pte = ptep_get(vmf->pte);
+
+	if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
 		pte_unmap_unlock(vmf->pte, vmf->ptl);
 		return 0;
 	}
@@ -5530,7 +5533,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 					       vmf->address, &vmf->ptl);
 		if (unlikely(!vmf->pte))
 			return 0;
-		if (unlikely(!pte_same(old_pte, ptep_get(vmf->pte)))) {
+		if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
 			pte_unmap_unlock(vmf->pte, vmf->ptl);
 			return 0;
 		}

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Huang, Ying]

Kefeng Wang <wangkefeng.wang@huawei.com> writes:

> On 2024/8/8 22:21, Zi Yan wrote:
>> On 8 Aug 2024, at 10:14, David Hildenbrand wrote:
>> 
>>> On 08.08.24 16:13, Zi Yan wrote:
>>>> On 8 Aug 2024, at 4:22, David Hildenbrand wrote:
>>>>
>>>>> On 08.08.24 05:19, Baolin Wang wrote:
>>>>>>
>>>>>>
> ...
>>>>> Agreed, maybe we should simply handle that right away and replace the "goto out;" users by "return 0;".
>>>>>
>>>>> Then, just copy the 3 LOC.
>>>>>
>>>>> For mm/memory.c that would be:
>>>>>
>>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>>> index 67496dc5064f..410ba50ca746 100644
>>>>> --- a/mm/memory.c
>>>>> +++ b/mm/memory.c
>>>>> @@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>            if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
>>>>>                   pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>> -               goto out;
>>>>> +               return 0;
>>>>>           }
>>>>>            pte = pte_modify(old_pte, vma->vm_page_prot);
>>>>> @@ -5528,15 +5528,14 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>                   vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>>>>>                                                  vmf->address, &vmf->ptl);
>>>>>                   if (unlikely(!vmf->pte))
>>>>> -                       goto out;
>>>>> +                       return 0;
>>>>>                   if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
>>>>>                           pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>> -                       goto out;
>>>>> +                       return 0;
>>>>>                   }
>>>>>                   goto out_map;
>>>>>           }
>>>>>    -out:
>>>>>           if (nid != NUMA_NO_NODE)
>>>>>                   task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>>>>           return 0;
>
> Maybe drop this part too,
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 410ba50ca746..07343c1469e0 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5523,6 +5523,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>         if (!migrate_misplaced_folio(folio, vma, target_nid)) {
>                 nid = target_nid;
>                 flags |= TNF_MIGRATED;
> +               goto out;
>         } else {
>                 flags |= TNF_MIGRATE_FAIL;
>                 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
> @@ -5533,12 +5534,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>                         pte_unmap_unlock(vmf->pte, vmf->ptl);
>                         return 0;
>                 }
> -               goto out_map;
>         }
>
> -       if (nid != NUMA_NO_NODE)
> -               task_numa_fault(last_cpupid, nid, nr_pages, flags);
> -       return 0;
>  out_map:
>         /*
>          * Make it present again, depending on how arch implements

IMHO, migration success is normal path, while migration failure is error
processing path.  If so, it's better to use "goto" for error processing
instead of normal path.

> @@ -5551,6 +5548,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>                 numa_rebuild_single_mapping(vmf, vma, vmf->address,
>                 vmf->pte,
>                                             writable);
>         pte_unmap_unlock(vmf->pte, vmf->ptl);
> +out:
>         if (nid != NUMA_NO_NODE)
>                 task_numa_fault(last_cpupid, nid, nr_pages, flags);
>         return 0;
>
>

--
Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 7041 bytes --]

On 8 Aug 2024, at 21:25, Huang, Ying wrote:

> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>
>> On 2024/8/8 22:21, Zi Yan wrote:
>>> On 8 Aug 2024, at 10:14, David Hildenbrand wrote:
>>>
>>>> On 08.08.24 16:13, Zi Yan wrote:
>>>>> On 8 Aug 2024, at 4:22, David Hildenbrand wrote:
>>>>>
>>>>>> On 08.08.24 05:19, Baolin Wang wrote:
>>>>>>>
>>>>>>>
>> ...
>>>>>> Agreed, maybe we should simply handle that right away and replace the "goto out;" users by "return 0;".
>>>>>>
>>>>>> Then, just copy the 3 LOC.
>>>>>>
>>>>>> For mm/memory.c that would be:
>>>>>>
>>>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>>>> index 67496dc5064f..410ba50ca746 100644
>>>>>> --- a/mm/memory.c
>>>>>> +++ b/mm/memory.c
>>>>>> @@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>>            if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
>>>>>>                   pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>>> -               goto out;
>>>>>> +               return 0;
>>>>>>           }
>>>>>>            pte = pte_modify(old_pte, vma->vm_page_prot);
>>>>>> @@ -5528,15 +5528,14 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>>                   vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>>>>>>                                                  vmf->address, &vmf->ptl);
>>>>>>                   if (unlikely(!vmf->pte))
>>>>>> -                       goto out;
>>>>>> +                       return 0;
>>>>>>                   if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
>>>>>>                           pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>>> -                       goto out;
>>>>>> +                       return 0;
>>>>>>                   }
>>>>>>                   goto out_map;
>>>>>>           }
>>>>>>    -out:
>>>>>>           if (nid != NUMA_NO_NODE)
>>>>>>                   task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>>>>>           return 0;
>>
>> Maybe drop this part too,
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 410ba50ca746..07343c1469e0 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -5523,6 +5523,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>         if (!migrate_misplaced_folio(folio, vma, target_nid)) {
>>                 nid = target_nid;
>>                 flags |= TNF_MIGRATED;
>> +               goto out;
>>         } else {
>>                 flags |= TNF_MIGRATE_FAIL;
>>                 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>> @@ -5533,12 +5534,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>                         pte_unmap_unlock(vmf->pte, vmf->ptl);
>>                         return 0;
>>                 }
>> -               goto out_map;
>>         }
>>
>> -       if (nid != NUMA_NO_NODE)
>> -               task_numa_fault(last_cpupid, nid, nr_pages, flags);
>> -       return 0;
>>  out_map:
>>         /*
>>          * Make it present again, depending on how arch implements
>
> IMHO, migration success is normal path, while migration failure is error
> processing path.  If so, it's better to use "goto" for error processing
> instead of normal path.
>
>> @@ -5551,6 +5548,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>                 numa_rebuild_single_mapping(vmf, vma, vmf->address,
>>                 vmf->pte,
>>                                             writable);
>>         pte_unmap_unlock(vmf->pte, vmf->ptl);
>> +out:
>>         if (nid != NUMA_NO_NODE)
>>                 task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>         return 0;
>>
>>

How about calling task_numa_fault and return in the migration successful path?
(target_nid cannot be NUMA_NO_NODE, so if is not needed)

diff --git a/mm/memory.c b/mm/memory.c
index 3441f60d54ef..abdb73a68b80 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5526,7 +5526,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
        if (!migrate_misplaced_folio(folio, vma, target_nid)) {
                nid = target_nid;
                flags |= TNF_MIGRATED;
-               goto out;
+               task_numa_fault(last_cpupid, nid, nr_pages, flags);
+               return 0;
        } else {
                flags |= TNF_MIGRATE_FAIL;
                vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
@@ -5550,7 +5551,6 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
                numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
                                            writable);
        pte_unmap_unlock(vmf->pte, vmf->ptl);
-out:
        if (nid != NUMA_NO_NODE)
                task_numa_fault(last_cpupid, nid, nr_pages, flags);
        return 0;




Or move the make-present code inside migration failed branch? This one
does not duplicate code but others can jump into this branch.

diff --git a/mm/memory.c b/mm/memory.c
index 3441f60d54ef..c9b4e7099815 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5526,7 +5526,6 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
        if (!migrate_misplaced_folio(folio, vma, target_nid)) {
                nid = target_nid;
                flags |= TNF_MIGRATED;
-               goto out;
        } else {
                flags |= TNF_MIGRATE_FAIL;
                vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
@@ -5537,20 +5536,20 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
                        pte_unmap_unlock(vmf->pte, vmf->ptl);
                        return 0;
                }
-       }
 out_map:
-       /*
-        * Make it present again, depending on how arch implements
-        * non-accessible ptes, some can allow access by kernel mode.
-        */
-       if (folio && folio_test_large(folio))
-               numa_rebuild_large_mapping(vmf, vma, folio, pte, ignore_writable,
-                                          pte_write_upgrade);
-       else
-               numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
-                                           writable);
-       pte_unmap_unlock(vmf->pte, vmf->ptl);
-out:
+               /*
+                * Make it present again, depending on how arch implements
+                * non-accessible ptes, some can allow access by kernel mode.
+                */
+               if (folio && folio_test_large(folio))
+                       numa_rebuild_large_mapping(vmf, vma, folio, pte,
+                                       ignore_writable, pte_write_upgrade);
+               else
+                       numa_rebuild_single_mapping(vmf, vma, vmf->address,
+                                       vmf->pte, writable);
+               pte_unmap_unlock(vmf->pte, vmf->ptl);
+       }
+
        if (nid != NUMA_NO_NODE)
                task_numa_fault(last_cpupid, nid, nr_pages, flags);
        return 0;


Of course, I will need to change mm/huge_memory.c as well.

Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Huang, Ying]

Zi Yan <ziy@nvidia.com> writes:

> On 8 Aug 2024, at 21:25, Huang, Ying wrote:
>
>> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>>
>>> On 2024/8/8 22:21, Zi Yan wrote:
>>>> On 8 Aug 2024, at 10:14, David Hildenbrand wrote:
>>>>
>>>>> On 08.08.24 16:13, Zi Yan wrote:
>>>>>> On 8 Aug 2024, at 4:22, David Hildenbrand wrote:
>>>>>>
>>>>>>> On 08.08.24 05:19, Baolin Wang wrote:
>>>>>>>>
>>>>>>>>
>>> ...
>>>>>>> Agreed, maybe we should simply handle that right away and replace the "goto out;" users by "return 0;".
>>>>>>>
>>>>>>> Then, just copy the 3 LOC.
>>>>>>>
>>>>>>> For mm/memory.c that would be:
>>>>>>>
>>>>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>>>>> index 67496dc5064f..410ba50ca746 100644
>>>>>>> --- a/mm/memory.c
>>>>>>> +++ b/mm/memory.c
>>>>>>> @@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>>>            if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
>>>>>>>                   pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>>>> -               goto out;
>>>>>>> +               return 0;
>>>>>>>           }
>>>>>>>            pte = pte_modify(old_pte, vma->vm_page_prot);
>>>>>>> @@ -5528,15 +5528,14 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>>>                   vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>>>>>>>                                                  vmf->address, &vmf->ptl);
>>>>>>>                   if (unlikely(!vmf->pte))
>>>>>>> -                       goto out;
>>>>>>> +                       return 0;
>>>>>>>                   if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
>>>>>>>                           pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>>>> -                       goto out;
>>>>>>> +                       return 0;
>>>>>>>                   }
>>>>>>>                   goto out_map;
>>>>>>>           }
>>>>>>>    -out:
>>>>>>>           if (nid != NUMA_NO_NODE)
>>>>>>>                   task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>>>>>>           return 0;
>>>
>>> Maybe drop this part too,
>>>
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index 410ba50ca746..07343c1469e0 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -5523,6 +5523,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>         if (!migrate_misplaced_folio(folio, vma, target_nid)) {
>>>                 nid = target_nid;
>>>                 flags |= TNF_MIGRATED;
>>> +               goto out;
>>>         } else {
>>>                 flags |= TNF_MIGRATE_FAIL;
>>>                 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>>> @@ -5533,12 +5534,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>                         pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>                         return 0;
>>>                 }
>>> -               goto out_map;
>>>         }
>>>
>>> -       if (nid != NUMA_NO_NODE)
>>> -               task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>> -       return 0;
>>>  out_map:
>>>         /*
>>>          * Make it present again, depending on how arch implements
>>
>> IMHO, migration success is normal path, while migration failure is error
>> processing path.  If so, it's better to use "goto" for error processing
>> instead of normal path.
>>
>>> @@ -5551,6 +5548,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>                 numa_rebuild_single_mapping(vmf, vma, vmf->address,
>>>                 vmf->pte,
>>>                                             writable);
>>>         pte_unmap_unlock(vmf->pte, vmf->ptl);
>>> +out:
>>>         if (nid != NUMA_NO_NODE)
>>>                 task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>>         return 0;
>>>
>>>
>
> How about calling task_numa_fault and return in the migration successful path?
> (target_nid cannot be NUMA_NO_NODE, so if is not needed)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 3441f60d54ef..abdb73a68b80 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5526,7 +5526,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>         if (!migrate_misplaced_folio(folio, vma, target_nid)) {
>                 nid = target_nid;
>                 flags |= TNF_MIGRATED;
> -               goto out;
> +               task_numa_fault(last_cpupid, nid, nr_pages, flags);
> +               return 0;
>         } else {
>                 flags |= TNF_MIGRATE_FAIL;
>                 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
> @@ -5550,7 +5551,6 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>                 numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
>                                             writable);
>         pte_unmap_unlock(vmf->pte, vmf->ptl);
> -out:
>         if (nid != NUMA_NO_NODE)
>                 task_numa_fault(last_cpupid, nid, nr_pages, flags);
>         return 0;
>

This looks better for me, or change it further.

       if (migrate_misplaced_folio(folio, vma, target_nid))
               goto out_map_pt;

       nid = target_nid;
       flags |= TNF_MIGRATED;
       task_numa_fault(last_cpupid, nid, nr_pages, flags);

       return 0;

out_map_pt:
       flags |= TNF_MIGRATE_FAIL;
       vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
       ...
        
>
>
> Or move the make-present code inside migration failed branch? This one
> does not duplicate code but others can jump into this branch.
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 3441f60d54ef..c9b4e7099815 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5526,7 +5526,6 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>         if (!migrate_misplaced_folio(folio, vma, target_nid)) {
>                 nid = target_nid;
>                 flags |= TNF_MIGRATED;
> -               goto out;
>         } else {
>                 flags |= TNF_MIGRATE_FAIL;
>                 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
> @@ -5537,20 +5536,20 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>                         pte_unmap_unlock(vmf->pte, vmf->ptl);
>                         return 0;
>                 }
> -       }
>  out_map:
> -       /*
> -        * Make it present again, depending on how arch implements
> -        * non-accessible ptes, some can allow access by kernel mode.
> -        */
> -       if (folio && folio_test_large(folio))
> -               numa_rebuild_large_mapping(vmf, vma, folio, pte, ignore_writable,
> -                                          pte_write_upgrade);
> -       else
> -               numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
> -                                           writable);
> -       pte_unmap_unlock(vmf->pte, vmf->ptl);
> -out:
> +               /*
> +                * Make it present again, depending on how arch implements
> +                * non-accessible ptes, some can allow access by kernel mode.
> +                */
> +               if (folio && folio_test_large(folio))
> +                       numa_rebuild_large_mapping(vmf, vma, folio, pte,
> +                                       ignore_writable, pte_write_upgrade);
> +               else
> +                       numa_rebuild_single_mapping(vmf, vma, vmf->address,
> +                                       vmf->pte, writable);
> +               pte_unmap_unlock(vmf->pte, vmf->ptl);
> +       }
> +
>         if (nid != NUMA_NO_NODE)
>                 task_numa_fault(last_cpupid, nid, nr_pages, flags);
>         return 0;
>
>
> Of course, I will need to change mm/huge_memory.c as well.
>

--
Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 8052 bytes --]

On 9 Aug 2024, at 3:52, Huang, Ying wrote:

> Zi Yan <ziy@nvidia.com> writes:
>
>> On 8 Aug 2024, at 21:25, Huang, Ying wrote:
>>
>>> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>>>
>>>> On 2024/8/8 22:21, Zi Yan wrote:
>>>>> On 8 Aug 2024, at 10:14, David Hildenbrand wrote:
>>>>>
>>>>>> On 08.08.24 16:13, Zi Yan wrote:
>>>>>>> On 8 Aug 2024, at 4:22, David Hildenbrand wrote:
>>>>>>>
>>>>>>>> On 08.08.24 05:19, Baolin Wang wrote:
>>>>>>>>>
>>>>>>>>>
>>>> ...
>>>>>>>> Agreed, maybe we should simply handle that right away and replace the "goto out;" users by "return 0;".
>>>>>>>>
>>>>>>>> Then, just copy the 3 LOC.
>>>>>>>>
>>>>>>>> For mm/memory.c that would be:
>>>>>>>>
>>>>>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>>>>>> index 67496dc5064f..410ba50ca746 100644
>>>>>>>> --- a/mm/memory.c
>>>>>>>> +++ b/mm/memory.c
>>>>>>>> @@ -5461,7 +5461,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>>>>            if (unlikely(!pte_same(old_pte, vmf->orig_pte))) {
>>>>>>>>                   pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>>>>> -               goto out;
>>>>>>>> +               return 0;
>>>>>>>>           }
>>>>>>>>            pte = pte_modify(old_pte, vma->vm_page_prot);
>>>>>>>> @@ -5528,15 +5528,14 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>>>>>                   vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>>>>>>>>                                                  vmf->address, &vmf->ptl);
>>>>>>>>                   if (unlikely(!vmf->pte))
>>>>>>>> -                       goto out;
>>>>>>>> +                       return 0;
>>>>>>>>                   if (unlikely(!pte_same(ptep_get(vmf->pte), vmf->orig_pte))) {
>>>>>>>>                           pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>>>>> -                       goto out;
>>>>>>>> +                       return 0;
>>>>>>>>                   }
>>>>>>>>                   goto out_map;
>>>>>>>>           }
>>>>>>>>    -out:
>>>>>>>>           if (nid != NUMA_NO_NODE)
>>>>>>>>                   task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>>>>>>>           return 0;
>>>>
>>>> Maybe drop this part too,
>>>>
>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>> index 410ba50ca746..07343c1469e0 100644
>>>> --- a/mm/memory.c
>>>> +++ b/mm/memory.c
>>>> @@ -5523,6 +5523,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>         if (!migrate_misplaced_folio(folio, vma, target_nid)) {
>>>>                 nid = target_nid;
>>>>                 flags |= TNF_MIGRATED;
>>>> +               goto out;
>>>>         } else {
>>>>                 flags |= TNF_MIGRATE_FAIL;
>>>>                 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>>>> @@ -5533,12 +5534,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>                         pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>                         return 0;
>>>>                 }
>>>> -               goto out_map;
>>>>         }
>>>>
>>>> -       if (nid != NUMA_NO_NODE)
>>>> -               task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>>> -       return 0;
>>>>  out_map:
>>>>         /*
>>>>          * Make it present again, depending on how arch implements
>>>
>>> IMHO, migration success is normal path, while migration failure is error
>>> processing path.  If so, it's better to use "goto" for error processing
>>> instead of normal path.
>>>
>>>> @@ -5551,6 +5548,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>>>                 numa_rebuild_single_mapping(vmf, vma, vmf->address,
>>>>                 vmf->pte,
>>>>                                             writable);
>>>>         pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>> +out:
>>>>         if (nid != NUMA_NO_NODE)
>>>>                 task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>>>         return 0;
>>>>
>>>>
>>
>> How about calling task_numa_fault and return in the migration successful path?
>> (target_nid cannot be NUMA_NO_NODE, so if is not needed)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 3441f60d54ef..abdb73a68b80 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -5526,7 +5526,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>         if (!migrate_misplaced_folio(folio, vma, target_nid)) {
>>                 nid = target_nid;
>>                 flags |= TNF_MIGRATED;
>> -               goto out;
>> +               task_numa_fault(last_cpupid, nid, nr_pages, flags);
>> +               return 0;
>>         } else {
>>                 flags |= TNF_MIGRATE_FAIL;
>>                 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>> @@ -5550,7 +5551,6 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
>>                 numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
>>                                             writable);
>>         pte_unmap_unlock(vmf->pte, vmf->ptl);
>> -out:
>>         if (nid != NUMA_NO_NODE)
>>                 task_numa_fault(last_cpupid, nid, nr_pages, flags);
>>         return 0;
>>
>
> This looks better for me, or change it further.

Thanks. Will put a fixup below.

>
>        if (migrate_misplaced_folio(folio, vma, target_nid))
>                goto out_map_pt;
>
>        nid = target_nid;
>        flags |= TNF_MIGRATED;
>        task_numa_fault(last_cpupid, nid, nr_pages, flags);
>
>        return 0;
>
> out_map_pt:
>        flags |= TNF_MIGRATE_FAIL;
>        vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
>        ...
>

I will send a separate patch for this refactoring, since this patch
is meant for fixing commit b99a342d4f11 ("NUMA balancing: reduce TLB flush via delaying mapping on hint page fault”).


Hi Andrew,

Can you fold the fixup below to this patch? Thanks.


From 645fa83b2eed14494755ed67e5c52a55656287ac Mon Sep 17 00:00:00 2001
From: Zi Yan <ziy@nvidia.com>
Date: Thu, 8 Aug 2024 22:06:41 -0400
Subject: [PATCH] fixup! fixup! mm/numa: no task_numa_fault() call if page
 table is changed

Based on suggestion from Ying.

Link: https://lore.kernel.org/linux-mm/87cymizdvc.fsf@yhuang6-desk2.ccr.corp.intel.com/
Signed-off-by: Zi Yan <ziy@nvidia.com>
---
 mm/huge_memory.c | 5 +++--
 mm/memory.c      | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 4e4364a17e6d..0e79ccaaf5e4 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1724,7 +1724,8 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	if (!migrate_misplaced_folio(folio, vma, target_nid)) {
 		flags |= TNF_MIGRATED;
 		nid = target_nid;
-		goto out;
+		task_numa_fault(last_cpupid, nid, HPAGE_PMD_NR, flags);
+		return 0;
 	} else {
 		flags |= TNF_MIGRATE_FAIL;
 		vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd);
@@ -1743,7 +1744,7 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
 	set_pmd_at(vma->vm_mm, haddr, vmf->pmd, pmd);
 	update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
 	spin_unlock(vmf->ptl);
-out:
+
 	if (nid != NUMA_NO_NODE)
 		task_numa_fault(last_cpupid, nid, HPAGE_PMD_NR, flags);
 	return 0;
diff --git a/mm/memory.c b/mm/memory.c
index d9b1dff9dc57..6c3aadf3e5ad 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5523,7 +5523,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 	if (!migrate_misplaced_folio(folio, vma, target_nid)) {
 		nid = target_nid;
 		flags |= TNF_MIGRATED;
-		goto out;
+		task_numa_fault(last_cpupid, nid, nr_pages, flags);
+		return 0;
 	} else {
 		flags |= TNF_MIGRATE_FAIL;
 		vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
@@ -5547,7 +5548,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 		numa_rebuild_single_mapping(vmf, vma, vmf->address, vmf->pte,
 					    writable);
 	pte_unmap_unlock(vmf->pte, vmf->ptl);
-out:
+
 	if (nid != NUMA_NO_NODE)
 		task_numa_fault(last_cpupid, nid, nr_pages, flags);
 	return 0;
-- 
2.43.0




Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[David Hildenbrand]

> Hi Andrew,
> 
> Can you fold the fixup below to this patch? Thanks.

It might be reasonable to send out a new version. At least I am confused 
now what the latest state is :D

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] mm/numa: no task_numa_fault() call if page table is changed

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 330 bytes --]

On 9 Aug 2024, at 9:49, David Hildenbrand wrote:

>> Hi Andrew,
>>
>> Can you fold the fixup below to this patch? Thanks.
>
> It might be reasonable to send out a new version. At least I am confused now what the latest state is :D

Sure. I will send v3 to include all patches I sent recently.


Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]