Re: [v3 PATCH 1/2] hugetlb: arm64: add mte support
From: David Hildenbrand
Date: Mon Sep 09 2024 - 04:44:06 EST
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@xxxxxxxxxxxxxxxxxxxxxx>
---
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