mm: memory-failure: fix HWPoison flag race with non-atomic page flag ops

This series fixes the race by:

1. Having memory_failure() call synchronize_rcu() + retry after setting HWPoison, so that any in-flight non-atomic RMW that read the old flags value completes before we proceed.

2. Wrapping all non-atomic page flag operations in rcu_read_lock/rcu_read_unlock (CONFIG_MEMORY_FAILURE only), so that synchronize_rcu() actually drains them.

Performance impact (page alloc+free microbenchmark, 200K iterations, 20 runs, KVM guest, error bars are 3-sigma):

!PREEMPT_RCU (x86):

                         insns/iter      cycles/iter
    base:                12237 +/- 1     17954 +/- 136
    patched:             +22 +/- 1       -124 +/- 122
                         (+0.18%)        (within noise)

PREEMPT_RCU:

                         insns/iter      cycles/iter
    base:                12512 +/- 3     18541 +/- 214
    patched:             +95 +/- 3       -12 +/- 161
                         (+0.76%)        (within noise)

When !CONFIG_MEMORY_FAILURE, all wrappers compile away completely.

Quoted 9 lines · Michael S. Tsirkin

This series fixes the race by:

1. Having memory_failure() call synchronize_rcu() + retry after setting HWPoison, so that any in-flight non-atomic RMW that read the old flags value completes before we proceed.

2. Wrapping all non-atomic page flag operations in rcu_read_lock/rcu_read_unlock (CONFIG_MEMORY_FAILURE only), so that synchronize_rcu() actually drains them.

It wouldn't surprise me if your underlying performance assumptions -- an non contended atomic is cheaper than a rcu_read_lock/unlock -- are not true in various CPU/kernel configuration combinations.

Modern CPUs have fast atomics when uncontended in normal circumstances. But it probably doesn't matter much either way because the difference shouldn't be very much.

It seems very complicated for something that
could be much simpler.

But I guess it's fine.

-Andi

Quoted 11 lines · Andi Kleen

Quoted 9 lines · Michael S. Tsirkin

This series fixes the race by:

1. Having memory_failure() call synchronize_rcu() + retry after setting HWPoison, so that any in-flight non-atomic RMW that read the old flags value completes before we proceed.

2. Wrapping all non-atomic page flag operations in rcu_read_lock/rcu_read_unlock (CONFIG_MEMORY_FAILURE only), so that synchronize_rcu() actually drains them.

Quoted 7 lines

It wouldn't surprise me if your underlying performance assumptions -- an non contended atomic is cheaper than a rcu_read_lock/unlock -- are not true in various CPU/kernel configuration combinations.

Modern CPUs have fast atomics when uncontended in normal circumstances. But it probably doesn't matter much either way because the difference shouldn't be very much.

Hmm. It's a bit silly that I didn't try. Seemed clear to me, but, on this old xeon...

                                   insns/iter    cycles/iter
     -------------------------------------------------------
       base                       12238 +/- 1.0    17889 +/- 97.9
       rcu_read_lock              12251 +/- 7.3    17991 +/-191.6
       atomic ops                 12233 +/- 1.9    17733 +/-136.5

The diff in the noise.

And old, slow CPUs maybe don't have MF at all.

So maybe just atomics instead of all this mess.

Quoted 6 lines

It seems very complicated for something that
could be much simpler.

But I guess it's fine.

-Andi

Indeed. David already said he's gonnu look at this himself, but he

Quoted 11 lines · Andi Kleen

Quoted 9 lines · Michael S. Tsirkin

This series fixes the race by:

1. Having memory_failure() call synchronize_rcu() + retry after setting HWPoison, so that any in-flight non-atomic RMW that read the old flags value completes before we proceed.

2. Wrapping all non-atomic page flag operations in rcu_read_lock/rcu_read_unlock (CONFIG_MEMORY_FAILURE only), so that synchronize_rcu() actually drains them.

Quoted 7 lines

It wouldn't surprise me if your underlying performance assumptions -- an non contended atomic is cheaper than a rcu_read_lock/unlock -- are not true in various CPU/kernel configuration combinations.

Modern CPUs have fast atomics when uncontended in normal circumstances. But it probably doesn't matter much either way because the difference shouldn't be very much.

Hmm. It's a bit silly that I didn't try. Seemed clear to me, but, on this old xeon...

                                   insns/iter    cycles/iter
     -------------------------------------------------------
       base                       12238 +/- 1.0    17889 +/- 97.9
       rcu_read_lock              12251 +/- 7.3    17991 +/-191.6
       atomic ops                 12233 +/- 1.9    17733 +/-136.5

The diff in the noise.

And old, slow CPUs maybe don't have MF at all.

So maybe just atomics instead of all this mess.

Quoted 9 lines

It seems very complicated for something that
could be much simpler.

But I guess it's fine.

-Andi

Indeed. David already said he's gonnu look at this himself, but here's what I tested, maybe helpful for whoever wants to try this approach:

Signed-off-by Michael S. Tsirkin <[email protected]>

include/linux/page-flags.h +45 -7

index 7223f6f4e2b4..8f0940cf2f29 100644
@@ -404,6 +404,44 @@ static unsigned long *folio_flags(struct folio *folio, unsigned n)
 #define FOLIO_HEAD_PAGE		0
 #define FOLIO_SECOND_PAGE	1
 
+/*
+ * When CONFIG_MEMORY_FAILURE is enabled, non-atomic page flag operations
+ * must be atomic to prevent clobbering concurrent TestSetPageHWPoison()
+ * in memory_failure().  Otherwise, use the cheaper non-atomic versions.
+ */
+#ifdef CONFIG_MEMORY_FAILURE
+#define __pf_set_bit		set_bit
+#define __pf_clear_bit		clear_bit
+
+static __always_inline void page_flags_clear(unsigned long *flags,
+					     unsigned long mask)
+{
+	atomic_long_andnot(mask, (atomic_long_t *)flags);
+}
+
+static __always_inline void page_flags_set(unsigned long *flags,
+					   unsigned long mask)
+{
+	atomic_long_or(mask, (atomic_long_t *)flags);
+}
+
+#else /* !CONFIG_MEMORY_FAILURE */
+#define __pf_set_bit		__set_bit
+#define __pf_clear_bit		__clear_bit
+
+static __always_inline void page_flags_clear(unsigned long *flags,
+					     unsigned long mask)
+{
+	*flags &= ~mask;
+}
+
+static __always_inline void page_flags_set(unsigned long *flags,
+					   unsigned long mask)
+{
+	*flags |= mask;
+}
+#endif /* CONFIG_MEMORY_FAILURE */
+
 /*
  * Macros to create function definitions for page flags
  */

@@ -421,11 +459,11 @@ static __always_inline void folio_clear_##name(struct folio *folio)	\
 
 #define __FOLIO_SET_FLAG(name, page)					\
 static __always_inline void __folio_set_##name(struct folio *folio)	\
