[v3 PATCH 1/2] hugetlb: arm64: add mte support
David Hildenbrand
david at redhat.com
Mon Sep 9 01:43:20 PDT 2024
On 06.09.24 19:59, Yang Shi wrote:
> Enable MTE support for hugetlb.
>
> The MTE page flags will be set on the folio only. When copying
> hugetlb folio (for example, CoW), the tags for all subpages will be copied
> when copying the first subpage.
>
> When freeing hugetlb folio, the MTE flags will be cleared.
>
> Signed-off-by: Yang Shi <yang at os.amperecomputing.com>
> ---
> arch/arm64/include/asm/hugetlb.h | 15 ++++++-
> arch/arm64/include/asm/mman.h | 3 +-
> arch/arm64/include/asm/mte.h | 67 ++++++++++++++++++++++++++++++++
> arch/arm64/kernel/hibernate.c | 7 ++++
> arch/arm64/kernel/mte.c | 25 +++++++++++-
> arch/arm64/kvm/guest.c | 16 ++++++--
> arch/arm64/kvm/mmu.c | 11 ++++++
> arch/arm64/mm/copypage.c | 33 +++++++++++++---
> fs/hugetlbfs/inode.c | 2 +-
> 9 files changed, 166 insertions(+), 13 deletions(-)
>
> v3: * Fixed the build error when !CONFIG_ARM64_MTE.
> * Incorporated the comment from David to have hugetlb folio
> specific APIs for manipulating the page flags.
> * Don't assume the first page is the head page since huge page copy
> can start from any subpage.
> v2: * Reimplemented the patch to fix the comments from Catalin.
> * Added test cases (patch #2) per Catalin.
>
> diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h
> index 293f880865e8..06f621c5cece 100644
> --- a/arch/arm64/include/asm/hugetlb.h
> +++ b/arch/arm64/include/asm/hugetlb.h
> @@ -11,6 +11,7 @@
> #define __ASM_HUGETLB_H
>
> #include <asm/cacheflush.h>
> +#include <asm/mte.h>
> #include <asm/page.h>
>
> #ifdef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION
> @@ -18,9 +19,21 @@
> extern bool arch_hugetlb_migration_supported(struct hstate *h);
> #endif
>
> +#ifdef CONFIG_ARM64_MTE
> +#define CLEAR_FLAGS (BIT(PG_dcache_clean) | BIT(PG_mte_tagged) | \
> + BIT(PG_mte_lock))
> +#else
> +#define CLEAR_FLAGS BIT(PG_dcache_clean)
> +#endif
> +
> static inline void arch_clear_hugetlb_flags(struct folio *folio)
> {
> - clear_bit(PG_dcache_clean, &folio->flags);
> + if (!system_supports_mte()) {
> + clear_bit(PG_dcache_clean, &folio->flags);
> + return;
> + }
> +
> + folio->flags &= ~CLEAR_FLAGS;
In contrast to clear_bit, this is now not an atomic operation anymore.
Could we have concurrent modifications (locking the folio? mte?) where
we could mess up (IOW, is there a reason we don't do __clear_bit in
existing code)?
Maybe start with:
static inline void arch_clear_hugetlb_flags(struct folio *folio)
{
clear_bit(PG_dcache_clean, &folio->flags);
#ifdef CONFIG_ARM64_MTE
if (system_supports_mte()) {
clear_bit(PG_mte_tagged, &folio->flags);
clear_bit(PG_mte_lock, &folio->flags);
}
#endif
}
And if you can argue that atomics are not required, convert all to
__clear_bit() and have the compiler optimize it for you.
> }
> #define arch_clear_hugetlb_flags arch_clear_hugetlb_flags
> > diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index 0f84518632b4..cec9fb6fec3b 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -41,6 +41,8 @@ void mte_free_tag_storage(char *storage);
>
> static inline void set_page_mte_tagged(struct page *page)
> {
> + VM_WARN_ON_ONCE(folio_test_hugetlb(page_folio(page)));
> +
> /*
> * Ensure that the tags written prior to this function are visible
> * before the page flags update.
> @@ -51,6 +53,8 @@ static inline void set_page_mte_tagged(struct page *page)
>
> static inline bool page_mte_tagged(struct page *page)
> {
> + VM_WARN_ON_ONCE(folio_test_hugetlb(page_folio(page)));
> +
> bool ret = test_bit(PG_mte_tagged, &page->flags);
>
> /*
> @@ -76,6 +80,8 @@ static inline bool page_mte_tagged(struct page *page)
> */
> static inline bool try_page_mte_tagging(struct page *page)
> {
> + VM_WARN_ON_ONCE(folio_test_hugetlb(page_folio(page)));
> +
> if (!test_and_set_bit(PG_mte_lock, &page->flags))
> return true;
>
[...]
> +static inline void set_folio_hugetlb_mte_tagged(struct folio *folio)
> +{
> +}
> +
> +static inline bool folio_hugetlb_mte_tagged(struct folio *folio)
> +{
> + return false;
> +}
> +
> +static inline bool try_folio_hugetlb_mte_tagging(struct folio *folio)
> +{
> + return false;
> +}
I would suggest to stick to the format of our folio_test / folio_set ...
functions. Please refer to
folio_set_hugetlb_migratable/folio_set_hugetlb_hwpoison/ ...
Something like:
folio_test_hugetlb_mte_tagged
folio_set_hugetlb_mte_tagged
But the semantics of try_folio_hugetlb_mte_tagging() are a bit less
obvious. I would suggest
folio_test_and_set_hugetlb_mte_lock()
We should probably clean up the page_* variants separately.
But ARM maintainers can feel free to intervene here.
> +#endif
> +
> static inline void mte_disable_tco_entry(struct task_struct *task)
> {
> if (!system_supports_mte())
> diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
> index 02870beb271e..ebf81fffa79d 100644
> --- a/arch/arm64/kernel/hibernate.c
> +++ b/arch/arm64/kernel/hibernate.c
> @@ -266,10 +266,17 @@ static int swsusp_mte_save_tags(void)
> max_zone_pfn = zone_end_pfn(zone);
> for (pfn = zone->zone_start_pfn; pfn < max_zone_pfn; pfn++) {
> struct page *page = pfn_to_online_page(pfn);
> + struct folio *folio;
>
> if (!page)
> continue;
>
Nit: I would drop this empty line.
> + folio = page_folio(page);
> +
> + if (folio_test_hugetlb(folio) &&
> + !folio_hugetlb_mte_tagged(folio))
> + continue;
> +
> if (!page_mte_tagged(page))
> continue;
>
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 6174671be7c1..c8b13bf36fc6 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -38,7 +38,22 @@ EXPORT_SYMBOL_GPL(mte_async_or_asymm_mode);
> void mte_sync_tags(pte_t pte, unsigned int nr_pages)
> {
> struct page *page = pte_page(pte);
> - unsigned int i;
> + struct folio *folio = page_folio(page);
> + unsigned long i;
> +
> + if (folio_test_hugetlb(folio)) {
> + unsigned long nr = folio_nr_pages(folio);
Nit: empty line please.
> + /* Hugetlb MTE flags are set for head page only */
> + if (try_folio_hugetlb_mte_tagging(folio)) {
> + for (i = 0; i < nr; i++, page++)
> + mte_clear_page_tags(page_address(page));
> + set_folio_hugetlb_mte_tagged(folio);
> + }
> +
> + smp_wmb();
We already do have one in set_folio_hugetlb_mte_tagged() [and
try_folio_hugetlb_mte_tagging() does some magic as well], do we really
need this smp_wmb()?
In general, I think checkpatch will tell you to document memory barriers
and their counterparts thoroughly.
> +
> + return;
> + }
--
Cheers,
David / dhildenb
More information about the linux-arm-kernel
mailing list