On 2026/6/15 18:54, David Hildenbrand (Arm) wrote:
> On 6/15/26 05:29, Miaohe Lin wrote:
>> On 2026/6/11 21:20, David Hildenbrand (Arm) wrote:
>>> On 6/11/26 09:36, Miaohe Lin wrote:
>>>>
>>>> Agree, it's not worth to do so.
>>>>
>>>>
>>>> Since memory_failure might be the only place, this change would be
>>>> unacceptable.
>>>> We should come up with a better solution. Maybe we can try repeating
>>>> SetPageHWPoison
>>>> and ClearPageHWPoison at a first attempt though it looks somewhat weird to
>>>> me and makes
>>>> code more complicated.
>>>
>>> And I am fairly sure we could still have some remaining races ... it's
>>> shaky.
>>
>> I have to agree it's shaky.
>
> Right, just let writing task reschedule after reading the flags,
> but before writing the flags.
>
>> Any suggestion for next step?
>
> We have various code that assumes that no concurrent writes are
> possible, and consequently, we use no atomics.
>
> __free_pages_prepare() is just one user.
>
> Then we have __folio_set_locked(), __folio_clear_active()
> and __folio_clear_unevictable().
>
> But also __folio_mark_uptodate(), which is called rather frequently.
>
> page_cpupid_reset_last() is also a thing, but it mostly falls
> under __free_pages_prepare() handling.
>
> ... and __split_folio_to_order() also messes with flags directly without
> atomics.
>
>
> Many of these are only possible for frozen pages (refcount == 0). I think
> only __folio_set_locked() and __folio_mark_uptodate() are called on
> non-frozen pages, when there is the expectation that nobody will concurrently
> use atomics that would be bad (e.g., don't trylock if not an lru page).
>
Thanks David! This information is really helpful!
>
> We don't want to use atomics at these places just to please memory failure
> code.
Bad news. We have more places racing with memory failure code.
>
> Would it be sufficient to know in memory-failure code that concurrent
> handling succeeded?
I think so, that would be useful.
>
>
> Assume that we enlighten all non-atomics to grab the rcu read lock, such as
These non-atomics are defined and used because they want to avoid atomic ops
overhead?
So I'm afraid using rcu read lock in these places would lead to unexpected
overhead.
>
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 7223f6f4e2b4..3c3852b60bbd 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -803,10 +803,30 @@ static inline bool PageUptodate(const struct page *page)
> return folio_test_uptodate(page_folio(page));
> }
>
> +#ifdef CONFIG_MEMORY_FAILURE
> +static inline void page_flags_modify_nonatomic_begin(void)
> +{
> + rcu_read_lock();
> +}
> +static inline void page_flags_modify_nonatomic_end(void)
> +{
> + rcu_read_unlock();
> +}
> +#else
> +static inline void page_flags_modify_nonatomic_begin(void)
> +{
> +}
> +static inline void page_flags_modify_nonatomic_end(void)
> +{
> +}
> +#endif
> +
> static __always_inline void __folio_mark_uptodate(struct folio *folio)
> {
> smp_wmb();
> + page_flags_modify_nonatomic_begin();
> __set_bit(PG_uptodate, folio_flags(folio, 0));
> + page_flags_modify_nonatomic_end();
> }
>
>
> And then we have some retry logic such as:
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 51508a55c405..1123c40aaf43 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -162,6 +162,62 @@ static struct rb_root_cached pfn_space_itree =
> RB_ROOT_CACHED;
>
> static DEFINE_MUTEX(pfn_space_lock);
>
> +static bool page_test_set_hwpoison(struct page *page)
> +{
> + lockdep_assert_held(&mf_mutex);
> +
> + while (true) {
> + /* Already set -> not our problem. */
> + if (TestSetPageHWPoison(page))
> + return true;
> + /* Make sure concurrent non-atomic writers completed. */
> + synchronize_rcu();
> + /* Setting the flag was sticky. */
> + if (PageHWPoison(page))
> + return false;
> + }
> +}
> +
> +static bool page_test_clear_hwpoison(struct page *page)
> +{
> + lockdep_assert_held(&mf_mutex);
> +
> + while (true) {
> + /* Already clear -> not our problem. */
> + if (!TestClearPageHWPoison(page))
> + return false;
> + /* Make sure concurrent non-atomic writers completed. */
> + synchronize_rcu();
> + /* Clearing the flag was sticky. */
> + if (!PageHWPoison(page))
> + return true;
> + }
> +}
> +
> +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();
> + }
> +}
> +
> +static void page_clear_hwpoison(struct page *page)
> +{
> + lockdep_assert_held(&mf_mutex);
> +
> + while (PageHWPoison(page)) {
> + ClearPageHWPoison(page);
> +
> + /* Make sure concurrent non-atomic writers completed. */
> + synchronize_rcu();
> + }
> +}
> +
> /*
> * Return values:
> * 1: the page is dissolved (if needed) and taken off from buddy,
> @@ -199,7 +255,7 @@ static bool page_handle_poison(struct page *page, bool
> hugepage_or_freepage, boo
> return false;
> }
>
> - SetPageHWPoison(page);
> + page_set_hwpoison(page);
> if (release)
> put_page(page);
> page_ref_inc(page);
> @@ -1744,7 +1800,7 @@ static int mf_generic_kill_procs(unsigned long long
> pfn, int flags,
> * Use this flag as an indication that the dax page has been
> * remapped UC to prevent speculative consumption of poison.
> */
> - SetPageHWPoison(&folio->page);
> + page_set_hwpoison(&folio->page);
>
> /*
> * Unlike System-RAM there is no possibility to swap in a
> @@ -1789,7 +1845,7 @@ int mf_dax_kill_procs(struct address_space *mapping,
> pgoff_t index,
> goto unlock;
>
> if (!pre_remove)
> - SetPageHWPoison(page);
> + page_set_hwpoison(page);
>
> /*
> * The pre_remove case is revoking access, the memory is still
> @@ -1866,7 +1922,7 @@ static unsigned long __folio_free_raw_hwp(struct folio
> *folio, bool move_flag)
> head = llist_del_all(raw_hwp_list_head(folio));
> llist_for_each_entry_safe(p, next, head, node) {
> if (move_flag)
> - SetPageHWPoison(p->page);
> + page_set_hwpoison(p->page);
> else
> num_poisoned_pages_sub(page_to_pfn(p->page), 1);
> kfree(p);
> @@ -2380,7 +2436,7 @@ int memory_failure(unsigned long pfn, int flags)
> if (res != -ENOENT)
> goto unlock_mutex;
>
> - if (TestSetPageHWPoison(p)) {
> + if (page_test_set_hwpoison(p)) {
> res = -EHWPOISON;
> if (flags & MF_ACTION_REQUIRED)
> res = kill_accessing_process(current, pfn, flags);
> @@ -2410,7 +2466,7 @@ int memory_failure(unsigned long pfn, int flags)
> } else {
> /* We lost the race, try again */
> if (retry) {
> - ClearPageHWPoison(p);
> + page_clear_hwpoison(p);
> retry = false;
> goto try_again;
> }
> @@ -2431,7 +2487,7 @@ int memory_failure(unsigned long pfn, int flags)
> /* filter pages that are protected from hwpoison test by users */
> folio_lock(folio);
> if (hwpoison_filter(p)) {
> - ClearPageHWPoison(p);
> + page_clear_hwpoison(p);
> folio_unlock(folio);
> folio_put(folio);
> res = -EOPNOTSUPP;
> @@ -2751,7 +2807,7 @@ int unpoison_memory(unsigned long pfn)
> }
>
> folio_put(folio);
> - if (TestClearPageHWPoison(p)) {
> + if (page_test_clear_hwpoison(p)) {
> folio_put(folio);
> ret = 0;
> }
>
>
> Maybe that would work. There would still be issues to solve
>
> (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.
>
> (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.
>
>
> Further, while I assume the synchronize_rcu() on the MCE path should be fine
> (who cares about performance there?), I don't know if the added RCU read lock
> on some paths could be noticable.
>
> So one idea worth discussing, but I am sure there are more problems.
I think this is a good idea, although there are some remaining issues.
But such race should be really rare, is it worth all this effort? Could we
simply aim to resolve, not to be flawless? I.e. could we simply check
and re-set the hwpoison flag at the end of memory_failure handling to
simply avoid losing hwpoison flag as a best-effort attempt? Would it be
acceptable?
Thanks.
.
