Re: [PATCH v2 2/4] mm/hugetlb: fix two comments related to huge_pmd_unshare()

[Nadav Amit <nadav.amit@gmail.com>]

> On 19 Dec 2025, at 16:13, David Hildenbrand (Red Hat) <david@kernel.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@surriel.com>
>>>>> Tested-by: Laurence Oberman <loberman@redhat.com>
>>>>> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>>>>> Acked-by: Oscar Salvador <osalvador@suse.de>
>>>>> Cc: Liu Shixin <liushixin2@huawei.com>
>>>>> Signed-off-by: David Hildenbrand (Red Hat) <david@kernel.org>
>>>>> ---
>>>> 
>>>> Looks good to me,
>>>> Reviewed-by: Harry Yoo <harry.yoo@oracle.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.