linux-kernel - Re: [PATCH v2 2/4] mm/hugetlb: fix two comments related to huge_pmd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aUU1DY4206FibsLf@hyeyoo>
Date: Fri, 19 Dec 2025 20:20:45 +0900
From: Harry Yoo <harry.yoo@...cle.com>
To: "David Hildenbrand (Red Hat)" <david@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        linux-mm@...ck.org, Will Deacon <will@...nel.org>,
        "Aneesh Kumar K.V" <aneesh.kumar@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Nick Piggin <npiggin@...il.com>, Peter Zijlstra <peterz@...radead.org>,
        Arnd Bergmann <arnd@...db.de>, Muchun Song <muchun.song@...ux.dev>,
        Oscar Salvador <osalvador@...e.de>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        Pedro Falcato <pfalcato@...e.de>, Rik van Riel <riel@...riel.com>,
        Laurence Oberman <loberman@...hat.com>,
        Prakash Sangappa <prakash.sangappa@...cle.com>,
        Nadav Amit <nadav.amit@...il.com>, Liu Shixin <liushixin2@...wei.com>
Subject: Re: [PATCH v2 2/4] mm/hugetlb: fix two comments related to
 huge_pmd_unshare()

On Fri, Dec 19, 2025 at 07:11:00AM +0100, David Hildenbrand (Red Hat) wrote:
> On 12/19/25 05:44, Harry Yoo wrote:
> > On Fri, Dec 12, 2025 at 08:10:17AM +0100, David Hildenbrand (Red Hat) wrote:
> > > Ever since we stopped using the page count to detect shared PMD
> > > page tables, these comments are outdated.
> > > 
> > > The only reason we have to flush the TLB early is because once we drop
> > > the i_mmap_rwsem, the previously shared page table could get freed (to
> > > then get reallocated and used for other purpose). So we really have to
> > > flush the TLB before that could happen.
> > > 
> > > So let's simplify the comments a bit.
> > > 
> > > The "If we unshared PMDs, the TLB flush was not recorded in mmu_gather."
> > > part introduced as in commit a4a118f2eead ("hugetlbfs: flush TLBs
> > > correctly after huge_pmd_unshare") was confusing: sure it is recorded
> > > in the mmu_gather, otherwise tlb_flush_mmu_tlbonly() wouldn't do
> > > anything. So let's drop that comment while at it as well.
> > > 
> > > We'll centralize these comments in a single helper as we rework the code
> > > next.
> > > 
> > > Fixes: 59d9094df3d7 ("mm: hugetlb: independent PMD page table shared count")
> > > Reviewed-by: Rik van Riel <riel@...riel.com>
> > > Tested-by: Laurence Oberman <loberman@...hat.com>
> > > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > > Acked-by: Oscar Salvador <osalvador@...e.de>
> > > Cc: Liu Shixin <liushixin2@...wei.com>
> > > Signed-off-by: David Hildenbrand (Red Hat) <david@...nel.org>
> > > ---
> > 
> > Looks good to me,
> > Reviewed-by: Harry Yoo <harry.yoo@...cle.com>
> > 
> > with a question below.
> 
> Hi Harry,
> 
> thanks for the review!

No problem!
I would love to review more, as long as my time & ability allows ;)

> > >   mm/hugetlb.c | 24 ++++++++----------------
> > >   1 file changed, 8 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > > index 51273baec9e5d..3c77cdef12a32 100644
> > > --- a/mm/hugetlb.c
> > > +++ b/mm/hugetlb.c
> > > @@ -5304,17 +5304,10 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
> > >   	tlb_end_vma(tlb, vma);
> > >   	/*
> > > -	 * If we unshared PMDs, the TLB flush was not recorded in mmu_gather. We
> > > -	 * could defer the flush until now, since by holding i_mmap_rwsem we
> > > -	 * guaranteed that the last reference would not be dropped. But we must
> > > -	 * do the flushing before we return, as otherwise i_mmap_rwsem will be
> > > -	 * dropped and the last reference to the shared PMDs page might be
> > > -	 * dropped as well.
> > > -	 *
> > > -	 * In theory we could defer the freeing of the PMD pages as well, but
> > > -	 * huge_pmd_unshare() relies on the exact page_count for the PMD page to
> > > -	 * detect sharing, so we cannot defer the release of the page either.
> > > -	 * Instead, do flush now.
> > 
> > Does this mean we can now try defer-freeing of these page tables,
> > and if so, would it be worth it?
> 
> There is one very tricky thing:
> 
> Whoever is the last owner of a (previously) shared page table must unmap any
> contained pages (adjust mapcount/ref, sync a/d bit, ...).

Right.

> So it's not just a matter of deferring the freeing, because these page tables
> will still contain content.

I was (and maybe still) bit confused while reading the old comment as
it implied (or maybe I just misread) that by deferring freeing of page tables
we don't have to flush TLB in __unmap_hugepage_range() and can flush later
instead.

> I first tried to never allow for reuse of shared page tables, but precisely
> that resulted in most headakes.

I see your pain there...

> So I don't see an easy way to achieve that (and I'm also not sure if we want
> to add any further complexity to this).

Fair enough.

Thanks for answering!

> -- 
> Cheers
> 
> David

-- 
Cheers,
Harry / Hyeonggon