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] [day] [month] [year] [list]
Message-Id: <3D7F0E85-2509-4925-BDD5-149E9E8D5C1F@gmail.com>
Date: Fri, 19 Dec 2025 23:37:50 +0200
From: Nadav Amit <nadav.amit@...il.com>
To: "David Hildenbrand (Red Hat)" <david@...nel.org>
Cc: Harry Yoo <harry.yoo@...cle.com>,
 Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
 linux-arch@...r.kernel.org,
 "open list:MEMORY MANAGEMENT" <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>,
 Liu Shixin <liushixin2@...wei.com>
Subject: Re: [PATCH v2 2/4] mm/hugetlb: fix two comments related to
 huge_pmd_unshare()



> On 19 Dec 2025, at 16:13, David Hildenbrand (Red Hat) <david@...nel.org> wrote:
> 
> On 12/19/25 12:20, Harry Yoo wrote:
>> 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.
> 
> Yeah, I am also confused by the old comment. I think the idea there was to drop the reference only later and thereby deferred-free the page.

My bad. I looked again, and the comment indeed doesn’t make much sense. Thanks for fixing it.