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

[Imre Kaloz <kaloz@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3698 bytes --]

Hi Andreas,

On Thu, 15 Oct 2026, Andreas Larsson wrote:

> 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.

Will do.

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

Right, it's only on SMP and only on sun4u CPUs whose D-cache can alias;
on sun4v flush_dcache_folio_all() returns right away. I'll say so.

>> 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?

You're right. What I wanted to say is that flushing the page each PTE
maps is enough because the caller walks the range one PTE at a time, so
every page gets its own flush. I'll reword it that way.

>> 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.

Will do.

<snip>

>> 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.
>

I'll rewrite the comment that way.

I like the nr idea. Since sparc64 is the only user of move_pte(), it
would be a small change: move_ptes() would pass nr_ptes, the generic
version would ignore it, and sparc64 would flush every aliased page in
the batch. I’d still keep it as a separate patch on top of this one,
though, so the fix can go to stable as a plain sparc64 backport.

Matthew, Mike, Andrew, would you be fine with move_pte() taking nr?


Thanks,
Imre