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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

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)