Re: [PATCH v2 3/4] arm64/mm: Elide tlbi in contpte_convert() under BBML2

[David Hildenbrand <david@redhat.com>]

On 03.03.25 10:49, Mikołaj Lenczewski wrote:
> Hi David,
> 
> Thanks for taking the time to review.
> 
> On Mon, Mar 03, 2025 at 10:17:12AM +0100, David Hildenbrand wrote:
>> On 28.02.25 19:24, Mikołaj Lenczewski wrote:
>>> If we support bbml2 without conflict aborts, we can avoid the final
>>> flush and have hardware manage the tlb entries for us. Avoiding flushes
>>> is a win.
>>>
>>> Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
>>> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
>>> ---
>>>    arch/arm64/mm/contpte.c | 3 ---
>>>    1 file changed, 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
>>> index 145530f706a9..77ed03b30b72 100644
>>> --- a/arch/arm64/mm/contpte.c
>>> +++ b/arch/arm64/mm/contpte.c
>>> @@ -72,9 +72,6 @@ static void contpte_convert(struct mm_struct *mm, unsigned long addr,
>>>    		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
>>>    	__set_ptes(mm, start_addr, start_ptep, pte, CONT_PTES);
>>> -
>>> -	if (system_supports_bbml2_noabort())
>>> -		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
>>>    }
>>>    void __contpte_try_fold(struct mm_struct *mm, unsigned long addr,
>>
>> What's the point of not squashing this into #2? :)
>>
>> If this split was requested during earlier review, at least seeing patch #2
>> on its own confused me.
> 
> This split is a holdover from an earlier patchset, where it was still
> unknown whether the removal of the second flush was permitted with
> BBML2. Partly this was due to us being worried about conflict aborts
> after the removal, and partly this was because the "delay" is a separate
> optimisation that we could apply even if it turned out the final patch
> was not architecturally sound.
> 
> Now that we do not handle conflict aborts (preferring only systems that
> handle BBML2 without ever raising aborts), the first issue is not a
> problem. The reasoning behind the second patch is also a little bit
> outdated, but I can see the logical split between a tlbi reorder, and
> the removal of the tlbi. If this is truly redundant though, I would be
> happy to squash the two into a single patch.

Thanks for the information.

Does patch #2 (reordering the tlbi) have any benefit on its own? I read 
"other threads will not see an invalid pagetable entry", but I am not 
sure that is correct. A concurrent HW page table walker would still find 
the invalid PTE? It's just a matter of TLB state.

If there is no benefit in having patch #2 independently, I'd just squash 
them. Reordering to then remove is more complicated than just removing 
it IMHO.

-- 
Cheers,

David / dhildenb