Re: [PATCH v2 1/2] sparc64: flush only the aliased page, not its whole folio

[Andreas Larsson <andreas@gaisler.com>]

Hi Imre,

On 2026-09-28 15:21, Imre Kaloz wrote:
> Since the conversion to folios, move_pte() and tlb_batch_add() flush the
> D-cache of every page in the folio a single PTE maps.  Both run once per
> PTE, so moving or unmapping a range backed by a large folio flushes each
> of its pages once for every PTE of that folio, and each page flush is a
> cross-call to all other online CPUs.

It would be good if the description of what the patch does uses present
tense, but past tense for what was wrong before the patch, or was
otherwise reworded to make it clear.

The cross-call overhead is not true for all sparc64 machines and only
for SMP configurations.


> Only the page the PTE maps can hold lines at the old colour, so flush
> just that page, as before the conversion.  flush_dcache_folio_all() has
> no other users and becomes flush_dcache_page_all() again.

This could perhaps be a bit misleading, as multiple PTEs in the range
handled by the caller can hold the old colour, could it not? Rather it
is the traversing of the whole range one by one in the caller that does
the trick. What do you aim to add to the previous description with this
paragraph?


> On SMP QEMU guests, apt rebuilding its caches through mremap() triggers
> RCU stalls and soft lockups in move_pte().

Past tense or rewording here as well as this patch fixes this.

> 
> Fixes: 1a10a44dfc1d ("sparc64: implement the new page table range API")
> Cc: stable@vger.kernel.org
> Signed-off-by: Imre Kaloz <kaloz@kernel.org>
> ---
>  arch/sparc/include/asm/cacheflush_64.h |  3 +--
>  arch/sparc/include/asm/pgtable_64.h    |  7 +++++--
>  arch/sparc/kernel/smp_64.c             | 24 +++++++++++++-----------
>  arch/sparc/mm/init_64.c                | 22 ++++++++++++++++++++++
>  arch/sparc/mm/tlb.c                    |  2 +-
>  5 files changed, 42 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/sparc/include/asm/cacheflush_64.h b/arch/sparc/include/asm/cacheflush_64.h
> index 06092572c045..02c969417e7b 100644
> --- a/arch/sparc/include/asm/cacheflush_64.h
> +++ b/arch/sparc/include/asm/cacheflush_64.h
> @@ -38,11 +38,10 @@ void __flush_dcache_page(void *addr, int flush_icache);
>  void flush_dcache_folio_impl(struct folio *folio);
>  #ifdef CONFIG_SMP
>  void smp_flush_dcache_folio_impl(struct folio *folio, int cpu);
> -void flush_dcache_folio_all(struct mm_struct *mm, struct folio *folio);
>  #else
>  #define smp_flush_dcache_folio_impl(folio, cpu) flush_dcache_folio_impl(folio)
> -#define flush_dcache_folio_all(mm, folio) flush_dcache_folio_impl(folio)
>  #endif
> +void flush_dcache_page_all(struct mm_struct *mm, struct page *page);
>  
>  void __flush_dcache_range(unsigned long start, unsigned long end);
>  #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 1
> diff --git a/arch/sparc/include/asm/pgtable_64.h b/arch/sparc/include/asm/pgtable_64.h
> index 0837ebbc5dce..6712eac4baf1 100644
> --- a/arch/sparc/include/asm/pgtable_64.h
> +++ b/arch/sparc/include/asm/pgtable_64.h
> @@ -946,6 +946,9 @@ static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
>  	set_pte_at((mm), (addr), (ptep), __pte(0UL))
>  
>  #ifdef DCACHE_ALIASING_POSSIBLE
> +/* Without pte_batch_hint(), move_ptes() calls this once per PTE, so
> + * only the page this PTE maps can have lines at the old colour.
> + */

