Re: [PATCH v1] mm: Remove arch_flush_tlb_batched_pending() arch helper

[Ryan Roberts <ryan.roberts@arm.com>]

On 09/06/2025 11:45, Lorenzo Stoakes wrote:
> On Mon, Jun 09, 2025 at 11:31:30AM +0100, Ryan Roberts wrote:
>> Since commit 4b634918384c ("arm64/mm: Close theoretical race where stale
>> TLB entry remains valid"), all arches that use tlbbatch for reclaim
>> (arm64, riscv, x86) implement arch_flush_tlb_batched_pending() with a
>> flush_tlb_mm().
>>
>> So let's simplify by removing the unnecessary abstraction and doing the
>> flush_tlb_mm() directly in flush_tlb_batched_pending(). This effectively
>> reverts commit db6c1f6f236d ("mm/tlbbatch: introduce
>> arch_flush_tlb_batched_pending()").
>>
>> Suggested-by: Will Deacon <will@kernel.org>
>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> 
> Thanks, love to see an arch_*() helper go :)
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Thanks!

> 
> Couple points below.
> 
>> ---
>>  arch/arm64/include/asm/tlbflush.h | 11 -----------
>>  arch/riscv/include/asm/tlbflush.h |  1 -
>>  arch/riscv/mm/tlbflush.c          |  5 -----
>>  arch/x86/include/asm/tlbflush.h   |  5 -----
>>  mm/rmap.c                         |  2 +-
>>  5 files changed, 1 insertion(+), 23 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h
>> index aa9efee17277..18a5dc0c9a54 100644
>> --- a/arch/arm64/include/asm/tlbflush.h
>> +++ b/arch/arm64/include/asm/tlbflush.h
>> @@ -322,17 +322,6 @@ static inline bool arch_tlbbatch_should_defer(struct mm_struct *mm)
>>  	return true;
>>  }
>>
>> -/*
>> - * If mprotect/munmap/etc occurs during TLB batched flushing, we need to ensure
>> - * all the previously issued TLBIs targeting mm have completed. But since we
>> - * can be executing on a remote CPU, a DSB cannot guarantee this like it can
>> - * for arch_tlbbatch_flush(). Our only option is to flush the entire mm.
>> - */
> 
> Hm are we losing information here? I guess it's hard to know whewre to put
> this though.

The generic version of this comment exists above flush_tlb_batched_pending() in
rmap.c.

For the arm64-specific description of why we need to flush the whole mm, that's
captured in Commit 4b634918384c ("arm64/mm: Close theoretical race where stale
TLB entry remains valid"), although I accept that may not be the first place
someone looks.

I don't think we should be defining arch_ helpers just to provide a hook for
some arch-specific comments though.

> 
>> -static inline void arch_flush_tlb_batched_pending(struct mm_struct *mm)
>> -{
>> -	flush_tlb_mm(mm);
>> -}
>> -
>>  /*
>>   * To support TLB batched flush for multiple pages unmapping, we only send
>>   * the TLBI for each page in arch_tlbbatch_add_pending() and wait for the
>> diff --git a/arch/riscv/include/asm/tlbflush.h b/arch/riscv/include/asm/tlbflush.h
>> index 1a20dd746a49..eed0abc40514 100644
>> --- a/arch/riscv/include/asm/tlbflush.h
>> +++ b/arch/riscv/include/asm/tlbflush.h
>> @@ -63,7 +63,6 @@ void flush_pud_tlb_range(struct vm_area_struct *vma, unsigned long start,
>>  bool arch_tlbbatch_should_defer(struct mm_struct *mm);
>>  void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *batch,
>>  		struct mm_struct *mm, unsigned long start, unsigned long end);
>> -void arch_flush_tlb_batched_pending(struct mm_struct *mm);
>>  void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch);
>>
>>  extern unsigned long tlb_flush_all_threshold;
>> diff --git a/arch/riscv/mm/tlbflush.c b/arch/riscv/mm/tlbflush.c
>> index e737ba7949b1..8404530ec00f 100644
>> --- a/arch/riscv/mm/tlbflush.c
>> +++ b/arch/riscv/mm/tlbflush.c
>> @@ -234,11 +234,6 @@ void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *batch,
>>  	mmu_notifier_arch_invalidate_secondary_tlbs(mm, start, end);
>>  }
>>
>> -void arch_flush_tlb_batched_pending(struct mm_struct *mm)
>> -{
>> -	flush_tlb_mm(mm);
>> -}
>> -
>>  void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
>>  {
>>  	__flush_tlb_range(NULL, &batch->cpumask,
>> diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
>> index e9b81876ebe4..00daedfefc1b 100644
>> --- a/arch/x86/include/asm/tlbflush.h
>> +++ b/arch/x86/include/asm/tlbflush.h
>> @@ -356,11 +356,6 @@ static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b
>>  	mmu_notifier_arch_invalidate_secondary_tlbs(mm, 0, -1UL);
>>  }
>>
>> -static inline void arch_flush_tlb_batched_pending(struct mm_struct *mm)
>> -{
>> -	flush_tlb_mm(mm);
>> -}
>> -
>>  extern void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch);
>>
>>  static inline bool pte_flags_need_flush(unsigned long oldflags,
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index fb63d9256f09..fd160ddaa980 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -746,7 +746,7 @@ void flush_tlb_batched_pending(struct mm_struct *mm)
>>  	int flushed = batch >> TLB_FLUSH_BATCH_FLUSHED_SHIFT;
>>
>>  	if (pending != flushed) {
>> -		arch_flush_tlb_batched_pending(mm);
>> +		flush_tlb_mm(mm);
> 
> I see that CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH is only implemented in
> riscv (if !nommu), x86, arm64, and therefore we are only going to invoke
> this for those arches which previously did the same anyway, so this is
> safe.

It's also the way it used to be done before arm64 joined the party and thought
it could optimize by just issuing a DSB. I since discoved that the DSB approach
is buggy so arm64 has now fallen back to flush_tlb_mm() so the reason for the
original introduction of arch_flush_tlb_batched_pending() has gone.

Thanks,
Ryan

> 
> Kinda wish we could avoid this ugly #ifdef #else #endif pattern here in
> mm/rmap.c but probably necessary in this case.
> 
>>  		/*
>>  		 * If the new TLB flushing is pending during flushing, leave
>>  		 * mm->tlb_flush_batched as is, to avoid losing flushing.
>> --
>> 2.43.0
>>