linux-kernel - Re: [PATCH v5 1/4] mm/migration: make isolate_movable

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: <55ba8d7e-df15-5a27-cf53-84d3f5ed2a59@huawei.com>
Date:   Fri, 3 Feb 2017 09:42:36 +0800
From:   Yisheng Xie <xieyisheng1@...wei.com>
To:     Minchan Kim <minchan@...nel.org>, Michal Hocko <mhocko@...nel.org>
CC:     <ysxie@...mail.com>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>, <n-horiguchi@...jp.nec.com>,
        <akpm@...ux-foundation.org>, <vbabka@...e.cz>,
        <mgorman@...hsingularity.net>, <hannes@...xchg.org>,
        <iamjoonsoo.kim@....com>, <izumi.taku@...fujitsu.com>,
        <arbab@...ux.vnet.ibm.com>, <vkuznets@...hat.com>,
        <ak@...ux.intel.com>, <guohanjun@...wei.com>, <qiuxishi@...wei.com>
Subject: Re: [PATCH v5 1/4] mm/migration: make isolate_movable_page() return
 int type

Hi Minchan
Thanks for reviewing.
On 2017/2/2 15:28, Minchan Kim wrote:
> On Wed, Feb 01, 2017 at 11:00:23AM +0100, Michal Hocko wrote:
>> On Wed 01-02-17 18:46:36, Minchan Kim wrote:
>>> On Wed, Feb 01, 2017 at 08:59:24AM +0100, Michal Hocko wrote:
>>>> On Wed 01-02-17 15:48:21, Minchan Kim wrote:
>>>>> Hi Yisheng,
>>>>>
>>>>> On Tue, Jan 31, 2017 at 09:06:18PM +0800, ysxie@...mail.com wrote:
>>>>>> From: Yisheng Xie <xieyisheng1@...wei.com>
>>>>>>
>>>>>> This patch changes the return type of isolate_movable_page()
>>>>>> from bool to int. It will return 0 when isolate movable page
>>>>>> successfully, return -EINVAL when the page is not a non-lru movable
>>>>>> page, and for other cases it will return -EBUSY.
>>>>>>
>>>>>> There is no functional change within this patch but prepare
>>>>>> for later patch.
>>>>>>
>>>>>> Signed-off-by: Yisheng Xie <xieyisheng1@...wei.com>
>>>>>> Suggested-by: Michal Hocko <mhocko@...nel.org>
>>>>>
>>>>> Sorry for missing this one you guys were discussing.
>>>>> I don't understand the patch's goal although I read later patches.
>>>>
>>>> The point is that the failed isolation has to propagate error up the
>>>> call chain to the userspace which has initiated the migration.
>>>>
>>>>> isolate_movable_pages returns success/fail so that's why I selected
>>>>> bool rather than int but it seems you guys want to propagate more
>>>>> detailed error to the user so added -EBUSY and -EINVAL.
>>>>>
>>>>> But the question is why isolate_lru_pages doesn't have -EINVAL?
>>>>
>>>> It doesn't have to same as isolate_movable_pages. We should just return
>>>> EBUSY when the page is no longer movable.
>>>
>>> Why isolate_lru_page is okay to return -EBUSY in case of race while
>>> isolate_movable_page should return -EINVAL?
>>> What's the logic in your mind? I totally cannot understand.
>>
>> Let me rephrase. Both should return EBUSY.
> 
> It means it's binary return value(success: 0 fail : -EBUSY) so IMO,
> bool is better and caller should return -EBUSY if that functions
> returns *false*. No need to make deeper propagation level.
> Anyway, it's trivial so I'm not against it if you want to make
> isolate_movable_page returns int. Insetad, please remove -EINVAL
> in this patch and just return -EBUSY for isolate_movable_page to
> be consistent with isolate_lru_page.
> Then, we don't need to fix any driver side, either. Even, no need to
> update any document because you don't add any new error value.
> 
Ok, I will remove the -EINVAL.

Thanks.
Yisheng Xie.

> That's enough.
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@...ck.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>
> 
> .
>