Re: [PATCH v3 1/3] mm/memory-failure: userspace controls soft-offlining pages

[Miaohe Lin <linmiaohe@huawei.com>]

On 2024/6/18 7:17, Jiaqi Yan wrote:
> On Mon, Jun 17, 2024 at 12:13 PM Andrew Morton
> <akpm@linux-foundation.org> wrote:
>>
>> On Mon, 17 Jun 2024 17:05:43 +0000 Jiaqi Yan <jiaqiyan@google.com> wrote:
>>
>>> Correctable memory errors are very common on servers with large
>>> amount of memory, and are corrected by ECC. Soft offline is kernel's
>>> additional recovery handling for memory pages having (excessive)
>>> corrected memory errors. Impacted page is migrated to a healthy page
>>> if it is in-use; the original page is discarded for any future use.
>>>
>>> The actual policy on whether (and when) to soft offline should be
>>> maintained by userspace, especially in case of an 1G HugeTLB page.
>>> Soft-offline dissolves the HugeTLB page, either in-use or free, into
>>> chunks of 4K pages, reducing HugeTLB pool capacity by 1 hugepage.
>>> If userspace has not acknowledged such behavior, it may be surprised
>>> when later failed to mmap hugepages due to lack of hugepages.
>>> In case of a transparent hugepage, it will be split into 4K pages
>>> as well; userspace will stop enjoying the transparent performance.
>>>
>>> In addition, discarding the entire 1G HugeTLB page only because of
>>> corrected memory errors sounds very costly and kernel better not
>>> doing under the hood. But today there are at least 2 such cases
>>> doing so:
>>> 1. GHES driver sees both GHES_SEV_CORRECTED and
>>>    CPER_SEC_ERROR_THRESHOLD_EXCEEDED after parsing CPER.
>>> 2. RAS Correctable Errors Collector counts correctable errors per
>>>    PFN and when the counter for a PFN reaches threshold
>>> In both cases, userspace has no control of the soft offline performed
>>> by kernel's memory failure recovery.
>>>
>>> This commit gives userspace the control of softofflining any page:
>>> kernel only soft offlines raw page / transparent hugepage / HugeTLB
>>> hugepage if userspace has agreed to. The interface to userspace is a
>>> new sysctl at /proc/sys/vm/enable_soft_offline. By default its value
>>> is set to 1 to preserve existing behavior in kernel. When set to 0,
>>> soft-offline (e.g. MADV_SOFT_OFFLINE) will fail with EOPNOTSUPP.
>>>
>>
>> Seems reasonable.  A very simple patch.
> 
> Thanks for taking a look, Andrew!
> 
>>
>> Is there sufficient instrumentation in place for userspace to be able
>> to know that these errors are occurring?  To be able to generally
>> monitor the machine's health?
> 
> For corrected memory errors, in general they are available in kernel
> logs. On X86 Machine Check handling will log unparsed MCs (one needs
> to read mci_status to know what exactly the error is). On ARM, GHES
> logs parsed CPER (already containing error type and error severity).
> The shortcoming is logs are rate limited. So in a burst of corrected
> memory errors the user may not be able to figure out exactly how many
> there were.
> 
> For uncorrectable memory errors, num_poisoned_pages is a reliable counter.
> 
>>
>>> @@ -2783,6 +2795,12 @@ int soft_offline_page(unsigned long pfn, int flags)
>>>               return -EIO;
>>>       }
>>>
>>> +     if (!sysctl_enable_soft_offline) {
>>> +             pr_info("%#lx: OS-wide disabled\n", pfn);
>>
>> This doesn't seem a very good message.  There's no indication that it
>> comes from the memory failure code at all.  If the sysadmin sees this
>> come out in the kernels logs, he/she will have to grep the kernel
>> sources just to figure out where the message came from.  Perhaps we can
>> be more helpful here..
> 
> For sure. I took it for granted that any pr_info will have the "Memory
> failure: " prefix, but now realize there is a `#undef pr_fmt` +
> `#define pr_fmt(fmt) "" fmt` just above unpoison_memory.
> 
> I propose to do `#define pr_fmt(fmt) "Soft offline: " fmt` above
> mf_isolate_folio, so that any soft-offline related code generates logs
> with the same following format:
> 
>   "Soft offline: 0x${pfn}: ${detailed_message}"
> 
> If everyone thinks this is reasonable, in v4 I can insert a new commit
> to make the log formats unified.

This sounds fine to me. And even better, `#define pr_fmt(fmt) "Unpoison: " fmt` can
also be done just above unpoison_memory.

Thanks.
.