From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 78F73175A9E for ; Wed, 18 Mar 2026 16:49:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773852549; cv=none; b=kn5+CygssaQVcu+3JWLVFYArj6N9wztiZJEYZFAOMUdYq5ww1MIfPmcR4J18FEPRSsOD6dTujdgbfGoIsUVymerGOYqzWW2HZB7MI4PNNQCFH69WbnpgRK8sa9evR2L0ishb8rn2+rclac786UnPNNlZ+xNvKCkiT83bsYGzJ14= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773852549; c=relaxed/simple; bh=xsyjgsUcEFsoOWrMWXf6NXTE8aNyuqQ6bQfvEBebaMg=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=nzk9kP9/Q2KtBq64bMnYdI8247rizEeoRqL5CWwrkbziOt1oxTgzn/cEbzMnI5oy+/6ToADArZyMgnaIuS71N9wcpbzwlZizS5tihF0K9CxjVeLYVa++p2oVOksOYWdoZTx3MY2hv0j2m5LudZqnCbN/FUlBxcG74o3Ejp/X7lE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=YtYpcL+g; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=tU0tZZx3; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="YtYpcL+g"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="tU0tZZx3" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1773852546; h=from:from: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; bh=gRmS2ckI25IpIcwLZkK5otL0tcCQ/U40Yu29MTugS/8=; b=YtYpcL+gfJFmHq7FzO3mgCg2EAxDgJyRCCQY2VPh7/UVD/O5bjM7PzH8OCOWBGCM2MSBYx tNs5gSaJP7DUdwQYMFfbmY7E52G82h6ukBCZEw3CWIYRHsCSTT86wzPS92XvQqbFF4wphA 4Nv0ZcAukA+D1uX+KJDGM83/ocmgGq4= Received: from mail-qt1-f198.google.com (mail-qt1-f198.google.com [209.85.160.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-492-vjMRYb6AMBCSbj0wYeYw1A-1; Wed, 18 Mar 2026 12:49:05 -0400 X-MC-Unique: vjMRYb6AMBCSbj0wYeYw1A-1 X-Mimecast-MFC-AGG-ID: vjMRYb6AMBCSbj0wYeYw1A_1773852545 Received: by mail-qt1-f198.google.com with SMTP id d75a77b69052e-509070bda13so60852891cf.1 for ; Wed, 18 Mar 2026 09:49:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1773852544; x=1774457344; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:content-language:from :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=gRmS2ckI25IpIcwLZkK5otL0tcCQ/U40Yu29MTugS/8=; b=tU0tZZx3hZg3f8RVfNo3xbwatByPkxLF6vPII1XcUBXhgdxPmWzhFQDGDm/LsFYGTJ +5aVW4ASTurBNOx2C6FuVFvuyPpRj+077+DDOy29R0H6XUDO+InfWGMjbZxWdb9vyWSh UfjIagwGiJIY8c3Zsz5oHlB0YBzUGaaghJFsazpBYsvdbIzy4DJA94hfYMRQ2UPF0M3x 4VD0bVGsMKsiRrM/mvamvTxiS7J2kU/dlXa36KOMjtsz+YjGhLv/eLYqu/WzQBpWvSoT OQIZcpbbszoyyEdkGHQcSmZA2liLac3BS3JS1AdQu2GT6ZWpXeIbNqe1vaitWpmV+AzN Nv7A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1773852544; x=1774457344; h=content-transfer-encoding:in-reply-to:content-language:from :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=gRmS2ckI25IpIcwLZkK5otL0tcCQ/U40Yu29MTugS/8=; b=Cj/L8gE/HXVyQAafaTrFJx5XvAQNw5ui07sRVleSvbr5bP95IN+mKq0qzWNKlCcsBW WORuJJwqkQg2RyxTvTyWMHF45VTaFtu1RcKI5+Iv5pwmwdiTEy7e87i2Dq/E2xXD+Sgx Libd1iTO4KQ11SKsdagy1RqnqE05uGHA0bhoIaZwfb0nhpuY/tXeOu9KW636JP70Ql66 OhdagTel0JZn28TBzpCeZQ2UnlwZkvs0pwQKJcpY/jC7zgTmP8prgzxBS2bYhUfVW9Zn vOG9muP6sWqQwL81MN+0L/eMzo86m5WFYkTEJpslS3k//Dr5DMKA2VDzwb8K2cVdSPtE deNQ== X-Gm-Message-State: AOJu0YxL3Aa1UaoQRE2+c5GnhLnHU4vjr9sj26w6wL0jqjBbd4+H3hwt je6dRPSKYOafOJHU8H+MeKfWjNWfZxFa2UF/Qyn0ndHv1lnKq9LhjcuEuFzETsHpwBAe+D2w/7m S/ecSYlR6VnG9Ac3/MonnnTGc+kdafWD9sX2YrVW1XbC5hGqxZlHHNDnvZakggAJr0GjzJw4UcX Lg X-Gm-Gg: ATEYQzz3lSPsgG+ZVO/DSrlfLsXJL+GU1KSvlrsFBTYRQ9lHhoe+V52rQZV57qfLCno CtW0PmKJor/DLsKaX9JqEE0sDf2d5Gf915brp70NOQAMWvT4EH1E9TsnLDiZ+lEInTFKN4732jr ToDksh2dgcOutWZA12xAyVpIcTV0MRe7V6nFzFsY5bUbj+0flIRfiNx+8vr/QW5AtoA95ei0DhX 9XoAJR3ZmpF2qkWiV76hBXGsFmAyoXzlWD/5/24opPMJo9PT64G2C1mSeyB9RTDGFIXdac+UHxd 1NcXch7OZ2jUKFTJcielD0YdAfEeIl9hybS582A2gMqOqKrLVEX9XSYpsYvjeDxn1XsNiV8GiRY rX70g95UNM4OWf/fS02wNkw3z5N8k+UFVbLLrKBPZyEQLsqEZiCvngrdGYreD X-Received: by 2002:a05:622a:1895:b0:509:2135:ed59 with SMTP id d75a77b69052e-50b24793385mr3551641cf.36.1773852544050; Wed, 18 Mar 2026 09:49:04 -0700 (PDT) X-Received: by 2002:a05:622a:1895:b0:509:2135:ed59 with SMTP id d75a77b69052e-50b24793385mr3550861cf.36.1773852543285; Wed, 18 Mar 2026 09:49:03 -0700 (PDT) Received: from [192.168.10.111] (c-76-154-99-94.hsd1.co.comcast.net. [76.154.99.94]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-50b1348a5d0sm24584531cf.3.2026.03.18.09.48.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 18 Mar 2026 09:49:01 -0700 (PDT) Message-ID: <42341241-23dc-439e-b32b-da011743adce@redhat.com> Date: Wed, 18 Mar 2026 10:48:55 -0600 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH mm-unstable v3 1/5] mm: consolidate anonymous folio PTE mapping into helpers To: "Lorenzo Stoakes (Oracle)" Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, aarcange@redhat.com, akpm@linux-foundation.org, anshuman.khandual@arm.com, apopple@nvidia.com, baohua@kernel.org, baolin.wang@linux.alibaba.com, byungchul@sk.com, catalin.marinas@arm.com, cl@gentwo.org, corbet@lwn.net, dave.hansen@linux.intel.com, david@kernel.org, dev.jain@arm.com, gourry@gourry.net, hannes@cmpxchg.org, hughd@google.com, jackmanb@google.com, jack@suse.cz, jannh@google.com, jglisse@google.com, joshua.hahnjy@gmail.com, kas@kernel.org, lance.yang@linux.dev, Liam.Howlett@oracle.com, lorenzo.stoakes@oracle.com, mathieu.desnoyers@efficios.com, matthew.brost@intel.com, mhiramat@kernel.org, mhocko@suse.com, peterx@redhat.com, pfalcato@suse.de, rakie.kim@sk.com, raquini@redhat.com, rdunlap@infradead.org, richard.weiyang@gmail.com, rientjes@google.com, rostedt@goodmis.org, rppt@kernel.org, ryan.roberts@arm.com, shivankg@amd.com, sunnanyong@huawei.com, surenb@google.com, thomas.hellstrom@linux.intel.com, tiwai@suse.de, usamaarif642@gmail.com, vbabka@suse.cz, vishal.moola@gmail.com, wangkefeng.wang@huawei.com, will@kernel.org, willy@infradead.org, yang@os.amperecomputing.com, ying.huang@linux.alibaba.com, ziy@nvidia.com, zokeefe@google.com References: <20260311211315.450947-1-npache@redhat.com> <20260311211315.450947-2-npache@redhat.com> <5eae219d-62b8-4aea-905d-7a9b59b9892b@lucifer.local> From: Nico Pache Content-Language: en-US, en-ZM In-Reply-To: <5eae219d-62b8-4aea-905d-7a9b59b9892b@lucifer.local> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 3/16/26 12:17 PM, Lorenzo Stoakes (Oracle) wrote: > On Wed, Mar 11, 2026 at 03:13:11PM -0600, Nico Pache wrote: >> The anonymous page fault handler in do_anonymous_page() open-codes the >> sequence to map a newly allocated anonymous folio at the PTE level: >> - construct the PTE entry >> - add rmap >> - add to LRU >> - set the PTEs >> - update the MMU cache. > > Yikes yeah this all needs work. Thanks for looking at this! np! I believe it looks much cleaner now, and it has the added benefit of cleaning up some of the mTHP patchset. I also believe you suggested this so I'll add your SB when I add your RB tag :) > >> >> Introduce a two helpers to consolidate this duplicated logic, mirroring the > > NIT: 'Introduce a two helpers' -> "introduce two helpers' ack! > >> existing map_anon_folio_pmd_nopf() pattern for PMD-level mappings: >> >> map_anon_folio_pte_nopf(): constructs the PTE entry, takes folio >> references, adds anon rmap and LRU. This function also handles the >> uffd_wp that can occur in the pf variant. >> >> map_anon_folio_pte_pf(): extends the nopf variant to handle MM_ANONPAGES >> counter updates, and mTHP fault allocation statistics for the page fault >> path. > > MEGA nit, not sure why you're not just putting this in a bullet list, just > weird to see not-code indented here :) Ill drop the indentation! > >> >> The zero-page read path in do_anonymous_page() is also untangled from the >> shared setpte label, since it does not allocate a folio and should not >> share the same mapping sequence as the write path. Make nr_pages = 1 >> rather than relying on the variable. This makes it more clear that we >> are operating on the zero page only. >> >> This refactoring will also help reduce code duplication between mm/memory.c >> and mm/khugepaged.c, and provides a clean API for PTE-level anonymous folio >> mapping that can be reused by future callers. > > Maybe worth mentioning subseqent patches that will use what you set up > here? ok ill add something like "... that will be used when adding mTHP support to khugepaged, and may find future reuse by other callers." Speaking of which I tried to leverage this elsewhere and I believe that will take extra focus and time, but may be doable. > > Also you split things out into _nopf() and _pf() variants, it might be > worth saying exactly why you're doing that or what you are preparing to do? ok ill expand on this in the "bullet list" you reference above. > >> >> Reviewed-by: Dev Jain >> Reviewed-by: Lance Yang >> Acked-by: David Hildenbrand (Arm) >> Signed-off-by: Nico Pache > > There's nits above and below, but overall the logic looks good, so with > nits addressed/reasonably responded to: > > Reviewed-by: Lorenzo Stoakes (Oracle) Thanks :) Ill take care of those > >> --- >> include/linux/mm.h | 4 ++++ >> mm/memory.c | 60 +++++++++++++++++++++++++++++++--------------- >> 2 files changed, 45 insertions(+), 19 deletions(-) >> >> diff --git a/include/linux/mm.h b/include/linux/mm.h >> index 4c4fd55fc823..9fea354bd17f 100644 >> --- a/include/linux/mm.h >> +++ b/include/linux/mm.h >> @@ -4903,4 +4903,8 @@ static inline bool snapshot_page_is_faithful(const struct page_snapshot *ps) >> >> void snapshot_page(struct page_snapshot *ps, const struct page *page); >> >> +void map_anon_folio_pte_nopf(struct folio *folio, pte_t *pte, >> + struct vm_area_struct *vma, unsigned long addr, >> + bool uffd_wp); > > How I hate how uffd infiltrates all our code like this. > > Not your fault :) > >> + >> #endif /* _LINUX_MM_H */ >> diff --git a/mm/memory.c b/mm/memory.c >> index 6aa0ea4af1fc..5c8bf1eb55f5 100644 >> --- a/mm/memory.c >> +++ b/mm/memory.c >> @@ -5197,6 +5197,37 @@ static struct folio *alloc_anon_folio(struct vm_fault *vmf) >> return folio_prealloc(vma->vm_mm, vma, vmf->address, true); >> } >> >> +void map_anon_folio_pte_nopf(struct folio *folio, pte_t *pte, >> + struct vm_area_struct *vma, unsigned long addr, >> + bool uffd_wp) >> +{ >> + unsigned int nr_pages = folio_nr_pages(folio); > > const would be good > >> + pte_t entry = folio_mk_pte(folio, vma->vm_page_prot); >> + >> + entry = pte_sw_mkyoung(entry); >> + >> + if (vma->vm_flags & VM_WRITE) >> + entry = pte_mkwrite(pte_mkdirty(entry), vma); >> + if (uffd_wp) >> + entry = pte_mkuffd_wp(entry); >> + >> + folio_ref_add(folio, nr_pages - 1); >> + folio_add_new_anon_rmap(folio, vma, addr, RMAP_EXCLUSIVE); >> + folio_add_lru_vma(folio, vma); >> + set_ptes(vma->vm_mm, addr, pte, entry, nr_pages); >> + update_mmu_cache_range(NULL, vma, addr, pte, nr_pages); >> +} >> + >> +static void map_anon_folio_pte_pf(struct folio *folio, pte_t *pte, >> + struct vm_area_struct *vma, unsigned long addr, bool uffd_wp) >> +{ >> + unsigned int order = folio_order(folio); > > const would be good here also! ack on the const's > >> + >> + map_anon_folio_pte_nopf(folio, pte, vma, addr, uffd_wp); >> + add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1 << order); > > Is 1 << order strictly right here? This field is a long value, so 1L << > order maybe? I get nervous about these shifts... > > Note that folio_large_nr_pages() uses 1L << order so that does seem > preferable. Ok sounds good thanks! > >> + count_mthp_stat(order, MTHP_STAT_ANON_FAULT_ALLOC); >> +} >> + >> /* >> * We enter with non-exclusive mmap_lock (to exclude vma changes, >> * but allow concurrent faults), and pte mapped but not yet locked. >> @@ -5243,7 +5274,14 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf) >> pte_unmap_unlock(vmf->pte, vmf->ptl); >> return handle_userfault(vmf, VM_UFFD_MISSING); >> } >> - goto setpte; >> + if (vmf_orig_pte_uffd_wp(vmf)) >> + entry = pte_mkuffd_wp(entry); >> + set_pte_at(vma->vm_mm, addr, vmf->pte, entry); > > How I _despise_ how uffd is implemented in mm. Feels like open coded > nonsense spills out everywhere. > > Not your fault obviously :) > >> + >> + /* No need to invalidate - it was non-present before */ >> + update_mmu_cache_range(vmf, vma, addr, vmf->pte, >> + /*nr_pages=*/ 1); > > Is there any point in passing vmf here given you pass NULL above, and it > appears that nobody actually uses this? I guess it doesn't matter but > seeing this immediately made we question why you set it in one, and not the > other? > > Maybe I'm mistaken and some arch uses it? Don't think so though. > > Also can't we then just use update_mmu_cache() which is the single-page > wrapper of this AFAICT? That'd make it even simpler. > > Having done this, is there any reason to keep the annoying and confusing > initial assignment of nr_pages = 1 at declaration time? > > It seems that nr_pages is unconditionally assigned before it's used > anywhere now at line 5298: > > nr_pages = folio_nr_pages(folio); > addr = ALIGN_DOWN(vmf->address, nr_pages * PAGE_SIZE); > ... > > It's kinda weird to use nr_pages again after you go out of your way to > avoid it using folio_nr_pages() in map_anon_folio_pte_nopf() and > folio_order() in map_anon_folio_pte_pf(). Yes I believe so, thank you that is cleaner! In the past I had nr_pages as a variable but David suggested being explicit with the "1" so it would be obvious its a single page... update_mmu_cache() should indicate the same thing. > > But yeah, ok we align the address and it's yucky maybe leave for now (but > we can definitely stop defaulting nr_pages to 1 :) > >> + goto unlock; >> } >> >> /* Allocate our own private page. */ >> @@ -5267,11 +5305,6 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf) >> */ >> __folio_mark_uptodate(folio); >> >> - entry = folio_mk_pte(folio, vma->vm_page_prot); >> - entry = pte_sw_mkyoung(entry); >> - if (vma->vm_flags & VM_WRITE) >> - entry = pte_mkwrite(pte_mkdirty(entry), vma); >> - >> vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, addr, &vmf->ptl); >> if (!vmf->pte) >> goto release; >> @@ -5293,19 +5326,8 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf) >> folio_put(folio); >> return handle_userfault(vmf, VM_UFFD_MISSING); >> } >> - >> - folio_ref_add(folio, nr_pages - 1); >> - add_mm_counter(vma->vm_mm, MM_ANONPAGES, nr_pages); >> - count_mthp_stat(folio_order(folio), MTHP_STAT_ANON_FAULT_ALLOC); >> - folio_add_new_anon_rmap(folio, vma, addr, RMAP_EXCLUSIVE); >> - folio_add_lru_vma(folio, vma); >> -setpte: >> - if (vmf_orig_pte_uffd_wp(vmf)) >> - entry = pte_mkuffd_wp(entry); >> - set_ptes(vma->vm_mm, addr, vmf->pte, entry, nr_pages); >> - >> - /* No need to invalidate - it was non-present before */ >> - update_mmu_cache_range(vmf, vma, addr, vmf->pte, nr_pages); >> + map_anon_folio_pte_pf(folio, vmf->pte, vma, addr, >> + vmf_orig_pte_uffd_wp(vmf)); > > So we're going from: > > entry = folio_mk_pte(...) > entry = pte_sw_mkyoung(...) > if (write) > entry = pte_mkwrite(also dirty...) > folio_ref_add(nr_pages - 1) > add_mm_counter(... MM_ANON_PAGES, nr_pages) > count_mthp_stat(folio_order(folio), MTHP_STAT_ANON_FAULT_ALLOC) > folio_add_new_anon_rmap(.., RMAP_EXCLUSIVE) > folio_add_lru_vma(folio, vma) > if (vmf_orig_pte_uffd_wp(vmf)) > entry = pte_mkuffd_wp(entry) > set_ptes(mm, addr, vmf->pte, entry, nr_pages) > update_mmu_cache_range(vmf, vma, addr, vmf->pte, nr_pages) > > To: > > > entry = folio_mk_pte(...) > entry = pte_sw_mkyoung(...) > if (write) > entry = pte_mkwrite(also dirty...) > if (vmf_orig_pte_uffd_wp(vmf) ) <-- reordered > entry = pte_mkuffd_wp(entry) > folio_ref_add(nr_pages - 1) > folio_add_new_anon_rmap(.., RMAP_EXCLUSIVE) > folio_add_lru_vma(folio, vma) > set_ptes(mm, addr, pte, entry, nr_pages) > update_mmu_cache_range(NULL, vma, addr, pte, nr_pages) > > > add_mm_counter(... MM_ANON_PAGES, nr_pages) <-- reordered > count_mthp_stat(folio_order(folio), MTHP_STAT_ANON_FAULT_ALLOC) <-- reodrdered > > But the reorderings seem fine, and it is achieving the same thing. > > All the parameters being passed seem correct too. Thanks for the review and verifying the logic :) I was particularly scared of changing the page fault handler, so im glad multiple people have confirmed this seems fine. Cheers, -- Nico > >> unlock: >> if (vmf->pte) >> pte_unmap_unlock(vmf->pte, vmf->ptl); >> -- >> 2.53.0 >> > > Cheers, Lorenzo >