-{ __set_bit(PG_##name, folio_flags(folio, page)); }
+{ __pf_set_bit(PG_##name, folio_flags(folio, page)); }
 
 #define __FOLIO_CLEAR_FLAG(name, page)					\
 static __always_inline void __folio_clear_##name(struct folio *folio)	\
-{ __clear_bit(PG_##name, folio_flags(folio, page)); }
+{ __pf_clear_bit(PG_##name, folio_flags(folio, page)); }
 
 #define FOLIO_TEST_SET_FLAG(name, page)					\
 static __always_inline bool folio_test_set_##name(struct folio *folio)	\

@@ -458,12 +496,12 @@ static __always_inline void ClearPage##uname(struct page *page)		\
 #define __SETPAGEFLAG(uname, lname, policy)				\
 __FOLIO_SET_FLAG(lname, FOLIO_##policy)					\
 static __always_inline void __SetPage##uname(struct page *page)		\
-{ __set_bit(PG_##lname, &policy(page, 1)->flags.f); }
+{ __pf_set_bit(PG_##lname, &policy(page, 1)->flags.f); }
 
 #define __CLEARPAGEFLAG(uname, lname, policy)				\
 __FOLIO_CLEAR_FLAG(lname, FOLIO_##policy)				\
 static __always_inline void __ClearPage##uname(struct page *page)	\
-{ __clear_bit(PG_##lname, &policy(page, 1)->flags.f); }
+{ __pf_clear_bit(PG_##lname, &policy(page, 1)->flags.f); }
 
 #define TESTSETFLAG(uname, lname, policy)				\
 FOLIO_TEST_SET_FLAG(lname, FOLIO_##policy)				\

@@ -806,7 +844,7 @@ static inline bool PageUptodate(const struct page *page)
 static __always_inline void __folio_mark_uptodate(struct folio *folio)
 {
 	smp_wmb();
-	__set_bit(PG_uptodate, folio_flags(folio, 0));
+	__pf_set_bit(PG_uptodate, folio_flags(folio, 0));
 }
 
 static __always_inline void folio_mark_uptodate(struct folio *folio)

@@ -1162,14 +1200,14 @@ static __always_inline void ClearPageAnonExclusive(struct page *page)
 {
 	VM_BUG_ON_PGFLAGS(!PageAnonNotKsm(page), page);
 	VM_BUG_ON_PGFLAGS(PageHuge(page) && !PageHead(page), page);
-	clear_bit(PG_anon_exclusive, &PF_ANY(page, 1)->flags.f);
+	__pf_clear_bit(PG_anon_exclusive, &PF_ANY(page, 1)->flags.f);
 }
 
 static __always_inline void __ClearPageAnonExclusive(struct page *page)
 {
 	VM_BUG_ON_PGFLAGS(!PageAnon(page), page);
 	VM_BUG_ON_PGFLAGS(PageHuge(page) && !PageHead(page), page);
-	__clear_bit(PG_anon_exclusive, &PF_ANY(page, 1)->flags.f);
+	__pf_clear_bit(PG_anon_exclusive, &PF_ANY(page, 1)->flags.f);
 }
 
 #ifdef CONFIG_MMU

include/linux/mm.h +7 -7

index 06bbe9eba636..931dfc84319f 100644
@@ -2343,7 +2343,7 @@ int folio_xchg_last_cpupid(struct folio *folio, int cpupid);
 
 static inline void page_cpupid_reset_last(struct page *page)
 {
-	page->flags.f |= LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT;
+	page_flags_set(&page->flags.f, LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT);
 }
 #endif /* LAST_CPUPID_NOT_IN_PAGE_FLAGS */
 

@@ -2503,8 +2503,8 @@ static inline struct zone *folio_zone(const struct folio *folio)
 #ifdef SECTION_IN_PAGE_FLAGS
 static inline void set_page_section(struct page *page, unsigned long section)
 {
-	page->flags.f &= ~(SECTIONS_MASK << SECTIONS_PGSHIFT);
-	page->flags.f |= (section & SECTIONS_MASK) << SECTIONS_PGSHIFT;
+	page_flags_clear(&page->flags.f, SECTIONS_MASK << SECTIONS_PGSHIFT);
+	page_flags_set(&page->flags.f, (section & SECTIONS_MASK) << SECTIONS_PGSHIFT);
 }
 
 static inline unsigned long memdesc_section(memdesc_flags_t mdf)

@@ -2719,14 +2719,14 @@ static inline bool folio_is_longterm_pinnable(struct folio *folio)
 
 static inline void set_page_zone(struct page *page, enum zone_type zone)
 {
-	page->flags.f &= ~(ZONES_MASK << ZONES_PGSHIFT);
-	page->flags.f |= (zone & ZONES_MASK) << ZONES_PGSHIFT;
+	page_flags_clear(&page->flags.f, ZONES_MASK << ZONES_PGSHIFT);
+	page_flags_set(&page->flags.f, (zone & ZONES_MASK) << ZONES_PGSHIFT);
 }
 
 static inline void set_page_node(struct page *page, unsigned long node)
 {
-	page->flags.f &= ~(NODES_MASK << NODES_PGSHIFT);
-	page->flags.f |= (node & NODES_MASK) << NODES_PGSHIFT;
+	page_flags_clear(&page->flags.f, NODES_MASK << NODES_PGSHIFT);
+	page_flags_set(&page->flags.f, (node & NODES_MASK) << NODES_PGSHIFT);
 }
 
 static inline void set_page_links(struct page *page, enum zone_type zone,

mm/huge_memory.c +2 -2

index 970e077019b7..100226f59490 100644
@@ -3624,8 +3624,8 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
 		 * unreferenced sub-pages of an anonymous THP: we can simply drop
 		 * PG_anon_exclusive (-> PG_mappedtodisk) for these here.
 		 */
-		new_folio->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
-		new_folio->flags.f |= (folio->flags.f &
+		page_flags_clear(&new_folio->flags.f, PAGE_FLAGS_CHECK_AT_PREP);
+		page_flags_set(&new_folio->flags.f, folio->flags.f &
 				((1L << PG_referenced) |
 				 (1L << PG_swapbacked) |
 				 (1L << PG_swapcache) |

mm/memremap.c +1 -1

index 053842d45cb1..194ca2f04a87 100644
@@ -494,7 +494,7 @@ void zone_device_page_init(struct page *page, struct dev_pagemap *pgmap,
 		 * blindly clear bits which could have set my order field here,
 		 * including page head.
 		 */
-		new_page->flags.f &= ~0xffUL;	/* Clear possible order, page head */
+		page_flags_clear(&new_page->flags.f, 0xffUL);	/* Clear possible order, page head */
 
 #ifdef NR_PAGES_IN_LARGE_FOLIO
 		/*

mm/page_alloc.c +3 -3

index d49c254174da..a3447124a725 100644
@@ -1359,7 +1359,7 @@ __always_inline bool __free_pages_prepare(struct page *page,
 		int i;
 
 		if (compound) {
-			page[1].flags.f &= ~PAGE_FLAGS_SECOND;
+			page_flags_clear(&page[1].flags.f, PAGE_FLAGS_SECOND);
 #ifdef NR_PAGES_IN_LARGE_FOLIO
 			folio->_nr_pages = 0;
 #endif

@@ -1373,7 +1373,7 @@ __always_inline bool __free_pages_prepare(struct page *page,
 					continue;
 				}
 			}
-			(page + i)->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
+			page_flags_clear(&(page + i)->flags.f, PAGE_FLAGS_CHECK_AT_PREP);
 		}
 	}
 	if (folio_test_anon(folio)) {

@@ -1392,7 +1392,7 @@ __always_inline bool __free_pages_prepare(struct page *page,
 	}
 
 	page_cpupid_reset_last(page);
-	page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
+	page_flags_clear(&page->flags.f, PAGE_FLAGS_CHECK_AT_PREP);
 	page->private = 0;
 	reset_page_owner(page, order);
 	page_table_check_free(page, order);

mm/slub.c +1 -1

index a2bf3756ca7d..a55199f642d3 100644
@@ -617,7 +617,7 @@ static inline void slab_set_pfmemalloc(struct slab *slab)
 
 static inline void __slab_clear_pfmemalloc(struct slab *slab)
 {
-	__clear_bit(SL_pfmemalloc, &slab->flags.f);
+	__pf_clear_bit(SL_pfmemalloc, &slab->flags.f);
 }
 
 /*

However, this was a basic test, when allocating 4k pages. With 2M hugepages:

                                   insns/iter    cycles/iter
     -------------------------------------------------------
       base                       20758 +/- 12.5   191208 +/-1946.6
       rcu                        20785 +/-  3.7   197108 +/- 132.1
       atomic                     20727 +/-  6.4   204591 +/- 160.2

       rcu vs base                  +27 (+0.13%)     +5900 (+3.09%)
       atomic vs base               -31 (-0.15%)    +13383 (+7.00%)

and even with THP:

                                   insns/iter    cycles/iter
     -------------------------------------------------------
       base                       27220 +/-  2.8   192151 +/- 483.3
       rcu                        27248 +/- 30.1   194159 +/-2746.6
       atomic                     27186 +/-  3.2   200526 +/- 746.2

       rcu vs base                  +28 (+0.10%)     +2008 (+1.04%)
       atomic vs base               -34 (-0.12%)     +8374 (+4.36%)

needs more thought.

Quoted 23 lines

However, this was a basic test, when allocating 4k pages. With 2M hugepages:

                                   insns/iter    cycles/iter
     -------------------------------------------------------
       base                       20758 +/- 12.5   191208 +/-1946.6
       rcu                        20785 +/-  3.7   197108 +/- 132.1
       atomic                     20727 +/-  6.4   204591 +/- 160.2

       rcu vs base                  +27 (+0.13%)     +5900 (+3.09%)
       atomic vs base               -31 (-0.15%)    +13383 (+7.00%)

and even with THP:

                                   insns/iter    cycles/iter
     -------------------------------------------------------
       base                       27220 +/-  2.8   192151 +/- 483.3
       rcu                        27248 +/- 30.1   194159 +/-2746.6
       atomic                     27186 +/-  3.2   200526 +/- 746.2

       rcu vs base                  +28 (+0.10%)     +2008 (+1.04%)
       atomic vs base               -34 (-0.12%)     +8374 (+4.36%)

Quoted 1 line

needs more thought.

Well the alternative is to not bother with RCU, but just wait a bit and check if the bit stuck and repeat if needed. While that could in theory livelock it is extremely unlikely (especially if you add a bit of randomization to the sleep)

Quoted 25 lines · Andi Kleen

Quoted 24 lines

However, this was a basic test, when allocating 4k pages. With 2M hugepages:

                                   insns/iter    cycles/iter
     -------------------------------------------------------
       base                       20758 +/- 12.5   191208 +/-1946.6
       rcu                        20785 +/-  3.7   197108 +/- 132.1
       atomic                     20727 +/-  6.4   204591 +/- 160.2

       rcu vs base                  +27 (+0.13%)     +5900 (+3.09%)
       atomic vs base               -31 (-0.15%)    +13383 (+7.00%)

and even with THP:

                                   insns/iter    cycles/iter
     -------------------------------------------------------
       base                       27220 +/-  2.8   192151 +/- 483.3
       rcu                        27248 +/- 30.1   194159 +/-2746.6
       atomic                     27186 +/-  3.2   200526 +/- 746.2

       rcu vs base                  +28 (+0.10%)     +2008 (+1.04%)
       atomic vs base               -34 (-0.12%)     +8374 (+4.36%)

needs more thought.

Quoted 4 lines

Well the alternative is to not bother with RCU, but just wait a bit and check if the bit stuck and repeat if needed. While that could in theory livelock it is extremely unlikely (especially if you add a bit of randomization to the sleep)

We discussed that a bit already. Hypervisors make it fairly unpredictable how long you would actually have to spin.

Signature · 3 lines

Cheers,

David

I don't like it that we are adding overhead to the good path for the benefit of memory failure, which never triggers on many systems, but I don't have a better idea. Pls take a look.

Non-atomic page flag operations (page->flags.f &= ~mask, __set_bit, __clear_bit) can race with atomic TestSetPageHWPoison() in memory_failure(). The non-atomic RMW reads flags, memory_failure() atomically sets HWPoison, then the RMW writes back the old value without HWPoison, clobbering the bit.

The race was confirmed by injecting a cpu_relax() delay between the load and store of the non-atomic RMW in __free_pages_prepare, then running concurrent MADV_HWPOISON injection. The clobbered HWPoison bit was observed repeatedly.

This series fixes the race by:

1. Having memory_failure() call synchronize_rcu() + retry after setting HWPoison, so that any in-flight non-atomic RMW that read the old flags value completes before we proceed.

2. Wrapping all non-atomic page flag operations in rcu_read_lock/rcu_read_unlock (CONFIG_MEMORY_FAILURE only), so that synchronize_rcu() actually drains them.

Performance impact (page alloc+free microbenchmark, 200K iterations, 20 runs, KVM guest, error bars are 3-sigma):

!PREEMPT_RCU (x86):

                         insns/iter      cycles/iter
    base:                12237 +/- 1     17954 +/- 136
    patched:             +22 +/- 1       -124 +/- 122
                         (+0.18%)        (within noise)

PREEMPT_RCU:

                         insns/iter      cycles/iter
    base:                12512 +/- 3     18541 +/- 214
    patched:             +95 +/- 3       -12 +/- 161
                         (+0.76%)        (within noise)

When !CONFIG_MEMORY_FAILURE, all wrappers compile away completely.

Suggested-by David Hildenbrand <[email protected]>

Quoted 3 lines · Michael S. Tsirkin

I don't like it that we are adding overhead to the good path for the benefit of memory failure, which never triggers on many systems, but I don't have a better idea. Pls take a look.

As I said on Friday.

"It's also doesn't address the mf_mutex implications and the x86 thingies I mentioned.

...

I'll either take care of that myself or find someone that can work on this with attention to all details.
"

This is nothing to vibe-code. This needs a real expert.

Quoted 32 lines

Non-atomic page flag operations (page->flags.f &= ~mask, __set_bit, __clear_bit) can race with atomic TestSetPageHWPoison() in memory_failure(). The non-atomic RMW reads flags, memory_failure() atomically sets HWPoison, then the RMW writes back the old value without HWPoison, clobbering the bit.

The race was confirmed by injecting a cpu_relax() delay between the load and store of the non-atomic RMW in __free_pages_prepare, then running concurrent MADV_HWPOISON injection. The clobbered HWPoison bit was observed repeatedly.

This series fixes the race by:

1. Having memory_failure() call synchronize_rcu() + retry after setting HWPoison, so that any in-flight non-atomic RMW that read the old flags value completes before we proceed.

2. Wrapping all non-atomic page flag operations in rcu_read_lock/rcu_read_unlock (CONFIG_MEMORY_FAILURE only), so that synchronize_rcu() actually drains them.

Performance impact (page alloc+free microbenchmark, 200K iterations, 20 runs, KVM guest, error bars are 3-sigma):

!PREEMPT_RCU (x86):

                         insns/iter      cycles/iter
    base:                12237 +/- 1     17954 +/- 136
    patched:             +22 +/- 1       -124 +/- 122
                         (+0.18%)        (within noise)

PREEMPT_RCU:

Quoted 8 lines

                         insns/iter      cycles/iter
    base:                12512 +/- 3     18541 +/- 214
    patched:             +95 +/- 3       -12 +/- 161
                         (+0.76%)        (within noise)

When !CONFIG_MEMORY_FAILURE, all wrappers compile away completely.

Suggested-by David Hildenbrand <[email protected]>

No ;)

Signature · 3 lines

Cheers,

David

Quoted 9 lines · David Hildenbrand (Arm)

Quoted 3 lines · Michael S. Tsirkin

I don't like it that we are adding overhead to the good path for the benefit of memory failure, which never triggers on many systems, but I don't have a better idea. Pls take a look.

As I said on Friday.

"It's also doesn't address the mf_mutex implications and the x86 thingies I mentioned.

Well I did attempt addressing this. These would be these two:

(a) We don't hold the mf_mutex on all call paths, but we really need it so a page_test_set_hwpoison() cannot race in weird ways with the other primitives I think.

page_test_set_hwpoison was this code you wrote:

(quoted diff) +11 -0

+static void page_set_hwpoison(struct page *page)
+{
+       lockdep_assert_held(&mf_mutex);
+
+       while (!PageHWPoison(page)) {
+               SetPageHWPoison(page);
+
+               /* Make sure concurrent non-atomic writers completed. */
+               synchronize_rcu();
+       }
+}

and indeed the test+set combination seems racy. But consider the version I posted, for example:

(quoted diff) +18 -0

+/*
+ * Drain any in-flight non-atomic page flag operations that could
+ * clobber a concurrently set HWPoison bit.  Retries until the bit sticks.
+ */
+static void set_hwpoison_drain_rcu(struct page *p)
+{
+       do {
+               synchronize_rcu();
+       } while (!TestSetPageHWPoison(p));
+}
+
 
...
 
+static bool test_and_set_hwpoison_drain_rcu(struct page *p)
+{
+       bool was_set = TestSetPageHWPoison(p);
+
+       set_hwpoison_drain_rcu(p);
+       return was_set;
+}

does not seem racy without a lock. But maybe I don't get it.

(b) There are some leftover SetPageHWPoison etc. instances. The ones in arch/x86/kernel/cpu/mce/core.c likely cannot grab the mutex, but maybe they are corner cases either way and we can document the situation.

Well, I did try to document the situation - it's in the commit log for patch 1:

Note: the MCE handler in arch/x86/kernel/cpu/mce/core.c also calls SetPageHWPoison() and is subject to the same race. It cannot use the drain helpers (MCE context cannot call synchronize_rcu()). For recoverable MCE errors, memory_failure() is queued via work items (kill_me_maybe/kill_me_never) and will re-set the bit via test_and_set_hwpoison_drain_rcu() if it was clobbered. The mce_panic() path sets HWPoison for kdump right before panic() so the race is irrelevant there. The MCG_STATUS_SEAM_NR path does not queue memory_failure(), but the affected page belongs to a TDX guest whose CPU core has already been marked dead - the page is not subject to concurrent non-atomic flag operations in the buddy allocator, so the race does not apply.

Quoted 1 line

This is nothing to vibe-code. This needs a real expert.

Well I had this sitting on the disk anyway, so I thought I'd post.

I wouldn't call this vibe-code - a bunch of manual work went into this, llms mostly as a grep/sed replacement.

But hey. I don't object to
someone taking over, for sure. Was fun, and maybe these patches will be helpful as a starting point.

In particular, maybe I should have been more explicit about how your points from Friday are addressed.

If you want to add a bit more to explain the exact concerns here, for whoever works on this next, feel free to do so.

Quoted 56 lines · Michael S. Tsirkin

Quoted 9 lines · David Hildenbrand (Arm)

Quoted 3 lines · Michael S. Tsirkin

I don't like it that we are adding overhead to the good path for the benefit of memory failure, which never triggers on many systems, but I don't have a better idea. Pls take a look.

As I said on Friday.

"It's also doesn't address the mf_mutex implications and the x86 thingies I mentioned.

Well I did attempt addressing this. These would be these two:

(a) We don't hold the mf_mutex on all call paths, but we really need it so a page_test_set_hwpoison() cannot race in weird ways with the other primitives I think.

page_test_set_hwpoison was this code you wrote:

(quoted diff) +11 -0

+static void page_set_hwpoison(struct page *page)
+{
+       lockdep_assert_held(&mf_mutex);
+
+       while (!PageHWPoison(page)) {
+               SetPageHWPoison(page);
+
+               /* Make sure concurrent non-atomic writers completed. */
+               synchronize_rcu();
+       }
+}

and indeed the test+set combination seems racy. But consider the version I posted, for example:

(quoted diff) +18 -0

+/*
+ * Drain any in-flight non-atomic page flag operations that could
+ * clobber a concurrently set HWPoison bit.  Retries until the bit sticks.
+ */
+static void set_hwpoison_drain_rcu(struct page *p)
+{
+       do {
+               synchronize_rcu();
+       } while (!TestSetPageHWPoison(p));
+}
+
 
...
 
+static bool test_and_set_hwpoison_drain_rcu(struct page *p)
+{
+       bool was_set = TestSetPageHWPoison(p);
+
+       set_hwpoison_drain_rcu(p);
+       return was_set;
+}

Quoted 1 line

does not seem racy without a lock. But maybe I don't get it.

Staring at your implementation, just think about two concurrent invocations of test_and_set_hwpoison_drain() in your code:

Assume HWPoison flag is not set.

Thread 1: test_and_set_hwpoison_drain_rcu() -> TestSetPageHWPoison()

	-> was_set = false

Thread 2: update that overwrites page->flags. HWPoison accidentally cleared.

Thread 3: test_and_set_hwpoison_drain_rcu() -> TestSetPageHWPoison()

	-> was_set = false

Thread 1: does RCU sync and returns "!was_set"
thread 2: does RCU sync and returns "!was_set"

So you could end up with two thread believing that they atomically cleared the flag, and you really need to lock.

We really have to document and enforce that the mutex is involved.

And I fear there are more nasty details to be uncovered while we rework some of this properly, mandating a detailed look.

For example, TestClearPageHWPoison() in put_page_back_buddy() likely needs a proper treatment as well.

Likely that code should be reworked entirely to not
have arbitrary hwpoison page flag modifications throughout the codebase.

Quoted 10 lines

(b) There are some leftover SetPageHWPoison etc. instances. The ones in arch/x86/kernel/cpu/mce/core.c likely cannot grab the mutex, but maybe they are corner cases either way and we can document the situation.

Well, I did try to document the situation - it's in the commit log for patch 1:

Quoted 13 lines

Note: the MCE handler in arch/x86/kernel/cpu/mce/core.c also calls SetPageHWPoison() and is subject to the same race. It cannot use the drain helpers (MCE context cannot call synchronize_rcu()). For recoverable MCE errors, memory_failure() is queued via work items (kill_me_maybe/kill_me_never) and will re-set the bit via test_and_set_hwpoison_drain_rcu() if it was clobbered. The mce_panic() path sets HWPoison for kdump right before panic() so the race is irrelevant there. The MCG_STATUS_SEAM_NR path does not queue memory_failure(), but the affected page belongs to a TDX guest whose CPU core has already been marked dead - the page is not subject to concurrent non-atomic flag operations in the buddy allocator, so the race does not apply.

We should have a central mechanism in place to document this and avoid future mistakes.

I am not even sure if we should clearly document for SetPageHWPoison() when and how they can be used, or if we need a completely new set of helpers.

And that's something to figure out (e.g., interaction with the mutex) by looking into all of the details, so I expect this to take a lot more time.

[...]

Quoted 3 lines

Quoted 1 line

This is nothing to vibe-code. This needs a real expert.

Well I had this sitting on the disk anyway, so I thought I'd post.

It would be good to coordinate here.

Like a reply to my mail, asking whether you should post a new version that you have already in place.

Quoted 3 lines

I wouldn't call this vibe-code - a bunch of manual work went into this, llms mostly as a grep/sed replacement.

The version you posted earlier had real AI vibes to it, so I can only speculate. I know that you did some manual work on this, but the details are really ugly in this code.

Quoted 6 lines

But hey. I don't object to
someone taking over, for sure. Was fun, and maybe these patches will be helpful as a starting point.

In particular, maybe I should have been more explicit about how your points from Friday are addressed.

Yes.

Quoted 3 lines

If you want to add a bit more to explain the exact concerns here, for whoever works on this next, feel free to do so.

I raised some above. I'll try to find someone to take a closer look and see to which degree we could optimize this.

Or if there are actually more performant alternatives that we could use. (I still doubt that using atomics is ok in general)

Quoted 2 lines

Or if there are actually more performant alternatives that we could use. (I still doubt that using atomics is ok in general)

Right now I'm thinking of looking at something like stop_machine maybe.

Quoted 4 lines

Quoted 2 lines

Or if there are actually more performant alternatives that we could use. (I still doubt that using atomics is ok in general)

Right now I'm thinking of looking at something like stop_machine maybe.

There can be (rare) cases where yoy get a lot of memory errors in a flood. While memory-failure doesn't need to be particularly optimized, it shouldn't be excessively slow or impact the rest of the machine.

stop_machine is a big hammer that is likely to violate most of this.

-Andi

We can maybe batch a bunch of these, and do stop machine once?

Quoted 1 line

We can maybe batch a bunch of these, and do stop machine once?

If you add a long delay you increase the risk that the machine or the critical process dies because there is an unrecoverable or process killing AR access before the page can be off lined.

(BTW that's a problem even with the RCU approach, the cure might be worse than the disease)

If you don't add a long delay you would do a lot of stop machines on a flood, likely bringing it to a halt, even though other sockets etc. might still be fine.

-Andi

And again, I'm really not sure fixing a theoretical race when memory is failing is worth slowing the world by 0.1-1% for.

From what I saw in my testing, if we allocate 4k pages it's hidden by the overhead. With hp and thp it's measureably worse than rcu on !preempt config.

[...]

Quoted 4 lines

And again, I'm really not sure fixing a theoretical race when memory is failing is worth slowing the world by 0.1-1% for.

Fully agreed. I was hoping RCU was cheaper (I mean, we were once told that RCU read side locking is essentially for free ... well in some configs :) )

The question if we could optimize it reasonably enough ...

Quoted 4 lines

From what I saw in my testing, if we allocate 4k pages it's hidden by the overhead. With hp and thp it's measureably worse than rcu on !preempt config.

... for example, by doing the rcu read lock + unlock around the

	for (i = 1; i < (1 << order); i++) {

loop on the alloc path. But I suspect it's not going to make that much of a difference.

I concluded, similar to Andi, that stop_machine() is too big of a hammer.

I wonder if something could be built out of preempt_disable() and sync SMP calls. hmm :(

Quoted 24 lines · David Hildenbrand (Arm)

[...]

Quoted 4 lines

And again, I'm really not sure fixing a theoretical race when memory is failing is worth slowing the world by 0.1-1% for.

Fully agreed. I was hoping RCU was cheaper (I mean, we were once told that RCU read side locking is essentially for free ... well in some configs :) )

The question if we could optimize it reasonably enough ...

Quoted 4 lines

From what I saw in my testing, if we allocate 4k pages it's hidden by the overhead. With hp and thp it's measureably worse than rcu on !preempt config.

... for example, by doing the rcu read lock + unlock around the

	for (i = 1; i < (1 << order); i++) {

loop on the alloc path. But I suspect it's not going to make that much of a difference.

Quoted 4 lines

I concluded, similar to Andi, that stop_machine() is too big of a hammer.

I wonder if something could be built out of preempt_disable() and sync SMP calls. hmm :(

Scrap that, shouldn't work I think ...

Quoted 19 lines · David Hildenbrand (Arm)

Quoted 18 lines · David Hildenbrand (Arm)

[...]

Fully agreed. I was hoping RCU was cheaper (I mean, we were once told that RCU read side locking is essentially for free ... well in some configs :) )

The question if we could optimize it reasonably enough ...

... for example, by doing the rcu read lock + unlock around the

	for (i = 1; i < (1 << order); i++) {

loop on the alloc path. But I suspect it's not going to make that much of a difference.

I concluded, similar to Andi, that stop_machine() is too big of a hammer.

Quoted 5 lines

Quoted 2 lines

I wonder if something could be built out of preempt_disable() and sync SMP calls. hmm :(

Scrap that, shouldn't work I think ...

Wait a sec, what about call_rcu_tasks? Use that and re-check the bit is still set?

Quoted 20 lines · Michael S. Tsirkin

Quoted 19 lines · David Hildenbrand (Arm)

Quoted 18 lines · David Hildenbrand (Arm)

[...]

Fully agreed. I was hoping RCU was cheaper (I mean, we were once told that RCU read side locking is essentially for free ... well in some configs :) )

The question if we could optimize it reasonably enough ...

... for example, by doing the rcu read lock + unlock around the

	for (i = 1; i < (1 << order); i++) {

loop on the alloc path. But I suspect it's not going to make that much of a difference.

I concluded, similar to Andi, that stop_machine() is too big of a hammer.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I wonder if something could be built out of preempt_disable() and sync SMP calls. hmm :(

Scrap that, shouldn't work I think ...

Wait a sec, what about call_rcu_tasks? Use that and re-check the bit is still set?

So, in essence the idea I had yestarday when it was late was the following:

Assume we

1) Can have a way to guarantee that a function on a CPU cannot execute within our critical section (while updating the flags)

2) We can request to execute a function on each CPU and wait for completion

I think we could just let each CPU execute our desired action (e.g., try setting the bit).

E.g.,

	local_irq_save(flags);
	page->flags &= whatever;
	local_irq_restore(flags);

And assume we want to set the bit, do a

	SetPageHWPoison(page);
	smp_call_function(set_hwpoison_smp_sync, page, 1);

whereby

	static void set_hwpoison_smp_sync(void *info)
	{
		SetPageHWPoison(page);
	}

The idea is (that needs double checking) that a CPU will execute the SetPageHWPoison() either before the local_irq_save() or after the local_irq_restore(). So it's own non-atomic update cannot get interrupted.

Now, IIUC when it comes to "how expensive is this" I think we have (cheap to expensive):

1) preempt_disable()
2) rcu_read_lock()
3) local_irq_save()

So the above wouldn't be better than an rcu-based approach we have right now. We'd need something that relies on disabled preemption only.

Huh, but I read that "anything that disables preemption also marks an RCU-sched read-side critical section including preempt_disable() and preempt_enable()".

So for our use case we should be able to use preempt_disable() instead of local_irq_save(). That should already work for your existing implementation.

Signature · 3 lines

Cheers,

David

Quoted 48 lines · David Hildenbrand (Arm)

Quoted 3 lines · Michael S. Tsirkin

Wait a sec, what about call_rcu_tasks? Use that and re-check the bit is still set?

So, in essence the idea I had yestarday when it was late was the following:

Assume we

1) Can have a way to guarantee that a function on a CPU cannot execute within our critical section (while updating the flags)

2) We can request to execute a function on each CPU and wait for completion

I think we could just let each CPU execute our desired action (e.g., try setting the bit).

E.g.,

	local_irq_save(flags);
	page->flags &= whatever;
	local_irq_restore(flags);

And assume we want to set the bit, do a

	SetPageHWPoison(page);
	smp_call_function(set_hwpoison_smp_sync, page, 1);

whereby

	static void set_hwpoison_smp_sync(void *info)
	{
		SetPageHWPoison(page);
	}

The idea is (that needs double checking) that a CPU will execute the SetPageHWPoison() either before the local_irq_save() or after the local_irq_restore(). So it's own non-atomic update cannot get interrupted.

Now, IIUC when it comes to "how expensive is this" I think we have (cheap to expensive):

1) preempt_disable()
2) rcu_read_lock()
3) local_irq_save()

Quoted 13 lines

So the above wouldn't be better than an rcu-based approach we have right now. We'd need something that relies on disabled preemption only.

Huh, but I read that "anything that disables preemption also marks an RCU-sched read-side critical section including preempt_disable() and preempt_enable()".

So for our use case we should be able to use preempt_disable() instead of local_irq_save(). That should already work for your existing implementation.

Signature · 3 lines

Cheers,

David

We have:

#else /* #ifdef CONFIG_PREEMPT_RCU */

static inline void __rcu_read_lock(void)

{
        preempt_disable();
}

...

static __always_inline void rcu_read_lock(void)
__acquires_shared(RCU)

{
        __rcu_read_lock();
        __acquire_shared(RCU);
        rcu_lock_acquire(&rcu_lock_map);
        RCU_LOCKDEP_WARN(!rcu_is_watching(),
                         "rcu_read_lock() used illegally while idle");
}

So on non-debug build witout CONFIG_PREEMPT_RCU (what I tested), rcu_lock is exactly same as preempt_disable. It's relatively cheap but not free.

preempt_disable is not going to be cheaper.

I can test if you want but it seems clear.

But IIUC task rcu might be cheaper - IIUC it does not need rcu lock/unlock at all, it relies on readers to invoke the scheduler instead.
No?

Quoted 88 lines · Michael S. Tsirkin

Quoted 48 lines · David Hildenbrand (Arm)

Quoted 3 lines · Michael S. Tsirkin

Wait a sec, what about call_rcu_tasks? Use that and re-check the bit is still set?

So, in essence the idea I had yestarday when it was late was the following:

Assume we

1) Can have a way to guarantee that a function on a CPU cannot execute within our critical section (while updating the flags)

2) We can request to execute a function on each CPU and wait for completion

I think we could just let each CPU execute our desired action (e.g., try setting the bit).

E.g.,

	local_irq_save(flags);
	page->flags &= whatever;
	local_irq_restore(flags);

And assume we want to set the bit, do a

	SetPageHWPoison(page);
	smp_call_function(set_hwpoison_smp_sync, page, 1);

whereby

	static void set_hwpoison_smp_sync(void *info)
	{
		SetPageHWPoison(page);
	}

The idea is (that needs double checking) that a CPU will execute the SetPageHWPoison() either before the local_irq_save() or after the local_irq_restore(). So it's own non-atomic update cannot get interrupted.

Now, IIUC when it comes to "how expensive is this" I think we have (cheap to expensive):

1) preempt_disable()
2) rcu_read_lock()
3) local_irq_save()

Quoted 13 lines

So the above wouldn't be better than an rcu-based approach we have right now. We'd need something that relies on disabled preemption only.

Huh, but I read that "anything that disables preemption also marks an RCU-sched read-side critical section including preempt_disable() and preempt_enable()".

So for our use case we should be able to use preempt_disable() instead of local_irq_save(). That should already work for your existing implementation.

Signature · 3 lines

Cheers,

David

We have:

#else /* #ifdef CONFIG_PREEMPT_RCU */

static inline void __rcu_read_lock(void)

{
        preempt_disable();
}

...

static __always_inline void rcu_read_lock(void)
__acquires_shared(RCU)

{
        __rcu_read_lock();
        __acquire_shared(RCU);
        rcu_lock_acquire(&rcu_lock_map);
        RCU_LOCKDEP_WARN(!rcu_is_watching(),
                         "rcu_read_lock() used illegally while idle");
}

Quoted 5 lines

So on non-debug build witout CONFIG_PREEMPT_RCU (what I tested), rcu_lock is exactly same as preempt_disable. It's relatively cheap but not free.

preempt_disable is not going to be cheaper.

Well, it will be cheaper in the general case (CONFIG_PREEMPT_RCU) :)

But yes, not for this case.

Quoted 3 lines

I can test if you want but it seems clear.

If you measured only !CONFIG_PREEMPT_RCU, then yes, it won't change a thing for that scenario.

Quoted 1 line

Quoted 4 lines

But IIUC task rcu might be cheaper - IIUC it does not need rcu lock/unlock at all, it relies on readers to invoke the scheduler instead.
No?

I thought that still requires protection of sorts (preempt_disable / rcu_read_lock), because it might fire whenever the task is preempted?

[...]

Quoted 4 lines

And again, I'm really not sure fixing a theoretical race when memory is failing is worth slowing the world by 0.1-1% for.

Fully agreed. I was hoping RCU was cheaper (I mean, we were once told that RCU read side locking is essentially for free ... well in some configs :) )

The question if we could optimize it reasonably enough ...

Quoted 4 lines

From what I saw in my testing, if we allocate 4k pages it's hidden by the overhead. With hp and thp it's measureably worse than rcu on !preempt config.

... for example, by doing the rcu read lock + unlock around the

	for (i = 1; i < (1 << order); i++) {

loop on the alloc path.

But I suspect it's not going to make that much of a difference.

I concluded, similar to Andi, that stop_machine() is too big of a hammer.

I wonder if something could be built out of preempt_disable() and sync SMP calls. hmm :(

Signature · 3 lines

Cheers,

David

Quoted 17 lines · David Hildenbrand (Arm)

[...]

Quoted 4 lines

And again, I'm really not sure fixing a theoretical race when memory is failing is worth slowing the world by 0.1-1% for.

Fully agreed. I was hoping RCU was cheaper (I mean, we were once told that RCU read side locking is essentially for free ... well in some configs :) )

The question if we could optimize it reasonably enough ...

Quoted 4 lines

From what I saw in my testing, if we allocate 4k pages it's hidden by the overhead. With hp and thp it's measureably worse than rcu on !preempt config.

Quoted 5 lines

... for example, by doing the rcu read lock + unlock around the

	for (i = 1; i < (1 << order); i++) {

loop on the alloc path.

Is this different from what this patch is doing?

Quoted 5 lines

I concluded, similar to Andi, that stop_machine() is too big of a hammer.

I wonder if something could be built out of preempt_disable() and sync SMP calls. hmm :(

rcu_lock is basically same as preempt_disable if rcu is non preemptible, no?

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines · David Hildenbrand (Arm)

[...]

Fully agreed. I was hoping RCU was cheaper (I mean, we were once told that RCU read side locking is essentially for free ... well in some configs :) )

The question if we could optimize it reasonably enough ...

Quoted 7 lines

Quoted 5 lines

... for example, by doing the rcu read lock + unlock around the

	for (i = 1; i < (1 << order); i++) {

loop on the alloc path.

Is this different from what this patch is doing?

Ah, I missed that we batch this already. We could make it include the

page_cpupid_reset_last(page);
page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;

As well, to reduce from 3 to 1 locks.

So I guess there is potential for optimization.

[...]

Quoted 8 lines

Quoted 5 lines

I concluded, similar to Andi, that stop_machine() is too big of a hammer.

I wonder if something could be built out of preempt_disable() and sync SMP calls. hmm :(

rcu_lock is basically same as preempt_disable if rcu is non preemptible, no?

Yes. See my other mail, I learned that preempt_disable() should likely just do for our use case. So the preemptible RCU case would not matter.

I assume that's as good as it gets.

1) Use preempt_disable/preempt_enable to protect
2) Batch as good as possible in the page allocator

If the overhead is then still noticeable, there is not a lot we can do to handle this cleanly I'm afraid.

Quoted 44 lines · David Hildenbrand (Arm)

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines · David Hildenbrand (Arm)

[...]

Fully agreed. I was hoping RCU was cheaper (I mean, we were once told that RCU read side locking is essentially for free ... well in some configs :) )

The question if we could optimize it reasonably enough ...

Quoted 7 lines

Quoted 5 lines

... for example, by doing the rcu read lock + unlock around the

	for (i = 1; i < (1 << order); i++) {

loop on the alloc path.

Is this different from what this patch is doing?

Ah, I missed that we batch this already. We could make it include the

page_cpupid_reset_last(page);
page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;

As well, to reduce from 3 to 1 locks.

So I guess there is potential for optimization.

[...]

Quoted 8 lines

Quoted 5 lines

I concluded, similar to Andi, that stop_machine() is too big of a hammer.

I wonder if something could be built out of preempt_disable() and sync SMP calls. hmm :(

rcu_lock is basically same as preempt_disable if rcu is non preemptible, no?

Yes. See my other mail, I learned that preempt_disable() should likely just do for our use case. So the preemptible RCU case would not matter.

I assume that's as good as it gets.

Quoted 5 lines

1) Use preempt_disable/preempt_enable to protect
2) Batch as good as possible in the page allocator

If the overhead is then still noticeable, there is not a lot we can do to handle this cleanly I'm afraid.

FWIW, there seems to be preempt_enable_no_resched() that is a bit more lightweight. I'm sure there is some downside, but it might be worth a try.

Signature · 3 lines

Cheers,

David

Quoted 3 lines

Cheers,

David

Yay. I did that + dropped the extra lock/unlock and now it's in the noise in my testing. needs much more testing of course.

If you want me to post (including addressing your other feedback) let me know.

Or look into call_rcu_tasks/call_rcu_tasks_rude which might work without changes in mm.

Or if someone else is gonnu work on this, absolutely fine too.

Quoted 6 lines

Quoted 3 lines

Cheers,

David

Yay. I did that + dropped the extra lock/unlock and now it's in the noise in my testing. needs much more testing of course.

Cool. I'd expect that latency-sensitive workloads (PREEMPT_RT) would not want to have hwpoison handling either way, so using the no_resched variants at these places might be doable.

Quoted 4 lines

If you want me to post (including addressing your other feedback) let me know.

Let's first discuss the options. We essentially have the following one so far:

1) Ignore the problem

It's been there forever ... but I am not quite happy about that.

2) Use atomics everywhere

The easiest+cleanest, but as measured, the performance hit is real.

3) Keep retrying for a couple of times

The big problem is "how long". A CPU in a hypervisor might be stalled for quite a while (20s? can be longer).

4) Disable preemption around non-atomic updates + synchronize_rcu() loop

I think it should work, but I don't like the possibly endless retry loop. (well, it would never be an endless loop in practice)

Is there a problem with synchronize_rcu() latency, given that it can take in bad scenarios a couple of seconds? (grace periods can be large ... but also very short)

4) disable local interrupts around non-atomic updates + let all CPUs perform atomic setting/clearing of the bit through smp_call_function_many().

Disabling local interrupts is way too expensive. :(

5) disable preemption around non-atomic updates + let all CPUs perform atomic setting/clearing of the bit through schedule_on_each_cpu()

Mixture of 3) + 4), but schedule_on_each_cpu() can also possibly take a long time (as long as we cannot schedule on a CPU ...).

We could likely build something that remembers all pages with bits-to-set / bits-to-clear, to then kick off a per-cpu work ... and once all CPUs processed a page it was fully processed ... needs much more thought.

6) stop_machine()

Big hammer. I think we'd still need to disable preemption around non-atomic updates.

7) Move hwpoison bit out of page->flags

Use a sparse bitmap. Quite invasive, and any hwpoison bit checking code would get more expensive.

Did I mess something up / forget something?

Quoted 2 lines

Or look into call_rcu_tasks/call_rcu_tasks_rude which might work without changes in mm.

Looking into call_rcu_tasks() (the first time) the semantics are interesting ("involuntary preemption is not a Tasks RCU quiescent state").

I'm curious how this interacts with random page allocations (on the IRQ path?), and which design you envision given that we only get a single callback (who prevents code to re-enter).

Quoted 2 lines

Or if someone else is gonnu work on this, absolutely fine too.

I can likely let someone work on that, but I think we should first figure out what can really be done. (and likely when we send it out, it should be an RFC)

Signature · 3 lines

Cheers,

David

Quoted 19 lines · David Hildenbrand (Arm)

Quoted 3 lines

Yay. I did that + dropped the extra lock/unlock and now it's in the noise in my testing. needs much more testing of course.

Cool. I'd expect that latency-sensitive workloads (PREEMPT_RT) would not want to have hwpoison handling either way, so using the no_resched variants at these places might be doable.

Quoted 4 lines

If you want me to post (including addressing your other feedback) let me know.

Let's first discuss the options. We essentially have the following one so far:

1) Ignore the problem

It's been there forever ... but I am not quite happy about that.

Quoted 8 lines

2) Use atomics everywhere

