linux-kernel - Re: [PATCH v2 1/2] mm: migrate: Fix return value if all subpages of THPs are migrated successfully

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87h6ztr6f9.fsf@nvidia.com>
Date:   Mon, 24 Oct 2022 19:06:57 +1100
From:   Alistair Popple <apopple@...dia.com>
To:     Baolin Wang <baolin.wang@...ux.alibaba.com>
Cc:     akpm@...ux-foundation.org, david@...hat.com, ying.huang@...el.com,
        ziy@...dia.com, shy828301@...il.com, jingshan@...ux.alibaba.com,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] mm: migrate: Fix return value if all subpages of
 THPs are migrated successfully


Baolin Wang <baolin.wang@...ux.alibaba.com> writes:

> On 10/24/2022 3:24 PM, Alistair Popple wrote:
>> Baolin Wang <baolin.wang@...ux.alibaba.com> writes:
>>
>>> On 10/24/2022 10:36 AM, Alistair Popple wrote:
>>>> Baolin Wang <baolin.wang@...ux.alibaba.com> writes:
>>>>
>>>>> When THP migration, if THPs are split and all subpages are migrated successfully
>>>>> , the migrate_pages() will still return the number of THP that were not migrated.
>>>>> That will confuse the callers of migrate_pages(), for example, which will make
>>>>> the longterm pinning failed though all pages are migrated successfully.
>>>>>
>>>>> Thus we should return 0 to indicate all pages are migrated in this case.
>>>>>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@...ux.alibaba.com>
>>>>> ---
>>>>> Changes from v1:
>>>>> - Fix the return value of migrate_pages() instead of fixing the
>>>>>     callers' validation.
>>>>> ---
>>>>>    mm/migrate.c | 7 +++++++
>>>>>    1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>> index 8e5eb6e..1da0dbc 100644
>>>>> --- a/mm/migrate.c
>>>>> +++ b/mm/migrate.c
>>>>> @@ -1582,6 +1582,13 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>>    	 */
>>>>>    	list_splice(&ret_pages, from);
>>>>>
>>>>> +	/*
>>>>> +	 * Return 0 in case all subpages of fail-to-migrate THPs are
>>>>> +	 * migrated successfully.
>>>>> +	 */
>>>>> +	if (nr_thp_split && list_empty(from))
>>>>> +		rc = 0;
>>>> Why do you need to check nr_thp_split? Wouldn't list_empty(from) == True
>>>
>>> Only in the case of THP split, we can meet this abnormal case. So if no THP
>>> split, just return the original 'rc' instead of validating the list, since the
>>> 'nr_thp_split' validation is cheaper than the list_empty() validation IMHO.
>> Is it really that much cheaper? We're already retrying migrations
>> multiple times, etc. so surely the difference here would be marginal at
>> best, and IMHO the code would be much clearer if we always set rc = 0
>> when list_empty(from) = True.
>
> Yeah, the difference is marginal and I have no strong preference. OK, will drop
> the 'nr_thp_split' in next version. Thanks.

Thanks. With that change feel free to add:

Reviewed-by: Alistair Popple <apopple@...dia.com>

>>>> imply success? And if it doesn't imply success wouldn't it be possible
>>>> to end up with nr_thp_split && list_empty(from) whilst still having
>>>> pages that failed to migrate?
>>>> The list management and return code logic from unmap_and_move() has
>>>> gotten pretty difficult to follow and could do with some rework IMHO.
>>>
>>> Yes, Huang Ying has sent a RFC patchset[1] doing some code refactor, which seems
>>> a good start.
>> Thanks for pointing that out, I looked at it a while back but missed the
>> clean ups. I was kind of waiting for the non-RFC version before taking
>> another closer look.
>>
>>> [1] https://lore.kernel.org/all/20220921060616.73086-1-ying.huang@intel.com/