Message-ID: <2bbe1e49-95ba-42ea-b6af-5eeb61d68c4c@kernel.org>
Date: Fri, 19 Dec 2025 15:13:29 +0100
From: "David Hildenbrand (Red Hat)" <david@...nel.org>
To: Harry Yoo <harry.yoo@...cle.com>
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 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.

One could now grab a reference to the page table to keep it alive even 
after unsharing it (decrementing the shared counter), no longer 
confusing shared vs. unshared handling.

But the basic problem of the new exclusive owner reusing the page table 
for something else is not really affected at all by that change. We must 
flush before the exclusive owner could reuse it ... and the shared vs. 
refcount split does not really help in that regard AFAIKS.

-- 
Cheers

David

Powered by blists - more mailing lists