Message-ID: <3d595920-d3c6-4b68-88fa-3b5639207121@oracle.com>
Date: Wed, 27 Aug 2025 01:06:16 -0700
From: jane.chu@...cle.com
To: Jiaqi Yan <jiaqiyan@...gle.com>, Kyle Meyer <kyle.meyer@....com>,
        Miaohe Lin <linmiaohe@...wei.com>, russ.anderson@....com
Cc: akpm@...ux-foundation.org, david@...hat.com, tony.luck@...el.com,
        bp@...en8.de, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        linux-edac@...r.kernel.org, lorenzo.stoakes@...cle.com,
        Liam.Howlett@...cle.com, vbabka@...e.cz, rppt@...nel.org,
        surenb@...gle.com, mhocko@...e.com, nao.horiguchi@...il.com,
        osalvador@...e.de
Subject: Re: [PATCH] mm/memory-failure: Do not call action_result() on already
 poisoned pages



On 8/26/2025 2:22 PM, Jiaqi Yan wrote:
> On Tue, Aug 26, 2025 at 12:28 PM Kyle Meyer <kyle.meyer@....com> wrote:
>>
>> On Tue, Aug 26, 2025 at 10:24:07AM -0700, jane.chu@...cle.com wrote:
>>>
>>> On 8/25/2025 6:56 PM, Kyle Meyer wrote:
>>>> On Mon, Aug 25, 2025 at 03:36:54PM -0700, jane.chu@...cle.com wrote:
>>>>> On 8/25/2025 9:09 AM, Kyle Meyer wrote:
>>>>>> On Mon, Aug 25, 2025 at 11:04:43AM +0800, Miaohe Lin wrote:
>>>>>>> On 2025/8/22 8:24, Jiaqi Yan wrote:
>>>>>>>> On Thu, Aug 21, 2025 at 12:36 PM Kyle Meyer <kyle.meyer@....com> wrote:
>>>>>>>>>
>>>>>>>>> On Thu, Aug 21, 2025 at 11:23:48AM -0700, Jiaqi Yan wrote:
>>>>>>>>>> On Thu, Aug 21, 2025 at 9:46 AM Kyle Meyer <kyle.meyer@....com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Calling action_result() on already poisoned pages causes issues:
>>>>>>>>>>>
>>>>>>>>>>> * The amount of hardware corrupted memory is incorrectly incremented.
>>>>>>>>>>> * NUMA node memory failure statistics are incorrectly updated.
>>>>>>>>>>> * Redundant "already poisoned" messages are printed.
>>>>>
>>>>> Assuming this means that the numbers reported from
>>>>>     /sys/devices/system/node/node*/memory_failure/*
>>>>> do not match certain expectation, right?
>>>>>
>>>>> If so, could you clarify what is the expectation?
>>>>
>>>> Sure, and please let me know if I'm mistaken.
>>>>
>>>> Here's the description of total:
>>>>
>>>> What:               /sys/devices/system/node/nodeX/memory_failure/total
>>>> Date:               January 2023
>>>> Contact:    Jiaqi Yan <jiaqiyan@...gle.com>
>>>> Description:
>>>>              The total number of raw poisoned pages (pages containing
>>>>              corrupted data due to memory errors) on a NUMA node.
>>>>
>>>> That should emit the number of poisoned pages on NUMA node X. That's
>>>> incremented each time update_per_node_mf_stats() is called.
>>>>
>>>> Here's the description of failed:
>>>>
>>>> What:               /sys/devices/system/node/nodeX/memory_failure/failed
>>>> Date:               January 2023
>>>> Contact:    Jiaqi Yan <jiaqiyan@...gle.com>
>>>> Description:
>>>>              Of the raw poisoned pages on a NUMA node, how many pages are
>>>>              failed by memory error recovery attempt. This usually means
>>>>              a key recovery operation failed.
>>>>
>>>> That should emit the number of poisoned pages on NUMA node X that could
>>>> not be recovered because the attempt failed. That's incremented each time
>>>> update_per_node_mf_stats() is called with MF_FAILED.
>>>>
>>>> We're currently calling action_result() with MF_FAILED each time we encounter
>>>> a poisoned page (note: the huge page path is a bit different, we only call
>>>> action_result() with MF_FAILED when MF_ACTION_REQUIRED is set). That, IMO,
>>>> breaks the descriptions. We already incremented the per NUMA node MF statistics
>>>> to account for that poisoned page.
>>>
>>> Thanks!  My reading is that these numbers are best as hints, I won't take
>>> them literately.  As you alluded below, kill_accessing_process is applied
>>> only if MF_ACTION_REQUIRED is set, though the page is already marked
>>> poisoned.  Besides, there can be bug that renders a poisoned page not being
>>> properly isolated nor being properly categorized.  If you're looking for
>>> something precise, is there another way? maybe from firmware?
>>
>> Firmware records the number of memory errors that have been detected and
>> reported, but it doesn't record how Linux responded to those memory errors.
>>
>> Checking the ring buffer, the amount of hardware corrupted memory, and the
>> per NUMA node memory failure statistics is a simple way to determine how Linux
>> responded.
>>
>> Since commit b8b9488d50b7, that has become unreliable. The same memory error
>> may be reported by multiple sources and now each report increments the amount of
> 
> To me this multiple counting is an existing issue, and what Kyle
> originally targeted. It seems to me that as long as the
> mf_action_page_type is MF_MSG_ALREADY_POISONED (or HWPoison flag is
> set), action_result shouldn't invoke statistic related ops, and
> mf_results just become irrelevant (wrt
> /sys/devices/system/node/nodeX/memory_failure/*), right? IWO, we can
> decouple the two issues:
> 1. how to update statistics correctly?
> 2. what log msg to show for recovery result, MF_FAILED or something else/new?
> 
> While Kyle fixing issue #1, can we just keep MF_FAILED as is?
> 

Sounds good to me.

thanks,
-jane

>> hardware corrupted memory and the per NUMA node memory failure statistics. Isn't
>> that a regression?
>>
>> The per NUMA node memory failure statistics might not always be 100% accurate,
>> but this issue seems preventable.
>>
>>>>>>>>>>
>>>>>>>>>> All agreed.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Do not call action_result() on already poisoned pages and drop unused
>>>>>>>>>>> MF_MSG_ALREADY_POISONED.
>>>>>>>>>>
>>>>>>>>>> Hi Kyle,
>>>>>>>>>>
>>>>>>>>>> Patch looks great to me, just one thought...
>>>>>>>
>>>>>>> Thanks both.
>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Alternatively, have you thought about keeping MF_MSG_ALREADY_POISONED
>>>>>>>>>> but changing action_result for MF_MSG_ALREADY_POISONED?
>>>>>>>>>> - don't num_poisoned_pages_inc(pfn)
>>>>>>>>>> - don't update_per_node_mf_stats(pfn, result)
>>>>>>>>>> - still pr_err("%#lx: recovery action for %s: %s\n", ...)
>>>>>>>>>> - meanwhile remove "pr_err("%#lx: already hardware poisoned\n", pfn)"
>>>>>>>>>> in memory_failure and try_memory_failure_hugetlb
>>>>>>>>>
>>>>>>>>> I did consider that approach but I was concerned about passing
>>>>>>>>> MF_MSG_ALREADY_POISONED to action_result() with MF_FAILED. The message is a
>>>>>>>>> bit misleading.
>>>>>>>>
>>>>>>>> Based on my reading the documentation for MF_* in static const char
>>>>>>>> *action_name[]...
>>>>>>>>
>>>>>>>> Yeah, for file mapped pages, kernel may not have hole-punched or
>>>>>>>> truncated it from the file mapping (shmem and hugetlbfs for example)
>>>>>>>> but that still considered as MF_RECOVERED, so touching a page with
>>>>>>>> HWPoison flag doesn't mean that page was failed to be recovered
>>>>>>>> previously.
>>>>>>>>
>>>>>>>> For pages intended to be taken out of the buddy system, touching a
>>>>>>>> page with HWPoison flag does imply it isn't isolated and hence
>>>>>>>> MF_FAILED.
>>>>>>>
>>>>>>> There should be other cases that memory_failure failed to isolate the
>>>>>>> hwpoisoned pages at first time due to various reasons.
>>>>>>>
>>>>>>>>
>>>>>>>> In summary, seeing the HWPoison flag again doesn't necessarily
>>>>>>>> indicate what the recovery result was previously; it only indicate
>>>>>>>> kernel won't re-attempt to recover?
>>>>>>>
>>>>>>> Yes, kernel won't re-attempt to or just cannot recover.
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> How about introducing a new MF action result? Maybe MF_NONE? The message could
>>>>>>>>> look something like:
>>>>>>>>
>>>>>>>> Adding MF_NONE sounds fine to me, as long as we correctly document its
>>>>>>>> meaning, which can be subtle.
>>>>>>>
>>>>>>> Adding a new MF action result sounds good to me. But IMHO MF_NONE might not be that suitable
>>>>>>> as kill_accessing_process might be called to kill proc in this case, so it's not "NONE".
>>>>>>
>>>>>> OK, would you like a separate MF action result for each case? Maybe
>>>>>> MF_ALREADY_POISONED and MF_ALREADY_POISONED_KILLED?
>>>>>>
>>>>>> MF_ALREADY_POISONED can be the default and MF_ALREADY_POISONED_KILLED can be
>>>>>> used when kill_accessing_process() returns -EHWPOISON.
>>>>>>
>>>>>> The log messages could look like...
>>>>>>
>>>>>> Memory failure: 0xXXXXXXXX: recovery action for already poisoned page: None
>>>>>>          and
>>>>>> Memory failure: 0xXXXXXXXX: recovery action for already poisoned page: Process killed
>>>>>
>>>>> Agreed with Miaohe that "None" won't work.
>>>>
>>>> What action is M-F() taking to recover already poisoned pages that don't have
>>>> MF_ACTION_REQUIRED set?
>>>
>>> The action taken toward poisoned page not under MF_ACTION_REQUIRED is
>>> typically isolation, that is, remove the pte or mark the pte as poisoned
>>> special swap entry, so that subsequent page fault is given a chance to
>>> deliver a SIGBUS. That said, things might fail during the process, like
>>> encountering GUP pinned THP page.>
>>>>> "Process killed" sounds okay for MF_MSG_ALREADY_POISONED, but
>>>>> we need to understand why "Failed" doesn't work for your usecase.
>>>>> "Failed" means process is killed but page is not successfully isolated which
>>>>> applies to MF_MSG_ALREADY_POISONED case as well.
>>>>
>>>> So that accessing process is killed. Why call action_result() with MF_FAILED?
>>>> Doesn't that indicate we poisoned another page and the recovery attempt failed?
>>>
>>> What I recall is that, "recovery" is reserved for page that is clean,
>>> isolated, and even by chance, unmapped.  "failed" is reserved for page that
>>> has been(or might not?) removed from the page table, page might be dirty,
>>> certainly mapped, etc. A SIGBUS doesn't make recovery an automatic success.
>>>
>>> Others please correct me if I'm mistaken.
>>
>> Thank you very much for taking the time to explain everything.
>>
>> Thanks,
>> Kyle Meyer

Powered by blists - more mailing lists