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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2E175CD11C2 for ; Fri, 5 Apr 2024 11:55:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7F1A76B015E; Fri, 5 Apr 2024 07:55:17 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 77AE96B015F; Fri, 5 Apr 2024 07:55:17 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 61AD86B0160; Fri, 5 Apr 2024 07:55:17 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 471046B015E for ; Fri, 5 Apr 2024 07:55:17 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id BD196A1156 for ; Fri, 5 Apr 2024 11:55:16 +0000 (UTC) X-FDA: 81975322632.21.EAA3166 Received: from mail-ed1-f44.google.com (mail-ed1-f44.google.com [209.85.208.44]) by imf21.hostedemail.com (Postfix) with ESMTP id DD1221C0012 for ; Fri, 5 Apr 2024 11:55:13 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ns1JF9h4; spf=pass (imf21.hostedemail.com: domain of ioworker0@gmail.com designates 209.85.208.44 as permitted sender) smtp.mailfrom=ioworker0@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1712318114; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=KA1tTLEF6tQjQUASzxOQDBDY7cW0W5LfoybYp/6l9r0=; b=WBED5vVtLomJuP6rCiF33RaVmLixX3ZJ0npAtwoir+1FvRdPSWwUDRmJHojXGdi2ONK6Bz Qsg9jQTYGZF0re4yOGhUZfB4eenxId0C/L7/7YHi/DMK45bn1pQUF04+CDUwnmuHm08idd zVRpOWQxyskWuQRyTWlqcl3yiQPsA5U= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1712318114; a=rsa-sha256; cv=none; b=rWdVfPR+c3Okr5Zf6bO2ZkyYxTGqq047eCIvFqmpF7ku2atsUpso5zW0TbhVORE8bu7ALE RtQpf1fpR9mGKPtK998Yx3ZFocmEKNzxupiVvQ4pxm8xnaWcK80WRixby9YB2sQHwFi86q sgYuDwyHWyZAf8mtGhTcRixUfQGxVzA= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ns1JF9h4; spf=pass (imf21.hostedemail.com: domain of ioworker0@gmail.com designates 209.85.208.44 as permitted sender) smtp.mailfrom=ioworker0@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-ed1-f44.google.com with SMTP id 4fb4d7f45d1cf-563cb3ba9daso2169288a12.3 for ; Fri, 05 Apr 2024 04:55:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1712318112; x=1712922912; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=KA1tTLEF6tQjQUASzxOQDBDY7cW0W5LfoybYp/6l9r0=; b=ns1JF9h4q8qV4+vqRQEhqg2hCqlHoo2KERRExLIBe8hNSdDmDGuzcjcMAVnAl8b22U LTTTY0N+C9rgCyAuYa2GOseNSZvtyKKFbohHDNVKd4Z/8MO0fcKfKKhzMzJcAZ1Ldclm MJUmO9UL1rq7RJ+JL7ekH/uDAA43TSdGn/DEg7cuZUwlckqGR3G8kKLS+ETbjo/FldAj RAyk6ZOAXXZgIXQLFAfCK8AUc/5g1YR0Ys13bM+Fcgv1eFCVuwdFMV+21em5bfXl5z2Q Vb0wAvtWObBwHNaSOgS1ZMk+NcmFuOAmySTCCtNs9I8U9/6MwkU2lsN93XpfTAX3ZLvL XM8Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712318112; x=1712922912; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=KA1tTLEF6tQjQUASzxOQDBDY7cW0W5LfoybYp/6l9r0=; b=A22kJbwsV+vKQ72jNO7IulXFiofWn5CSchrMjmWMQBQoNA4E4lntwI+4qqeJTipCxm NqAxtWquPGDxMGlbkdSW5WZi2wuAVPhmahGUJ3Kuc5w7eFsyAD9dz3YuX3NSCL39cBO1 vFCpGgPHpKeFP2L4407JEuqprzho7uiMlVKb0Bzp3ShACZ58aWncUxWATAPJI1cp7Fjc pjVM0WHCJCXddqbu5Mxv2bQ1LIzSIy5H7Efae91GviiwMkhrFvsZauA18AH37DXGtryW q7GI/CZAh3Xi/D7B17qiOX6QICeKPCodTm06zBN3bfsDfWyeUOdR0rjFuCslz7gsQ6sg 6uqQ== X-Forwarded-Encrypted: i=1; AJvYcCVZBvxdtyGgEBzkrYC9G2fIZmrrPHNTQZBM+VWU3HXowgszl7DdVA0e+dY42GX1qr+XLKKkGTsuzpqXiwbEAsqltc4= X-Gm-Message-State: AOJu0Yxf8NrclFzEOWJSvmvRgORojUcCaogKkdiX8QZYHia2EiVpWx3a tMXfK47Uv3Gopdcz1Gy8NjSBI4/IDJqbi9QysXF13PzJukLLJA3k6VqVXM9+Gd2BH0G9MA65UkZ +XtavWK5BrweCbi2vE1Yg3yD/uhU= X-Google-Smtp-Source: AGHT+IF0/Z8sAs1AunjxxmeS1dwOmoclZmhO6PSJoiptsld3avrd2FV5hIVLWoVH+Fz5/ydSIQHHhwoPKFj9xS4/USo= X-Received: by 2002:a50:bb4b:0:b0:56d:f246:bfa4 with SMTP id y69-20020a50bb4b000000b0056df246bfa4mr1180007ede.23.1712318111914; Fri, 05 Apr 2024 04:55:11 -0700 (PDT) MIME-Version: 1.0 References: <20240402124029.47846-1-ioworker0@gmail.com> <20240402124029.47846-2-ioworker0@gmail.com> <0793c3d9-589b-4550-a556-9a4fa5a6cec2@arm.com> In-Reply-To: <0793c3d9-589b-4550-a556-9a4fa5a6cec2@arm.com> From: Lance Yang Date: Fri, 5 Apr 2024 19:55:00 +0800 Message-ID: Subject: Re: [PATCH v4 1/2] mm/madvise: introduce mkold_clean_ptes() batch helper To: Ryan Roberts Cc: akpm@linux-foundation.org, zokeefe@google.com, 21cnbao@gmail.com, shy828301@gmail.com, david@redhat.com, mhocko@suse.com, fengwei.yin@intel.com, xiehuan09@gmail.com, wangkefeng.wang@huawei.com, songmuchun@bytedance.com, peterx@redhat.com, minchan@kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: a1thfmxod58s4aq6yuziz5akg1d1suaa X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: DD1221C0012 X-Rspam-User: X-HE-Tag: 1712318113-421133 X-HE-Meta: U2FsdGVkX1/PTbK2a1H7EE0ygttw65oYIUegFuCfN9ahhfj4mn3+0oDPIq+56H5UCNEhhBsZWpasOJnJyDeGci8o2AVZesw2zMZmOSNUw226ZQC7VPAA+Uodb+UjwkPb1tfICcvabVzNs2XEpu3ErRGELt8ndyvv8g+3G2K9zIukbAZOADrlMfD9HvanBPN5IXdT/+MS79d4OlAVJ6CYiJwz1N36RM6/V2hwpJUtj4JmpCrHK756AgvII9zSLSipSMbRMgsaV7H1crTuiJ94XcadFcbQMDe2OFzZXP6UHcMql9mMi/nIe+qFyuno5wXPl0eQokLgEYzLweCPSHSVDRWep1zDmtwdeSaKe0fP2l71GZwEb067OVimIw/xzJkO2zh/WsJGEWEy1gh9m4kITCWz7mdJU1q5AyR7fxkj3JoFV6jb2lyE7tIGANMcuI+We2LAb5DkDTp//hx5WUcQlxezy3U8REOE0/iE1/DamyzfNlyCX7fd02PxzdLnpZNEU1LTsZDlQO7kfPHEx98ca5dDoXaCiV/xFyfhTMoHuj1BAxVMRkC2xaJHWe6S32ErWa+eoBEUk4l9SfCUnS/qi3gdkjn9evmcpVsf/mnSojPkj4JvyfvAYWKbWtzb5Pnu54GK22QncCePDmT7RJAu42XNo0/l6cvcmEubejiLtxbTqtXwRPS2W+0a8r0hixaA+HeTZw8dADg2/6pOono4l2HgoBbDfv11RacsODQRWcosTIulE/qeXGvSfc/YfQ/OVlf3yKlmYl4kPWQR1Ang8XnnHhF47Eu0P8Qpfd+3Jv5E1DAYrkAfzmkpFU00N3NnE00czQm/LdprPcnzYvVGDOr8jIOX64gL3PEX7q8nQ/CEBw5CtEOzr1EYYzTELRWKNrbtRF8bRj6AHRShmwIjzN6/bYBx+Fh/Q6ve5xQIdH07z2VeNZe+v4RK4RQeCxln4wFfn8xe4FlvVp8lbwy jNyt2T2+ ZdS23EIuMMD+TYbMeLF/y8TVvNRGC+mMetW5BOJMPH0dYDS/ThLR6LValUGx5fBYX9R91E+FlQyUu/mdJ786n8G/Mv1Mu3Se6JI5+OGGvpmes2Ny4VgLYIOMBJfcjf07crsz5yEWO1YdiZl3mQKvTBMFc9KYqoFMvkvSRbPyFI7EEhipdNzUR+X+BhCCucav+9q8RGTF74Xuqxf60Y1m+A3xtl5x/rUTIGEhVqH1V2nvV5jtbidmi0UYCe2Wr1NenMCfaagjzG4RUwGgUHOIsOp2M3qPhNElP0WvsAS20H6QizO4/lA/7OD4+op+aKEFF0kjuCnNOvlLXsYwDp/yghRaD+KgPbXXgSWY6EPX2WI3r8wJqO98OZRkrgGrT6xjz4uT0jPE/8JD7saeoSwtgxPjrxFsShoxjOH79 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: Hey Ryan, Thanks for taking time to review! On Thu, Apr 4, 2024 at 12:38=E2=80=AFAM Ryan Roberts = wrote: > > On 02/04/2024 13:40, Lance Yang wrote: > > Change the code that clears young and dirty bits from the PTEs to use > > ptep_get_and_clear_full() and set_pte_at(), via the new mkold_clean_pte= s() > > batch helper function. > > > > Unfortunately, the per-pte get_and_clear/modify/set approach would resu= lt > > in unfolding/refolding for contpte mappings on arm64. So we need to > > override mkold_clean_ptes() for arm64 to avoid it. > > > > Suggested-by: David Hildenbrand > > Suggested-by: Barry Song <21cnbao@gmail.com> > > Suggested-by: Ryan Roberts > > Signed-off-by: Lance Yang > > --- > > arch/arm64/include/asm/pgtable.h | 36 ++++++++++++++++++++++++++++++++ > > arch/arm64/mm/contpte.c | 10 +++++++++ > > include/linux/pgtable.h | 30 ++++++++++++++++++++++++++ > > 3 files changed, 76 insertions(+) > > When I suggested splitting out this patch, this wasn't quite what I meant= . > Instead I intended for the first patch to make the MADV_FREE change and a= s part > of that introduce mkold_clean_ptes() with a generic implementation. Then = a > second patch would do the arm64-specific specialization of mkold_clean_pt= es() > for an arm64-specific performance boost. In general it is often useful to > separate core changes from arch changes where practical since they have > different target audiences. Thanks for clearing that up! I=E2=80=98ll make sure to implement it as disc= ussed. > > > > > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/= pgtable.h > > index 9fd8613b2db2..b032c107090c 100644 > > --- a/arch/arm64/include/asm/pgtable.h > > +++ b/arch/arm64/include/asm/pgtable.h > > @@ -1086,6 +1086,27 @@ static inline bool pud_user_accessible_page(pud_= t pud) > > } > > #endif > > > > +static inline void __mkold_clean_pte(struct vm_area_struct *vma, > > + unsigned long addr, pte_t *ptep) > > +{ > > + pte_t old_pte, pte; > > + > > + pte =3D __ptep_get(ptep); > > + do { > > + old_pte =3D pte; > > + pte =3D pte_mkclean(pte_mkold(pte)); > > + pte_val(pte) =3D cmpxchg_relaxed(&pte_val(*ptep), > > + pte_val(old_pte), pte_val(= pte)); > > + } while (pte_val(pte) !=3D pte_val(old_pte)); > > +} > > I'm not sure how value it is to split this out, given we are only exposin= g a > batched version to the core. Perhaps just include the body in mkold_clean= _ptes()? I agree that including the body in mkold_clean_ptes() would be better. > > > + > > +static inline void mkold_clean_ptes(struct vm_area_struct *vma, > > + unsigned long addr, pte_t *ptep, unsi= gned int nr, int full) > > +{ > > + for (; nr-- > 0; ptep++, addr +=3D PAGE_SIZE) > > + __mkold_clean_pte(vma, addr, ptep); > > +} > > + > > /* > > * Atomic pte/pmd modifications. > > */ > > @@ -1379,6 +1400,8 @@ extern void contpte_wrprotect_ptes(struct mm_stru= ct *mm, unsigned long addr, > > extern int contpte_ptep_set_access_flags(struct vm_area_struct *vma, > > unsigned long addr, pte_t *ptep, > > pte_t entry, int dirty); > > +extern void contpte_ptep_mkold_clean(struct vm_area_struct *vma, > > + unsigned long addr, pte_t *ptep); > > Let's have this follow the same naming pattern; contpte_mkold_clean_ptes(= ) Got it. Let=E2=80=99s keep the naming consistent with contpte_mkold_clean_p= tes(). > > > > > static __always_inline void contpte_try_fold(struct mm_struct *mm, > > unsigned long addr, pte_t *ptep, pte_t pt= e) > > @@ -1603,6 +1626,18 @@ static inline int ptep_set_access_flags(struct v= m_area_struct *vma, > > return contpte_ptep_set_access_flags(vma, addr, ptep, entry, dirt= y); > > } > > > > +#define mkold_clean_ptes mkold_clean_ptes > > +static inline void mkold_clean_ptes(struct vm_area_struct *vma, > > + unsigned long addr, pte_t *ptep, unsi= gned int nr, int full) > > +{ > > + pte_t orig_pte =3D __ptep_get(ptep); > > + > > + if (likely(!pte_valid_cont(orig_pte))) > > + return __mkold_clean_ptes(vma, addr, ptep, nr, full); > > + > > + return contpte_ptep_mkold_clean(vma, addr, ptep);> +} > > This function is totally broken as far as I can tell. You are assuming if= the > first pte is not cont, then the whole range is not cont; you can't assume= that. > Then if you decide it is cont, you ignore nr and only fixup a single cont= pte block. > > Take a look at wrprotect_ptes() or another one of the batched helpers and= follow > that pattern. Sorry for the mistake. I'll take a closer look at wrprotect_ptes() and make sure to follow that pattern. > > > + > > #else /* CONFIG_ARM64_CONTPTE */ > > > > #define ptep_get __ptep_get > > @@ -1622,6 +1657,7 @@ static inline int ptep_set_access_flags(struct vm= _area_struct *vma, > > #define wrprotect_ptes __wrprotect_ptes > > #define __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS > > #define ptep_set_access_flags __ptep_set_access= _flags > > +#define mkold_clean_ptes __mkold_clean_ptes > > > > #endif /* CONFIG_ARM64_CONTPTE */ > > > > diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c > > index 1b64b4c3f8bf..560622cfb2a9 100644 > > --- a/arch/arm64/mm/contpte.c > > +++ b/arch/arm64/mm/contpte.c > > @@ -322,6 +322,16 @@ int contpte_ptep_test_and_clear_young(struct vm_ar= ea_struct *vma, > > } > > EXPORT_SYMBOL_GPL(contpte_ptep_test_and_clear_young); > > > > +void contpte_ptep_mkold_clean(struct vm_area_struct *vma, unsigned lon= g addr, > > + pte_t *ptep) > > +{ > > + ptep =3D contpte_align_down(ptep); > > + addr =3D ALIGN_DOWN(addr, CONT_PTE_SIZE); > > + > > + __mkold_clean_ptes(vma, addr, ptep, CONT_PTES, 0); > > As above, this is broken as is. > > > +} > > +EXPORT_SYMBOL_GPL(contpte_ptep_mkold_clean); > > + > > int contpte_ptep_clear_flush_young(struct vm_area_struct *vma, > > unsigned long addr, pte_t *ptep) > > { > > diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h > > index fa8f92f6e2d7..fd30779fe487 100644 > > --- a/include/linux/pgtable.h > > +++ b/include/linux/pgtable.h > > @@ -391,6 +391,36 @@ static inline void mkold_ptes(struct vm_area_struc= t *vma, unsigned long addr, > > } > > #endif > > > > +#ifndef mkold_clean_ptes > > +/** > > + * mkold_clean_ptes - Mark PTEs that map consecutive pages of the same= folio > > + * as old and clean. > > + * @vma: VMA the pages are mapped into. > > + * @addr: Address the first page is mapped at. > > + * @ptep: Page table pointer for the first entry. > > + * @nr: Number of entries to mark old and clean. > > + * > > + * May be overridden by the architecture; otherwise, implemented as a = simple > > + * loop over ptep_get_and_clear_full(). > > How about "otherwise, implemented by get_and_clear/modify/set for each pt= e in > the range."? Nice, this is clearer and more accurate than before. > > > + * > > + * Note that PTE bits in the PTE range besides the PFN can differ. For= example, > > + * some PTEs might be write-protected. > > + * > > + * Context: The caller holds the page table lock. The PTEs map consec= utive > > + * pages that belong to the same folio. The PTEs are all in the same = PMD. > > + */ > > +static inline void mkold_clean_ptes(struct vm_area_struct *vma, > > + unsigned long addr, pte_t *ptep, unsi= gned int nr, int full) > > You've included the "full" param in the function but not in the docs. I d= on't > think the full param is valuable though; it doesn't make sense to call th= is > function when tearing down a process. So drop the parameter and just call > ptep_get_and_clear(). Got it. I'll drop the "full" parameter. > > > +{ > > + pte_t pte; > > + > > + for (; nr-- > 0; ptep++, addr +=3D PAGE_SIZE) { > > nit: This is using a different pattern to all the other batch helpers > (set_ptes(), wrprotect_ptes() clear_full_ptes()). Thanks for pointing that out. Thanks, Lance > > > + pte =3D ptep_get_and_clear_full(vma->vm_mm, addr, ptep, f= ull); > > + set_pte_at(vma->vm_mm, addr, ptep, pte_mkclean(pte_mkold(= pte))); > > + } > > +} > > +#endif > > + > > #ifndef __HAVE_ARCH_PMDP_TEST_AND_CLEAR_YOUNG > > #if defined(CONFIG_TRANSPARENT_HUGEPAGE) || defined(CONFIG_ARCH_HAS_NO= NLEAF_PMD_YOUNG) > > static inline int pmdp_test_and_clear_young(struct vm_area_struct *vma= , >