Re: [PATCH] s390: Remove PageDirty check inside mk_pte()

[Alexander Gordeev <agordeev@linux.ibm.com>]

On Thu, Jan 16, 2025 at 09:23:36PM +0000, Matthew Wilcox (Oracle) wrote:

Hi Matthew,

> In commit abf09bed3cce ("s390/mm: implement software dirty bits"),
> the rationale for adding the PageDirty() test was:
> 
>     To avoid an excessive number of additional faults the
>     mk_pte primitive checks for PageDirty if the pgprot value
>     allows for writes and pre-dirties the pte. That avoids all
>     additional faults for tmpfs and shmem pages until these
>     pages are added to the swap cache.
> 
> shmem does not mark newly allocated folios as dirty since 2016 (commit
> 75edd345e8ed) so this test has been ineffective since then.  It still
> triggers for some calls to mk_pte(), but those calls are usually
> followed with other calls to pte_mkdirty() making the ones inside
> s390's mk_pte() redundant.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  arch/s390/include/asm/pgtable.h | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> Please check this doesn't cause any real world performance issues.
> Alexander and I briefly discussed this last year:
> https://lore.kernel.org/linux-mm/20240814154427.162475-5-willy@infradead.org/
> but then I missed his followup on August 22nd, and thought it best to
> reopen the conversation with a fresh patch to test.

I tried to take exactly this approach last year, but dropped the ball.

Despite the commit 75edd345e8ed claim above I still observed that a read
access to shmem page could end up in creation of a dirty PTE:

	handle_pte_fault() -> do_pte_missing() -> do_fault() ->
	do_read_fault() -> finish_fault() -> set_pte_range() -> mk_pte()

Our internal discussion (among other things) ended up in a need to really
understand where the faults are coming from.

Further, a cursory LTP test was showing ~18K page faults increase, which
I did not confirm. That is the first thing I will re-do.

Whether this change is a pre-requisite for something or what is your aim
wrt to this patch?

> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
> index 48268095b0a3..4c7e2fc2b5ff 100644
> --- a/arch/s390/include/asm/pgtable.h
> +++ b/arch/s390/include/asm/pgtable.h
> @@ -1443,11 +1443,8 @@ static inline pte_t mk_pte_phys(unsigned long physpage, pgprot_t pgprot)
>  static inline pte_t mk_pte(struct page *page, pgprot_t pgprot)
>  {
>  	unsigned long physpage = page_to_phys(page);
> -	pte_t __pte = mk_pte_phys(physpage, pgprot);
>  
> -	if (pte_write(__pte) && PageDirty(page))
> -		__pte = pte_mkdirty(__pte);
> -	return __pte;
> +	return mk_pte_phys(physpage, pgprot);
>  }
>  
>  #define pgd_index(address) (((address) >> PGDIR_SHIFT) & (PTRS_PER_PGD-1))

Thanks!