linux-kernel - Re: [PATCH v2] arm64: enable THP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b2694573-a696-8435-70eb-ebc9c06500a0@arm.com>
Date:   Mon, 30 May 2022 12:37:06 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Barry Song <21cnbao@...il.com>, akpm@...ux-foundation.org,
        catalin.marinas@....com, will@...nel.org, linux-mm@...ck.org
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        zhangshiming@...o.com, guojian@...o.com, hanchuanhua@...o.com,
        Barry Song <v-songbaohua@...o.com>,
        "Huang, Ying" <ying.huang@...el.com>,
        Minchan Kim <minchan@...nel.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Hugh Dickins <hughd@...gle.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Steven Price <steven.price@....com>,
        Yang Shi <shy828301@...il.com>
Subject: Re: [PATCH v2] arm64: enable THP_SWAP for arm64

Hello Barry,

On 5/27/22 15:36, Barry Song wrote:
> From: Barry Song <v-songbaohua@...o.com>
> 
> THP_SWAP has been proved to improve the swap throughput significantly
> on x86_64 according to commit bd4c82c22c367e ("mm, THP, swap: delay
> splitting THP after swapped out").
It will be useful to run similar experiments on arm64 platform to
demonstrate tangible benefit, else we might be just enabling this
feature just because x86 has it. Do you have some data points ?

> As long as arm64 uses 4K page size, it is quite similar with x86_64
> by having 2MB PMD THP. So we are going to get similar improvement.

This is an assumption without any data points (until now). Please
do provide some results.

> For other page sizes such as 16KB and 64KB, PMD might be too large.
> Negative side effects such as IO latency might be a problem. Thus,
> we can only safely enable the counterpart of X86_64.

Incorrect reasoning. Although sometimes it might be okay to enable
a feature on platforms with possible assumptions about its benefits,
but to claim 'similar improvement, safely, .. etc' while comparing
against x86 4K page config without data points, is not very helpful.

> A corner case is that MTE has an assumption that only base pages
> can be swapped. We won't enable THP_SWP for ARM64 hardware with
> MTE support until MTE is re-arched.

re-arched ?? Did you imply that MTE is reworked to support THP ?

> 
> Cc: "Huang, Ying" <ying.huang@...el.com>
> Cc: Minchan Kim <minchan@...nel.org>
> Cc: Johannes Weiner <hannes@...xchg.org>
> Cc: Hugh Dickins <hughd@...gle.com>
> Cc: Andrea Arcangeli <aarcange@...hat.com>
> Cc: Anshuman Khandual <anshuman.khandual@....com>
> Cc: Steven Price <steven.price@....com>
> Cc: Yang Shi <shy828301@...il.com>
> Signed-off-by: Barry Song <v-songbaohua@...o.com>
> ---
>  arch/arm64/Kconfig               |  1 +
>  arch/arm64/include/asm/pgtable.h |  2 ++
>  include/linux/huge_mm.h          | 12 ++++++++++++
>  mm/swap_slots.c                  |  2 +-
>  4 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index a4968845e67f..5306009df2dc 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -101,6 +101,7 @@ config ARM64
>  	select ARCH_WANT_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
>  	select ARCH_WANT_LD_ORPHAN_WARN
>  	select ARCH_WANTS_NO_INSTR
> +	select ARCH_WANTS_THP_SWAP if ARM64_4K_PAGES
>  	select ARCH_HAS_UBSAN_SANITIZE_ALL
>  	select ARM_AMBA
>  	select ARM_ARCH_TIMER
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 0b6632f18364..06076139c72c 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -45,6 +45,8 @@
>  	__flush_tlb_range(vma, addr, end, PUD_SIZE, false, 1)
>  #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>  
> +#define arch_thp_swp_supported !system_supports_mte

Does it check for 'system_supports_mte' as a symbol or call system_supports_mte()
to ascertain runtime MTE support ? It might well be correct, but just does not
look much intuitive.

> +
>  /*
>   * Outside of a few very special situations (e.g. hibernation), we always
>   * use broadcast TLB invalidation instructions, therefore a spurious page
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index de29821231c9..4ddaf6ad73ef 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -461,4 +461,16 @@ static inline int split_folio_to_list(struct folio *folio,
>  	return split_huge_page_to_list(&folio->page, list);
>  }
>  
> +/*
> + * archs that select ARCH_WANTS_THP_SWAP but don't support THP_SWP due to
> + * limitations in the implementation like arm64 MTE can override this to

A small nit.

A 'comma' here will be better. s/arm64 MTE can/arm64 MTE, can/

> + * false

Similarly a 'full stop' here will be better as well.

> + */
> +#ifndef arch_thp_swp_supported
> +static inline bool arch_thp_swp_supported(void)
> +{
> +	return true;
> +}
> +#endif
> +
>  #endif /* _LINUX_HUGE_MM_H */
> diff --git a/mm/swap_slots.c b/mm/swap_slots.c
> index 2a65a89b5b4d..10b94d64cc25 100644
> --- a/mm/swap_slots.c
> +++ b/mm/swap_slots.c
> @@ -307,7 +307,7 @@ swp_entry_t folio_alloc_swap(struct folio *folio)
>  	entry.val = 0;
>  
>  	if (folio_test_large(folio)) {
> -		if (IS_ENABLED(CONFIG_THP_SWAP))
> +		if (IS_ENABLED(CONFIG_THP_SWAP) && arch_thp_swp_supported())
>  			get_swap_pages(1, &entry, folio_nr_pages(folio));
>  		goto out;
>  	}

- Anshuman