On 5/12/26 15:33, Breno Leitao wrote:
> On Tue, May 12, 2026 at 10:21:50AM +0200, David Hildenbrand (Arm) wrote:
>>
>>> }
>>> goto unlock_mutex;
>>> } else if (res < 0) {
>>> - if (is_reserved)
>>> + /*
>>> + * Promote a stable unhandlable kernel page diagnosed by
>>> + * get_hwpoison_page() to MF_MSG_KERNEL alongside reserved
>>> + * pages; transient lifecycle races stay as MF_MSG_GET_HWPOISON.
>>> + */
>>> + if (is_reserved || gp_status == MF_GET_PAGE_UNHANDLABLE)
>>> res = action_result(pfn, MF_MSG_KERNEL, MF_IGNORED);
>>
>>
>> It's all a bit of a mess. get_hwpoison_page() should just indicate that a
>> page
>> is unhandable if it is PG_reserved?
>
> Are you saying that we should identify if the page is PG_reserved in
> get_hwpoison_page() instead of in memory_failure(), as done in the
> previous patch ("mm/memory-failure: report MF_MSG_KERNEL for reserved
> pages") ?
>
>> Why can't we just return a special error code from get_hwpoison_page()? We
>> ahve
>> plenty of errno values to chose from.
>
> Something like:
>
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 866c4428ac7ef..0a6d83575833e 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -878,7 +878,7 @@ static const char *action_name[] = {
> };
>
> static const char * const action_page_types[] = {
> - [MF_MSG_KERNEL] = "reserved kernel page",
> + [MF_MSG_KERNEL] = "unrecoverable kernel page",
> [MF_MSG_KERNEL_HIGH_ORDER] = "high-order kernel page",
> [MF_MSG_HUGE] = "huge page",
> [MF_MSG_FREE_HUGE] = "free huge page",
> @@ -1394,6 +1394,21 @@ static int get_any_page(struct page *p, unsigned long
> flags)
> int ret = 0, pass = 0;
> bool count_increased = false;
>
> + if (PageReserved(p)) {
> + ret = -ENOTRECOVERABLE;
> + goto out;
> + }
> +
> if (flags & MF_COUNT_INCREASED)
> count_increased = true;
>
> @@ -1422,7 +1437,7 @@ static int get_any_page(struct page *p, unsigned long
> flags)
> shake_page(p);
> goto try_again;
> }
> - ret = -EIO;
> + ret = -ENOTRECOVERABLE;
> goto out;
> }
> }
> @@ -1441,10 +1456,10 @@ static int get_any_page(struct page *p, unsigned long
> flags)
> goto try_again;
> }
> put_page(p);
> - ret = -EIO;
> + ret = -ENOTRECOVERABLE;
> }
> out:
> - if (ret == -EIO)
> + if (ret == -EIO || ret == -ENOTRECOVERABLE)
> pr_err("%#lx: unhandlable page.\n", page_to_pfn(p));
>
> return ret;
> @@ -2431,6 +2448,9 @@ int memory_failure(unsigned long pfn, int flags)
> res = action_result(pfn, MF_MSG_KERNEL_HIGH_ORDER,
> MF_IGNORED);
> }
> goto unlock_mutex;
> + } else if (res == -ENOTRECOVERABLE) {
> + res = action_result(pfn, MF_MSG_KERNEL, MF_IGNORED);
> + goto unlock_mutex;
> } else if (res < 0) {
> res = action_result(pfn, MF_MSG_GET_HWPOISON, MF_IGNORED);
> goto unlock_mutex;
That might probably read nicer as
switch (res) {
case 0: ...
case 1: ...
case -ENOTRECOVERABLE: ...
case ...
default:
}
>
>
> If that is what you are suggestion, maybe we can create another
> MF_MSG_RESERVED? and another return value for get_any_page() to track
> the reserve pages ?
I guess "reserved" is really just like most other kernel pages. So I wouldn't
special-case them here.
Or would there be a good reason?
--
Cheers,
David