I'm not ecstatic about implementing a function that in the general API
can be called for multiple ptes, even though this never happens on
sparc64 as things stands right now. The code paths that saves us, from
commit 0b5be138ce00 ("mm/mremap: avoid expensive folio lookup on mremap
folio pte batch"), was "only" added as a performance regression fix.

Perhaps the best logical solution would be if move_pte() got an added nr
argument just as get_and_clear_ptes() has.

Barring that, the comment should not describe why the change is ok, but
rather descrive the code as it stands after this patch, that it depends
on only being called for single pte's, and that this is accomplished by
using the default pte_batch_hint().

I would prefer the nr argument though, but that would go outside
arch/sparc. Comments from mm people on this would be great.

>  #define __HAVE_ARCH_MOVE_PTE
>  #define move_pte(pte, old_addr, new_addr)				\
>  ({									\
> @@ -955,8 +958,8 @@ static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
>  									\
>  		if (pfn_valid(this_pfn) &&				\
>  		    (((old_addr) ^ (new_addr)) & (1 << 13)))		\
> -			flush_dcache_folio_all(current->mm,		\
> -				page_folio(pfn_to_page(this_pfn)));	\
> +			flush_dcache_page_all(current->mm,		\
> +					      pfn_to_page(this_pfn));	\
>  	}								\
>  	newpte;								\
>  })
> diff --git a/arch/sparc/kernel/smp_64.c b/arch/sparc/kernel/smp_64.c
> index 371460e34484..18b6145da591 100644
> --- a/arch/sparc/kernel/smp_64.c
> +++ b/arch/sparc/kernel/smp_64.c
> @@ -982,8 +982,9 @@ void smp_flush_dcache_folio_impl(struct folio *folio, int cpu)
>  	put_cpu();
>  }
>  
> -void flush_dcache_folio_all(struct mm_struct *mm, struct folio *folio)
> +void flush_dcache_page_all(struct mm_struct *mm, struct page *page)
>  {
> +	struct folio *folio = page_folio(page);
>  	void *pg_addr;
>  	u64 data0;
>  
> @@ -996,7 +997,7 @@ void flush_dcache_folio_all(struct mm_struct *mm, struct folio *folio)
>  	atomic_inc(&dcpage_flushes);
>  #endif
>  	data0 = 0;
> -	pg_addr = folio_address(folio);
> +	pg_addr = page_address(page);
>  	if (tlb_type == spitfire) {
>  		data0 = ((u64)&xcall_flush_dcache_page_spitfire);
>  		if (folio_flush_mapping(folio) != NULL)
> @@ -1007,18 +1008,19 @@ void flush_dcache_folio_all(struct mm_struct *mm, struct folio *folio)
>  #endif
>  	}
>  	if (data0) {
> -		unsigned int i, nr = folio_nr_pages(folio);
> -
> -		for (i = 0; i < nr; i++) {
> -			xcall_deliver(data0, __pa(pg_addr),
> -				      (u64) pg_addr, cpu_online_mask);
> +		xcall_deliver(data0, __pa(pg_addr),
> +			      (u64)pg_addr, cpu_online_mask);
>  #ifdef CONFIG_DEBUG_DCFLUSH
> -			atomic_inc(&dcpage_flushes_xcall);
> +		atomic_inc(&dcpage_flushes_xcall);
>  #endif
> -			pg_addr += PAGE_SIZE;
> -		}
>  	}
> -	__local_flush_dcache_folio(folio);
> +#ifdef DCACHE_ALIASING_POSSIBLE
> +	__flush_dcache_page(pg_addr,
> +			    tlb_type == spitfire && folio_flush_mapping(folio));
> +#else
> +	if (tlb_type == spitfire && folio_flush_mapping(folio))
> +		__flush_icache_page(__pa(pg_addr));
> +#endif
>  
>  	preempt_enable();
>  }
> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
> index 103db4683b16..8792e5d92517 100644
> --- a/arch/sparc/mm/init_64.c
> +++ b/arch/sparc/mm/init_64.c
> @@ -214,6 +214,28 @@ inline void flush_dcache_folio_impl(struct folio *folio)
>  #endif
>  }
>  
> +#ifndef CONFIG_SMP
> +void flush_dcache_page_all(struct mm_struct *mm, struct page *page)
> +{
> +	struct folio *folio = page_folio(page);
> +
> +	if (tlb_type == hypervisor)
> +		return;
> +
> +#ifdef CONFIG_DEBUG_DCFLUSH
> +	atomic_inc(&dcpage_flushes);
> +#endif
> +
> +#ifdef DCACHE_ALIASING_POSSIBLE
> +	__flush_dcache_page(page_address(page),
> +			    tlb_type == spitfire && folio_flush_mapping(folio));
> +#else
> +	if (tlb_type == spitfire && folio_flush_mapping(folio))
> +		__flush_icache_page(page_to_phys(page));
> +#endif
> +}
> +#endif
> +
>  #define PG_dcache_dirty		PG_arch_1
>  #define PG_dcache_cpu_shift	32UL
>  #define PG_dcache_cpu_mask	\
> diff --git a/arch/sparc/mm/tlb.c b/arch/sparc/mm/tlb.c
> index 6d9dd5eb1328..1221814ca0e1 100644
> --- a/arch/sparc/mm/tlb.c
> +++ b/arch/sparc/mm/tlb.c
> @@ -144,7 +144,7 @@ void tlb_batch_add(struct mm_struct *mm, unsigned long vaddr,
>  
>  		paddr = (unsigned long) page_address(page);
>  		if ((paddr ^ vaddr) & (1 << 13))
> -			flush_dcache_folio_all(mm, folio);
> +			flush_dcache_page_all(mm, page);
>  	}
>  
>  no_cache_flush:

Thanks,
Andreas