From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7BA1FC61DF4 for ; Fri, 24 Nov 2023 04:28:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231176AbjKXE2h (ORCPT ); Thu, 23 Nov 2023 23:28:37 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48628 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229453AbjKXE2e (ORCPT ); Thu, 23 Nov 2023 23:28:34 -0500 Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.120]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A86291BE for ; Thu, 23 Nov 2023 20:28:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1700800120; x=1732336120; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version:content-transfer-encoding; bh=J4Ile0Qosg4PQ4W33TLsXAlMkveaq9qRrr4ZS5TQ5es=; b=gEbeAinWtbqgOe2WzAGsCu9uU62+ovPv7CN+HxxyqPrNNgXIvWfgAzII 8IEr+yeSeOPPKLzlP/Rgccxf0oBcbCAgyjlVIeU0Ys93cBg1FiBtmUot+ zv7G3NK835LDqpVpmoYEjG71cBm40skwH+j59PBifFvVStq0vWz+lAAvx cGdkuqtj+1agdyxr/hUCgxlA3HrVa5TEMJ9384w+TjkUqQePFQllg+8j4 z58BUkQv0JPJJmdwVowAIO52Qd+KYnhZZWkH7mgTVe1gjkX8V3VCAQSlm I2LPBoKE8YRzvpK0lLGnAfj8L2ALup+6wz5+tDzhh/I2CmOPSA6ozp8qt Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10902"; a="391238928" X-IronPort-AV: E=Sophos;i="6.04,223,1695711600"; d="scan'208";a="391238928" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Nov 2023 20:28:40 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10902"; a="1098976827" X-IronPort-AV: E=Sophos;i="6.04,223,1695711600"; d="scan'208";a="1098976827" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Nov 2023 20:28:37 -0800 From: "Huang, Ying" To: "zhangpeng (AS)" Cc: Yin Fengwei , , , , , , , , , , Subject: Re: [RFC PATCH] mm: filemap: avoid unnecessary major faults in filemap_fault() In-Reply-To: <87y1en7pq3.fsf@yhuang6-desk2.ccr.corp.intel.com> (Ying Huang's message of "Fri, 24 Nov 2023 12:13:56 +0800") References: <20231122140052.4092083-1-zhangpeng362@huawei.com> <801bd0c9-7d0c-4231-93e5-7532e8231756@intel.com> <48235d73-3dc6-263d-7822-6d479b753d46@huawei.com> <87y1en7pq3.fsf@yhuang6-desk2.ccr.corp.intel.com> Date: Fri, 24 Nov 2023 12:26:36 +0800 Message-ID: <87ttpb7p4z.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org "Huang, Ying" writes: > "zhangpeng (AS)" writes: > >> On 2023/11/23 13:26, Yin Fengwei wrote: >> >>> On 11/23/23 12:12, zhangpeng (AS) wrote: >>>> On 2023/11/23 9:09, Yin Fengwei wrote: >>>> >>>>> Hi Peng, >>>>> >>>>> On 11/22/23 22:00, Peng Zhang wrote: >>>>>> From: ZhangPeng >>>>>> >>>>>> The major fault occurred when using mlockall(MCL_CURRENT | MCL_FUTUR= E) >>>>>> in application, which leading to an unexpected performance issue[1]. >>>>>> >>>>>> This caused by temporarily cleared pte during a read/modify/write up= date >>>>>> of the pte, eg, do_numa_page()/change_pte_range(). >>>>>> >>>>>> For the data segment of the user-mode program, the global variable a= rea >>>>>> is a private mapping. After the pagecache is loaded, the private ano= nymous >>>>>> 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 triggere= d. >>>>>> >>>>>> 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 by holding ptl in filemap_fault() bef= ore >>>>>> triggering a major fault. >>>>>> >>>>>> [1] https://lore.kernel.org/linux-mm/9e62fd9a-bee0-52bf-50a7-498fa17= 434ee@huawei.com/ >>>>>> >>>>>> Signed-off-by: ZhangPeng >>>>>> Signed-off-by: Kefeng Wang >>>>>> --- >>>>>> =C2=A0 mm/filemap.c | 14 ++++++++++++++ >>>>>> =C2=A0 1 file changed, 14 insertions(+) >>>>>> >>>>>> diff --git a/mm/filemap.c b/mm/filemap.c >>>>>> index 71f00539ac00..bb5e6a2790dc 100644 >>>>>> --- a/mm/filemap.c >>>>>> +++ b/mm/filemap.c >>>>>> @@ -3226,6 +3226,20 @@ vm_fault_t filemap_fault(struct vm_fault *vmf) >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 mapping_locked =3D true; >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } else { >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pte_t *ptep =3D pte_offs= et_map_lock(vmf->vma->vm_mm, vmf->pmd, >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 vmf->address, &vmf->ptl); >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (ptep) { >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = /* >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 * Recheck pte with ptl locked as the pte can be cleared >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 * temporarily during a read/modify/write update. >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 */ >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = if (unlikely(!pte_none(ptep_get(ptep)))) >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 ret =3D VM_FAULT_NOPAGE; >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = pte_unmap_unlock(ptep, vmf->ptl); >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = if (unlikely(ret)) >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 return ret; >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >>>>> I am curious. Did you try not to take PTL here and just check whether= PTE is not NONE? >>>> Thank you for your reply. >>>> >>>> If we don't take PTL, the current use case won't trigger this issue ei= ther. >>> Is this verified by testing or just in theory? >> >> If we add a delay between ptep_modify_prot_start() and ptep_modify_prot_= commit(), >> this issue will also trigger. Without delay, we haven't reproduced this = problem >> so far. >> >>>> In most cases, if we don't take PTL, this issue won't be triggered. Ho= wever, >>>> there is still a possibility of triggering this issue. The corner case= is that >>>> task 2 triggers a page fault when task 1 is between ptep_modify_prot_s= tart() >>>> and ptep_modify_prot_commit() in do_numa_page(). Furthermore,task 2 pa= sses the >>>> check whether the PTE is not NONE before task 1 updates PTE in >>>> ptep_modify_prot_commit() without taking PTL. >>> There is very limited operations between ptep_modify_prot_start() and >>> ptep_modify_prot_commit(). While the code path from page fault to this = check is >>> long. My understanding is it's very likely the PTE is not NONE when do = PTE check >>> here without hold PTL (This is my theory. :)). >> >> Yes, there is a high probability that this issue won't occur without tak= ing PTL. >> >>> In the other side, acquiring/releasing PTL may bring performance impact= ion. It may >>> not be big deal because the IO operations in this code path. But it's b= etter to >>> collect some performance data IMHO. >> >> We tested the performance of file private mapping page fault (page_fault= 2.c of >> will-it-scale [1]) and file shared mapping page fault (page_fault3.c of = will-it-scale). >> The difference in performance (in operations per second) before and afte= r patch >> applied is about 0.7% on a x86 physical machine. > > Whether is it improvement or reduction? And I think that you need to test ramdisk cases too to verify whether this will cause performance regression and how much. -- Best Regards, Huang, Ying > -- > Best Regards, > Huang, Ying > >> [1] https://github.com/antonblanchard/will-it-scale/tree/master >> >>> >>> Regards >>> Yin, Fengwei >>> >>>>> Regards >>>>> Yin, Fengwei >>>>> >>>>>> + >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* No page i= n the page cache at all */ >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 count_vm_eve= nt(PGMAJFAULT); >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 count_memcg_= event_mm(vmf->vma->vm_mm, PGMAJFAULT);