Re: [PATCH] mm: Move set_pxd_safe() helpers from generic to platform

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 9/20/24 11:00, Anshuman Khandual wrote:
> set_pxd_safe() helpers that serve a specific purpose for both x86 and riscv
> platforms, do not need to be in the common memory code. Otherwise they just
> unnecessarily make the common API more complicated. This moves the helpers
> from common code to platform instead.
> 
> Cc: Paul Walmsley <paul.walmsley@sifive.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Ryan Roberts <ryan.roberts@arm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: x86@kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-riscv@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/riscv/include/asm/pgtable.h | 19 ++++++++++++++++
>  arch/x86/include/asm/pgtable.h   | 37 +++++++++++++++++++++++++++++++
>  include/linux/pgtable.h          | 38 --------------------------------
>  3 files changed, 56 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index 089f3c9f56a3..39ca652c5ebe 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -957,6 +957,25 @@ void misc_mem_init(void);
>  extern unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)];
>  #define ZERO_PAGE(vaddr) (virt_to_page(empty_zero_page))
>  
> +/*
> + * Use set_p*_safe(), and elide TLB flushing, when confident that *no*
> + * TLB flush will be required as a result of the "set". For example, use
> + * in scenarios where it is known ahead of time that the routine is
> + * setting non-present entries, or re-setting an existing entry to the
> + * same value. Otherwise, use the typical "set" helpers and flush the
> + * TLB.
> + */
> +#define set_p4d_safe(p4dp, p4d) \
> +({ \
> +	WARN_ON_ONCE(p4d_present(*p4dp) && !p4d_same(*p4dp, p4d)); \
> +	set_p4d(p4dp, p4d); \
> +})
> +
> +#define set_pgd_safe(pgdp, pgd) \
> +({ \
> +	WARN_ON_ONCE(pgd_present(*pgdp) && !pgd_same(*pgdp, pgd)); \
> +	set_pgd(pgdp, pgd); \
> +})
>  #endif /* !__ASSEMBLY__ */
>  
>  #endif /* _ASM_RISCV_PGTABLE_H */
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index e39311a89bf4..fefb52bb6b4d 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -1701,6 +1701,43 @@ bool arch_is_platform_page(u64 paddr);
>  #define arch_is_platform_page arch_is_platform_page
>  #endif
>  
> +/*
> + * Use set_p*_safe(), and elide TLB flushing, when confident that *no*
> + * TLB flush will be required as a result of the "set". For example, use
> + * in scenarios where it is known ahead of time that the routine is
> + * setting non-present entries, or re-setting an existing entry to the
> + * same value. Otherwise, use the typical "set" helpers and flush the
> + * TLB.
> + */
> +#define set_pte_safe(ptep, pte) \
> +({ \
> +	WARN_ON_ONCE(pte_present(*ptep) && !pte_same(*ptep, pte)); \
> +	set_pte(ptep, pte); \
> +})
> +
> +#define set_pmd_safe(pmdp, pmd) \
> +({ \
> +	WARN_ON_ONCE(pmd_present(*pmdp) && !pmd_same(*pmdp, pmd)); \
> +	set_pmd(pmdp, pmd); \
> +})
> +
> +#define set_pud_safe(pudp, pud) \
> +({ \
> +	WARN_ON_ONCE(pud_present(*pudp) && !pud_same(*pudp, pud)); \
> +	set_pud(pudp, pud); \
> +})
> +
> +#define set_p4d_safe(p4dp, p4d) \
> +({ \
> +	WARN_ON_ONCE(p4d_present(*p4dp) && !p4d_same(*p4dp, p4d)); \
> +	set_p4d(p4dp, p4d); \
> +})
> +
> +#define set_pgd_safe(pgdp, pgd) \
> +({ \
> +	WARN_ON_ONCE(pgd_present(*pgdp) && !pgd_same(*pgdp, pgd)); \
> +	set_pgd(pgdp, pgd); \
> +})
>  #endif	/* __ASSEMBLY__ */
>  
>  #endif /* _ASM_X86_PGTABLE_H */
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index 2a6a3cccfc36..0bf88e505aad 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -1050,44 +1050,6 @@ static inline int pgd_same(pgd_t pgd_a, pgd_t pgd_b)
>  }
>  #endif
>  
> -/*
> - * Use set_p*_safe(), and elide TLB flushing, when confident that *no*
> - * TLB flush will be required as a result of the "set". For example, use
> - * in scenarios where it is known ahead of time that the routine is
> - * setting non-present entries, or re-setting an existing entry to the
> - * same value. Otherwise, use the typical "set" helpers and flush the
> - * TLB.
> - */
> -#define set_pte_safe(ptep, pte) \
> -({ \
> -	WARN_ON_ONCE(pte_present(*ptep) && !pte_same(*ptep, pte)); \
> -	set_pte(ptep, pte); \
> -})
> -
> -#define set_pmd_safe(pmdp, pmd) \
> -({ \
> -	WARN_ON_ONCE(pmd_present(*pmdp) && !pmd_same(*pmdp, pmd)); \
> -	set_pmd(pmdp, pmd); \
> -})
> -
> -#define set_pud_safe(pudp, pud) \
> -({ \
> -	WARN_ON_ONCE(pud_present(*pudp) && !pud_same(*pudp, pud)); \
> -	set_pud(pudp, pud); \
> -})
> -
> -#define set_p4d_safe(p4dp, p4d) \
> -({ \
> -	WARN_ON_ONCE(p4d_present(*p4dp) && !p4d_same(*p4dp, p4d)); \
> -	set_p4d(p4dp, p4d); \
> -})
> -
> -#define set_pgd_safe(pgdp, pgd) \
> -({ \
> -	WARN_ON_ONCE(pgd_present(*pgdp) && !pgd_same(*pgdp, pgd)); \
> -	set_pgd(pgdp, pgd); \
> -})
> -
>  #ifndef __HAVE_ARCH_DO_SWAP_PAGE
>  static inline void arch_do_swap_page_nr(struct mm_struct *mm,
>  				     struct vm_area_struct *vma,

Hello Dave/Palmer,

Unless there is still any more objection from x86 or riscv, could this
patch be pulled ?

- Anshuman