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

[David Hildenbrand]

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@xxxxxxx>
> > > Reviewed-by: Ryan Roberts <ryan.roberts@xxxxxxx>
> > > ---
> > >    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