The easiest+cleanest, but as measured, the performance hit is real.

3) Keep retrying for a couple of times

The big problem is "how long". A CPU in a hypervisor might be stalled for quite a while (20s? can be longer).

So on this idea. It might not matter. What I had in mind is:
1. run the current logic
2. add page to a list of pages to check, then invoke e.g. call_rcu_tasks (or call_rcu_tasks_rude) maybe
3. in the callback, recheck and if poison cleared, go back to 1
4. otherwise everyone will see the bit set, remove from list we are done

it seems to not regress anything, and for the rare race, we set the bit eventually.

Quoted 36 lines · Michael S. Tsirkin

Quoted 19 lines · David Hildenbrand (Arm)

Quoted 3 lines

Yay. I did that + dropped the extra lock/unlock and now it's in the noise in my testing. needs much more testing of course.

Cool. I'd expect that latency-sensitive workloads (PREEMPT_RT) would not want to have hwpoison handling either way, so using the no_resched variants at these places might be doable.

Quoted 4 lines

If you want me to post (including addressing your other feedback) let me know.

Let's first discuss the options. We essentially have the following one so far:

1) Ignore the problem

It's been there forever ... but I am not quite happy about that.

Quoted 8 lines

2) Use atomics everywhere

The easiest+cleanest, but as measured, the performance hit is real.

