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

From: Anshuman Khandual <anshuman.khandual@arm.com>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>,
	Steve Capper <steve.capper@arm.com>,
	David Hildenbrand <david@redhat.com>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	nd@arm.com
Subject: Re: VM_BUG_ON(!tlb->end) on munmap() with CONT hugetlb pages
Date: Wed, 11 May 2022 09:12:14 +0530	[thread overview]
Message-ID: <916dde5f-8ffd-95ca-0dfd-6777d2ef8498@arm.com> (raw)
In-Reply-To: <YnkFCwoW0TIHuQyR@arm.com>

On 5/9/22 17:41, Catalin Marinas wrote:
> 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.

FYI, posted here.

https://lore.kernel.org/all/20220510043930.2410985-1-anshuman.khandual@arm.com/