Received-SPF: pass (google.com: domain of linux-kernel+bounces-51999-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: "Huang, Ying" <ying.huang@intel.com>
To: Peng Zhang <zhangpeng362@huawei.com>
Cc: <linux-mm@kvack.org>,  <linux-kernel@vger.kernel.org>,
  <akpm@linux-foundation.org>,  <willy@infradead.org>,
  <fengwei.yin@intel.com>,  <aneesh.kumar@linux.ibm.com>,
  <shy828301@gmail.com>,  <hughd@google.com>,  <david@redhat.com>,
  <wangkefeng.wang@huawei.com>
Subject: Re: [PATCH] filemap: avoid unnecessary major faults in filemap_fault()
In-Reply-To: <20240204093526.212636-1-zhangpeng362@huawei.com> (Peng Zhang's
	message of "Sun, 4 Feb 2024 17:35:26 +0800")
References: <20240204093526.212636-1-zhangpeng362@huawei.com>
Date: Mon, 05 Feb 2024 10:54:36 +0800
Message-ID: <874jen4o43.fsf@yhuang6-desk2.ccr.corp.intel.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=ascii

Peng Zhang <zhangpeng362@huawei.com> writes:

> From: ZhangPeng <zhangpeng362@huawei.com>
>
> The major fault occurred when using mlockall(MCL_CURRENT | MCL_FUTURE)
> in application, which leading to an unexpected performance issue[1].
>
> This caused by temporarily cleared PTE during a read/modify/write update
> of the PTE, eg, do_numa_page()/change_pte_range().
>
> For the data segment of the user-mode program, the global variable area
> is a private mapping. After the pagecache is loaded, the private anonymous
> page is generated after the COW is triggered. Mlockall can lock COW pages
> (anonymous pages), but the original file pages cannot be locked and may
> be reclaimed. If the global variable (private anon page) is accessed when
> vmf->pte is zeroed in numa fault, a file page fault will be triggered.
>
> At this time, the original private file page may have been reclaimed.
> If the page cache is not available at this time, a major fault will be
> triggered and the file will be read, causing additional overhead.
>
> Fix this by rechecking the PTE without acquiring PTL in filemap_fault()
> before triggering a major fault.
>
> Testing file anonymous page read and write page fault performance in ext4
> and ramdisk using will-it-scale[2] on a x86 physical machine. The data
> is the average change compared with the mainline after the patch is
> applied. The test results are within the range of fluctuation, and there
> is no obvious difference. The test results are as follows:
> 			processes processes_idle threads threads_idle
> ext4 file write:	-1.14%    -0.08%         -1.87%  0.13%
> ext4 file read:		 0.03%	  -0.65%         -0.51%	-0.08%
> ramdisk file write:	-1.21%    -0.21%         -1.12%  0.11%
> ramdisk file read:	 0.00%    -0.68%         -0.33% -0.02%

IIUC, this is the regression test results.  Right?  Can you also show
improvement test results to justify the change?

--
Best Regards,
Huang, Ying

> [1] https://lore.kernel.org/linux-mm/9e62fd9a-bee0-52bf-50a7-498fa17434ee@huawei.com/
> [2] https://github.com/antonblanchard/will-it-scale/
>
> Suggested-by: "Huang, Ying" <ying.huang@intel.com>
> Suggested-by: Yin Fengwei <fengwei.yin@intel.com>
> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
> RFC->v1:
> - Add error handling when ptep == NULL per Huang, Ying and Matthew Wilcox
> - Check the PTE without acquiring PTL in filemap_fault(), suggested by
>   Huang, Ying and Yin Fengwei
> - Add pmd_none() check before PTE map
> - Update commit message and add performance test information
>
>  mm/filemap.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 142864338ca4..b29cdeb6a03b 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3238,6 +3238,24 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  			mapping_locked = true;
>  		}
>  	} else {
> +		if (!pmd_none(*vmf->pmd)) {
> +			pte_t *ptep;
> +
> +			ptep = pte_offset_map_nolock(vmf->vma->vm_mm, vmf->pmd,
> +						     vmf->address, &vmf->ptl);
> +			if (unlikely(!ptep))
> +				return VM_FAULT_NOPAGE;
> +			/*
> +			 * Recheck pte as the pte can be cleared temporarily
> +			 * during a read/modify/write update.
> +			 */
> +			if (unlikely(!pte_none(ptep_get_lockless(ptep))))
> +				ret = VM_FAULT_NOPAGE;
> +			pte_unmap(ptep);
> +			if (unlikely(ret))
> +				return ret;
> +		}
> +
>  		/* No page in the page cache at all */
>  		count_vm_event(PGMAJFAULT);
>  		count_memcg_event_mm(vmf->vma->vm_mm, PGMAJFAULT);