3) Keep retrying for a couple of times

The big problem is "how long". A CPU in a hypervisor might be stalled for quite a while (20s? can be longer).

So on this idea. It might not matter. What I had in mind is:
1. run the current logic
2. add page to a list of pages to check, then invoke e.g. call_rcu_tasks (or call_rcu_tasks_rude) maybe
3. in the callback, recheck and if poison cleared, go back to 1
4. otherwise everyone will see the bit set, remove from list we are done

Quoted 3 lines

it seems to not regress anything, and for the rare race, we set the bit eventually.

So test-and-set (and friends) would also have to check the data structure that remembers bit to set/clear (and possibly update the data structure).

That does seem doable. Do you have a prototype?

Signature · 3 lines

Cheers,

David

Quoted 13 lines · David Hildenbrand (Arm)

Quoted 11 lines · Michael S. Tsirkin

So on this idea. It might not matter. What I had in mind is:
1. run the current logic
2. add page to a list of pages to check, then invoke e.g. call_rcu_tasks (or call_rcu_tasks_rude) maybe
3. in the callback, recheck and if poison cleared, go back to 1
4. otherwise everyone will see the bit set, remove from list we are done

it seems to not regress anything, and for the rare race, we set the bit eventually.

Quoted 4 lines

So test-and-set (and friends) would also have to check the data structure that remembers bit to set/clear (and possibly update the data structure).

