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@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx/
> 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!
