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

[Miaohe Lin <linmiaohe@huawei.com>]

On 2024/6/15 0:30, Jiaqi Yan wrote:
> On Fri, Jun 14, 2024 at 1:35 AM Lance Yang <ioworker0@gmail.com> wrote:
>>
>> Hi Jiaqi,
>>
>> On Wed, Jun 12, 2024 at 5:56 AM 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 inuse; 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 mmap hugepages MAP_FAILED 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:
>>> 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 called enable_soft_offline under /proc/sys/vm. By default
>>> enable_soft_line is 1 to preserve existing behavior in kernel.
>>
>> s/enable_soft_line/enable_soft_offline
> 
> Will fix this typo in v3.
> 
>>
>>>
>>> Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>
>>> ---
>>>  mm/memory-failure.c | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>> index d3c830e817e3..23415fe03318 100644
>>> --- a/mm/memory-failure.c
>>> +++ b/mm/memory-failure.c
>>> @@ -68,6 +68,8 @@ static int sysctl_memory_failure_early_kill __read_mostly;
>>>
>>>  static int sysctl_memory_failure_recovery __read_mostly = 1;
>>>
>>> +static int sysctl_enable_soft_offline __read_mostly = 1;
>>> +
>>>  atomic_long_t num_poisoned_pages __read_mostly = ATOMIC_LONG_INIT(0);
>>>
>>>  static bool hw_memory_failure __read_mostly = false;
>>> @@ -141,6 +143,15 @@ static struct ctl_table memory_failure_table[] = {
>>>                 .extra1         = SYSCTL_ZERO,
>>>                 .extra2         = SYSCTL_ONE,
>>>         },
>>> +       {
>>> +               .procname       = "enable_soft_offline",
>>> +               .data           = &sysctl_enable_soft_offline,
>>> +               .maxlen         = sizeof(sysctl_enable_soft_offline),
>>> +               .mode           = 0644,
>>> +               .proc_handler   = proc_dointvec_minmax,
>>> +               .extra1         = SYSCTL_ZERO,
>>> +               .extra2         = SYSCTL_ONE,
>>> +       }
>>>  };
>>>
>>>  /*
>>> @@ -2771,6 +2782,11 @@ int soft_offline_page(unsigned long pfn, int flags)
>>>         bool try_again = true;
>>>         struct page *page;
>>>
>>> +       if (!sysctl_enable_soft_offline) {
>>> +               pr_info("soft offline: %#lx: OS-wide disabled\n", pfn);
>>> +               return -EINVAL;
>>
>> IMO, "-EPERM" might sound better ;)
>>
>> Using "-EPERM" indicates that the operation is not permitted due to
>> the OS-wide configuration.
> 
> Miaohe suggested -EOPNOTSUPP. I agree both EOPNOTSUPP and EPERM may be
> better than EINVAL. But I wonder how about EAGAIN? With EAGAIN plus
> showing "disabled by /proc/sys/vm/enable_soft_offline" in dmesg, users
> now should be clear that they can try again with
> /proc/sys/vm/enable_soft_offline=1.

IMHO, it might not be suitable to use EAGAIN. Because it means "Resource temporarily unavailable" and
this can be solved by simply retry later without any further actions taken. But I might be wrong.

Thanks.
.