That does seem doable. Do you have a prototype?

what do you think ;) post it?

Quoted 17 lines · Michael S. Tsirkin

Quoted 16 lines · David Hildenbrand (Arm)

Quoted 11 lines · Michael S. Tsirkin

So on this idea. It might not matter. What I had in mind is:
1. run the current logic
2. add page to a list of pages to check, then invoke e.g. call_rcu_tasks (or call_rcu_tasks_rude) maybe
3. in the callback, recheck and if poison cleared, go back to 1
4. otherwise everyone will see the bit set, remove from list we are done

it seems to not regress anything, and for the rare race, we set the bit eventually.

So test-and-set (and friends) would also have to check the data structure that remembers bit to set/clear (and possibly update the data structure).

Quoted 3 lines

Quoted 1 line

That does seem doable. Do you have a prototype?

what do you think ;) post it?

As RFC please :) [and if it's AI generated, obviously properly reviewed and reworked by you]

Signature · 3 lines

Cheers,

David

Quoted 4 lines · David Hildenbrand (Arm)

Quoted 2 lines · Michael S. Tsirkin

what do you think ;) post it?

Quoted 6 lines

As RFC please :) [and if it's AI generated, obviously properly reviewed and reworked by you]

Signature · 3 lines

Cheers,

David

Not "generated" surely. But assisted, yes.

Still hacking on it, but the difficulty
with memory-failure is that fundamentally, it's not 100% robust.

For example, we have a fifo fed by hardware and consumed by a workqueue:

        struct memory_failure_cpu *mf_cpu;
        unsigned long proc_flags;
        bool buffer_overflow;
        struct memory_failure_entry entry = {
                .pfn =          pfn,
                .flags =        flags,
        };

        mf_cpu = &get_cpu_var(memory_failure_cpu);
        raw_spin_lock_irqsave(&mf_cpu->lock, proc_flags);
        buffer_overflow = !kfifo_put(&mf_cpu->fifo, entry);
        if (!buffer_overflow)
                schedule_work_on(smp_processor_id(), &mf_cpu->work);
        raw_spin_unlock_irqrestore(&mf_cpu->lock, proc_flags);
        put_cpu_var(memory_failure_cpu);
        if (buffer_overflow)
                pr_err("buffer overflow when queuing memory failure at %#lx\n",
                       pfn);

if there are lots of these and the scheduler is slow and it overflows, it's sayonara you have lost the flag, right?

Oh and by the way, I just noticed that when buddy merges pages it does not check the poison bit. So it looks like there's a simple way to lose the poison bit - have it merge with a non poisoned page.

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · David Hildenbrand (Arm)

Quoted 2 lines · Michael S. Tsirkin

what do you think ;) post it?

