Re: VM_BUG_ON(!tlb->end) on munmap() with CONT hugetlb pages

[Anshuman Khandual <anshuman.khandual@xxxxxxx>]

On 5/6/22 18:19, Will Deacon wrote:
> On Wed, Mar 23, 2022 at 04:34:26PM +0000, Steve Capper wrote:
>>
>>
>> On 23/03/2022 16:21, Catalin Marinas wrote:
>>> On Wed, Mar 23, 2022 at 11:51:25AM +0000, Steve Capper wrote:
>>>> On 22/03/2022 17:56, Catalin Marinas wrote:
>>>>> At a quick look, we wouldn't have a problem with missing TLB flushing
>>>>> since huge_ptep_get_and_clear() does this for contiguous PTEs. Not sure
>>>>> why it needs this though, Steve added it in commit d8bdcff28764. I think
>>>>> we can defer this flushing to tlb_remove_page_size().
>>>>
>>>> The TLB flush in huge_ptep_get_and_clear() was added because it was called
>>>> by hugetlb_change_protection() without any flushing. The concern was that,
>>>> without the flush, it would be possible to get to different views of the
>>>> same contiguous huge page. (Being contiguous they were not changed en masse
>>>> atomically).
>>>
>>> Maybe the code paths have been changed since but looking at
>>> hugetlb_change_protection(), we have huge_ptep_modify_prot_start()
>>> calling huge_ptep_get_and_clear() which AFAICT only needs to clear the
>>> ptes. huge_ptep_modify_prot_commit() calls set_huge_pte_at() which does
>>> another pte clearing + TLBI (clear_flush()) before setting the new ptes.
>>> So we do the pte clearing and TLBI twice already.
>>>
>>
>> Thanks, yeah indeed the code has changed and the flush should be removed
>> from the arm64 huge_ptep_get_and_clear.
> 
> Did anybody send a patch for this?

Planning to send a patch which drops TLB flushing from get_clear_flush() and
also renames it as required. Something like this but just slightly tested.

diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index cbace1c9e137..acdaeb3b9356 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -166,7 +166,7 @@ static inline int num_contig_ptes(unsigned long size, size_t *pgsize)
  *
  * This helper performs the break step.
  */
-static pte_t get_clear_flush(struct mm_struct *mm,
+static pte_t get_clear_contig(struct mm_struct *mm,
                             unsigned long addr,
                             pte_t *ptep,
                             unsigned long pgsize,
@@ -190,11 +190,6 @@ static pte_t get_clear_flush(struct mm_struct *mm,
                if (pte_young(pte))
                        orig_pte = pte_mkyoung(orig_pte);
        }
-
-       if (valid) {
-               struct vm_area_struct vma = TLB_FLUSH_VMA(mm, 0);
-               flush_tlb_range(&vma, saddr, addr);
-       }
        return orig_pte;
 }
 
@@ -392,7 +387,7 @@ pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
 
        ncontig = find_num_contig(mm, addr, ptep, &pgsize);
 
-       return get_clear_flush(mm, addr, ptep, pgsize, ncontig);
+       return get_clear_contig(mm, addr, ptep, pgsize, ncontig);
 }
 
 /*
@@ -443,7 +438,7 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma,
        if (!__cont_access_flags_changed(ptep, pte, ncontig))
                return 0;
 
-       orig_pte = get_clear_flush(vma->vm_mm, addr, ptep, pgsize, ncontig);
+       orig_pte = get_clear_contig(vma->vm_mm, addr, ptep, pgsize, ncontig);
 
        /* Make sure we don't lose the dirty or young state */
        if (pte_dirty(orig_pte))
@@ -476,7 +471,7 @@ void huge_ptep_set_wrprotect(struct mm_struct *mm,
        ncontig = find_num_contig(mm, addr, ptep, &pgsize);
        dpfn = pgsize >> PAGE_SHIFT;
 
-       pte = get_clear_flush(mm, addr, ptep, pgsize, ncontig);
+       pte = get_clear_contig(mm, addr, ptep, pgsize, ncontig);
        pte = pte_wrprotect(pte);
 
        hugeprot = pte_pgprot(pte);