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

[Catalin Marinas <catalin.marinas@arm.com>]

On Mon, May 09, 2022 at 04:49:25PM +0530, Anshuman Khandual wrote:
> 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.

The diff looks fine to me. I think this only works if we also have
commit 697a1d44af8b ("tlb: hugetlb: Add more sizes to
tlb_remove_huge_tlb_entry"), otherwise we risk missing some TLBIs on the
unmap path.

-- 
Catalin