Quoted 8 lines

Quoted 6 lines

As RFC please :) [and if it's AI generated, obviously properly reviewed and reworked by you]

Signature · 3 lines

Cheers,

David

Not "generated" surely. But assisted, yes.

What I thought.

Quoted 2 lines

Still hacking on it, but the difficulty
with memory-failure is that fundamentally, it's not 100% robust.

It's all a bit slapped on top of everything, yes.

What I was wondering is, assuming the call_task_rcu() and it takes forever, there might be quite a while where a hwpoisoned page that lost its bit is not marked as hwpoisoned.

So you'd actually want the one doing the test_and_set_bit() caller to wait until the bit is stable.

But that should be rather hairy as well. :(

Quoted 23 lines

For example, we have a fifo fed by hardware and consumed by a workqueue:

        struct memory_failure_cpu *mf_cpu;
        unsigned long proc_flags;
        bool buffer_overflow;
        struct memory_failure_entry entry = {
                .pfn =          pfn,
                .flags =        flags,
        };

        mf_cpu = &get_cpu_var(memory_failure_cpu);
        raw_spin_lock_irqsave(&mf_cpu->lock, proc_flags);
        buffer_overflow = !kfifo_put(&mf_cpu->fifo, entry);
        if (!buffer_overflow)
                schedule_work_on(smp_processor_id(), &mf_cpu->work);
        raw_spin_unlock_irqrestore(&mf_cpu->lock, proc_flags);
        put_cpu_var(memory_failure_cpu);
        if (buffer_overflow)
                pr_err("buffer overflow when queuing memory failure at %#lx\n",
                       pfn);

Quoted 2 lines

if there are lots of these and the scheduler is slow and it overflows, it's sayonara you have lost the flag, right?

I guess so. I assume on relevant hw you wouldn't expect a storm. But who knows :)

Quoted 2 lines

Quoted 3 lines

Oh and by the way, I just noticed that when buddy merges pages it does not check the poison bit. So it looks like there's a simple way to lose the poison bit - have it merge with a non poisoned page.

When we poison, we try to take the free page off the buddy. At least that's what I remember.

So I think we would just then go ahead and split the free higher-order buddy page to remove the single page.

Signature · 4 lines

-- 
Cheers,

David

Quoted 11 lines · David Hildenbrand (Arm)

Quoted 2 lines · Michael S. Tsirkin

Not "generated" surely. But assisted, yes.

What I thought.

Quoted 2 lines

Still hacking on it, but the difficulty
with memory-failure is that fundamentally, it's not 100% robust.

It's all a bit slapped on top of everything, yes.

Quoted 6 lines

What I was wondering is, assuming the call_task_rcu() and it takes forever, there might be quite a while where a hwpoisoned page that lost its bit is not marked as hwpoisoned.

So you'd actually want the one doing the test_and_set_bit() caller to wait until the bit is stable.

not sure I get it.

Quoted 33 lines

But that should be rather hairy as well. :(

Quoted 23 lines

For example, we have a fifo fed by hardware and consumed by a workqueue:

        struct memory_failure_cpu *mf_cpu;
        unsigned long proc_flags;
        bool buffer_overflow;
        struct memory_failure_entry entry = {
                .pfn =          pfn,
                .flags =        flags,
        };

        mf_cpu = &get_cpu_var(memory_failure_cpu);
        raw_spin_lock_irqsave(&mf_cpu->lock, proc_flags);
        buffer_overflow = !kfifo_put(&mf_cpu->fifo, entry);
        if (!buffer_overflow)
                schedule_work_on(smp_processor_id(), &mf_cpu->work);
        raw_spin_unlock_irqrestore(&mf_cpu->lock, proc_flags);
        put_cpu_var(memory_failure_cpu);
        if (buffer_overflow)
                pr_err("buffer overflow when queuing memory failure at %#lx\n",
                       pfn);

Quoted 2 lines

if there are lots of these and the scheduler is slow and it overflows, it's sayonara you have lost the flag, right?

I guess so. I assume on relevant hw you wouldn't expect a storm. But who knows :)

Quoted 2 lines

Quoted 6 lines

Quoted 3 lines

Oh and by the way, I just noticed that when buddy merges pages it does not check the poison bit. So it looks like there's a simple way to lose the poison bit - have it merge with a non poisoned page.

When we poison, we try to take the free page off the buddy. At least that's what I remember.

Yes but not immediately - we set hwpoison then we try to take it off.

Quoted 2 lines

So I think we would just then go ahead and split the free higher-order buddy page to remove the single page.

Later we split, yes. But I don't see where it sets HWPoison after split - it calls
SetPageHWPoisonTakenOff but it seems to assume HWPoison is already set.

Am I missing something? It's late here...

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · David Hildenbrand (Arm)

Quoted 2 lines · Michael S. Tsirkin

Not "generated" surely. But assisted, yes.

What I thought.

Quoted 2 lines

Still hacking on it, but the difficulty
with memory-failure is that fundamentally, it's not 100% robust.

It's all a bit slapped on top of everything, yes.

Quoted 8 lines

Quoted 6 lines

What I was wondering is, assuming the call_task_rcu() and it takes forever, there might be quite a while where a hwpoisoned page that lost its bit is not marked as hwpoisoned.

So you'd actually want the one doing the test_and_set_bit() caller to wait until the bit is stable.

not sure I get it.

Essentially, I think that after you set the bit you'd want everybody else in the system to see that state before you continue.

Such that other code that tests for poisoned pages would just immediately act on it. Like the buddy not handing out such a page anymore, immediately.

At least that way it's easier to reason about. Losing these bits is really just nasty :(

Quoted 47 lines

Quoted 33 lines

But that should be rather hairy as well. :(

Quoted 23 lines

For example, we have a fifo fed by hardware and consumed by a workqueue:

        struct memory_failure_cpu *mf_cpu;
        unsigned long proc_flags;
        bool buffer_overflow;
        struct memory_failure_entry entry = {
                .pfn =          pfn,
                .flags =        flags,
        };

        mf_cpu = &get_cpu_var(memory_failure_cpu);
        raw_spin_lock_irqsave(&mf_cpu->lock, proc_flags);
        buffer_overflow = !kfifo_put(&mf_cpu->fifo, entry);
        if (!buffer_overflow)
                schedule_work_on(smp_processor_id(), &mf_cpu->work);
        raw_spin_unlock_irqrestore(&mf_cpu->lock, proc_flags);
        put_cpu_var(memory_failure_cpu);
        if (buffer_overflow)
                pr_err("buffer overflow when queuing memory failure at %#lx\n",
                       pfn);

Quoted 2 lines

if there are lots of these and the scheduler is slow and it overflows, it's sayonara you have lost the flag, right?

I guess so. I assume on relevant hw you wouldn't expect a storm. But who knows :)

Quoted 2 lines

Quoted 6 lines

Quoted 3 lines

Oh and by the way, I just noticed that when buddy merges pages it does not check the poison bit. So it looks like there's a simple way to lose the poison bit - have it merge with a non poisoned page.

When we poison, we try to take the free page off the buddy. At least that's what I remember.

Yes but not immediately - we set hwpoison then we try to take it off.

Quoted 2 lines

So I think we would just then go ahead and split the free higher-order buddy page to remove the single page.

Quoted 5 lines

Later we split, yes. But I don't see where it sets HWPoison after split - it calls
SetPageHWPoisonTakenOff but it seems to assume HWPoison is already set.

Am I missing something? It's late here...

So, we set hwpoison on a page and that is sticky, even when the buddy merges them, right?

When handing out pages, the buddy will check all pages through check_new_pages(). If the poisoned page is part of a higher-order page, that would still find it.

So I would assume that the hwpoison page->flags stays there, and PageHWPoisonTakenOff() only tells us if we succeeded in removing the page from the buddy ourselves.

Signature · 4 lines

-- 
Cheers,

David

Quoted 17 lines · Michael S. Tsirkin

Quoted 16 lines · David Hildenbrand (Arm)

Quoted 11 lines · Michael S. Tsirkin

So on this idea. It might not matter. What I had in mind is:
1. run the current logic
2. add page to a list of pages to check, then invoke e.g. call_rcu_tasks (or call_rcu_tasks_rude) maybe
3. in the callback, recheck and if poison cleared, go back to 1
4. otherwise everyone will see the bit set, remove from list we are done

it seems to not regress anything, and for the rare race, we set the bit eventually.

So test-and-set (and friends) would also have to check the data structure that remembers bit to set/clear (and possibly update the data structure).

Quoted 3 lines

Quoted 1 line

That does seem doable. Do you have a prototype?

what do you think ;) post it?

As RFC please :) [and if it's AI generated, obviously properly reviewed and reworked by you]

Signature · 3 lines

Cheers,

David

Quoted 6 lines · David Hildenbrand (Arm)

Quoted 2 lines · Michael S. Tsirkin

what do you think ;) post it?

As RFC please :) [and if it's AI generated, obviously properly reviewed and reworked by you]

I spent a bit poking at it, and it's hairy.

The issue is that we have calls to both set and clear bits.
When we record them in a side side structure so they can be retried, we have to carefully record both sets and clears and their order and make sure they do not bypass each other. So if there's set already in the structure, we need to take it out of structure and only then add a clear.

It gets very hairy because we want to set from NMI. So now we can have clear in the data structure and set from NMI bypasses that. Now what? And set from NMI can't be 100% safe anyway as we discussed ... Kinda reduced my motivation to try and make this reliable at all.

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · David Hildenbrand (Arm)

Quoted 2 lines · Michael S. Tsirkin

what do you think ;) post it?

As RFC please :) [and if it's AI generated, obviously properly reviewed and reworked by you]

I spent a bit poking at it, and it's hairy.

What I thought ...

Quoted 10 lines

The issue is that we have calls to both set and clear bits.
When we record them in a side side structure so they can be retried, we have to carefully record both sets and clears and their order and make sure they do not bypass each other. So if there's set already in the structure, we need to take it out of structure and only then add a clear.

It gets very hairy because we want to set from NMI. So now we can have clear in the data structure and set from NMI bypasses that. Now what? And set from NMI can't be 100% safe anyway as we discussed ... Kinda reduced my motivation to try and make this reliable at all.

Maybe we should just really keep it as broken as it currently is. It's not really worth our time fixing basic design flaws ... given that we didn't have any report about these races so far :/

I mean, sure, if it's easy, ... but it sounds like getting more and more complex.

Signature · 3 lines

Cheers,

David

Quoted 23 lines · David Hildenbrand (Arm)

Quoted 2 lines · Michael S. Tsirkin

I spent a bit poking at it, and it's hairy.

What I thought ...

Quoted 10 lines

The issue is that we have calls to both set and clear bits.
When we record them in a side side structure so they can be retried, we have to carefully record both sets and clears and their order and make sure they do not bypass each other. So if there's set already in the structure, we need to take it out of structure and only then add a clear.

It gets very hairy because we want to set from NMI. So now we can have clear in the data structure and set from NMI bypasses that. Now what? And set from NMI can't be 100% safe anyway as we discussed ... Kinda reduced my motivation to try and make this reliable at all.

Maybe we should just really keep it as broken as it currently is. It's not really worth our time fixing basic design flaws ... given that we didn't have any report about these races so far :/

I mean, sure, if it's easy, ... but it sounds like getting more and more complex.

Quoted 4 lines

Signature · 3 lines

Cheers,

David

Right. Either we do the RCU thing or let it be. But then I'm uninclined to even bother with fixing other weird behaviour I see there.

[...]

Quoted 4 lines

And again, I'm really not sure fixing a theoretical race when memory is failing is worth slowing the world by 0.1-1% for.

Fully agreed.

I was hoping RCU was cheaper (I mean, we were once told that RCU read side locking is essentially for free ... well in some configs :) )

Quoted 7 lines · David Hildenbrand (Arm)

[...]

Quoted 4 lines

And again, I'm really not sure fixing a theoretical race when memory is failing is worth slowing the world by 0.1-1% for.

Quoted 1 line

Fully agreed.

Let's say, we just send every bad page through MF machinery X times, and hope the chances of a race are reduced enough not to matter. WDYT?

It would be good to coordinate here.

Quoted 1 line · David Hildenbrand (Arm)

It would be good to coordinate here.

Case in point, do you feel this should be blocking the zeroing/page reporting work (which adds more non atomic RMWs), or I can work more on that putting this aside?