hi Minchan,
Thanks for your reviewing.
On 2017/2/1 17:46, 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@xxxxxxxxxxx wrote:
>>>> From: Yisheng Xie <xieyisheng1@xxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxx>
>>>> Suggested-by: Michal Hocko <mhocko@xxxxxxxxxx>
>>>
>>> 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.
>
Sorry, that's my mistake for error understanding code.
You're right. It should be EBUSY if it is in race, I will change it.
Thanks
Yisheng Xie.
>>
>>> Secondly, madvise man page should update?
>>
>> Why?
>
> man page of madvise doesn't say anything about the error propagation
> for soft_offline.
>
>>
>>> Thirdly, if a driver fail isolation due to -ENOMEM, it should be
>>> propagated, too?
>>
>> Yes
>>
>>> if we want to propagte detailed error to user, driver's isolate_page
>>> function should return right error.
>>
>> Yes
>
> It seems we are okay to return just -EBUSY until now but now you try to
> return more various error. I don't understand what problem you are
> seeing with just -EBUSY. Anyway, if you want to do it, it should be able
> to propagate error from driver side. That means it should make rule
> what kinds of error driver can return. Please write down it to
> Documentation/vm/page_migration and fix zsmalloc/virtio-balloon, too.
>
>>
>>> I don't feel this all changes should be done now. What's the problem
>>> if we change isolate_lru_page from int to bool? it returns just binary
>>> value so it should be right place to use bool. If it fails, error val
>>> is just -EBUSY.
>>
>> We really want to propagate the reason why the offline operation has
>> failed. Why would we want to postpone that?
>
> I'm not against but if you want to do it, just do it rightly.
>
>>
>>>> Cc: Minchan Kim <minchan@xxxxxxxxxx>
>>>> Cc: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
>>>> CC: Vlastimil Babka <vbabka@xxxxxxx>
>>>> ---
>>>> include/linux/migrate.h | 2 +-
>>>> mm/compaction.c | 2 +-
>>>> mm/migrate.c | 11 +++++++----
>>>> 3 files changed, 9 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
>>>> index ae8d475..43d5deb 100644
>>>> --- a/include/linux/migrate.h
>>>> +++ b/include/linux/migrate.h
>>>> @@ -37,7 +37,7 @@ extern int migrate_page(struct address_space *,
>>>> struct page *, struct page *, enum migrate_mode);
>>>> extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
>>>> unsigned long private, enum migrate_mode mode, int reason);
>>>> -extern bool isolate_movable_page(struct page *page, isolate_mode_t mode);
>>>> +extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
>>>> extern void putback_movable_page(struct page *page);
>>>>
>>>> extern int migrate_prep(void);
>>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>>> index 949198d..1d89147 100644
>>>> --- a/mm/compaction.c
>>>> +++ b/mm/compaction.c
>>>> @@ -802,7 +802,7 @@ static bool too_many_isolated(struct zone *zone)
>>>> locked = false;
>>>> }
>>>>
>>>> - if (isolate_movable_page(page, isolate_mode))
>>>> + if (!isolate_movable_page(page, isolate_mode))
>>>> goto isolate_success;
>>>> }
>>>>
>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>> index 87f4d0f..bbbd170 100644
>>>> --- a/mm/migrate.c
>>>> +++ b/mm/migrate.c
>>>> @@ -74,8 +74,9 @@ int migrate_prep_local(void)
>>>> return 0;
>>>> }
>>>>
>>>> -bool isolate_movable_page(struct page *page, isolate_mode_t mode)
>>>> +int isolate_movable_page(struct page *page, isolate_mode_t mode)
>>>> {
>>>> + int ret = -EBUSY;
>>>> struct address_space *mapping;
>>>>
>>>> /*
>>>> @@ -95,8 +96,10 @@ bool isolate_movable_page(struct page *page, isolate_mode_t mode)
>>>> * assumes anybody doesn't touch PG_lock of newly allocated page
>>>> * so unconditionally grapping the lock ruins page's owner side.
>>>> */
>>>> - if (unlikely(!__PageMovable(page)))
>>>> + if (unlikely(!__PageMovable(page))) {
>>>> + ret = -EINVAL;
>>>> goto out_putpage;
>>>> + }
>>>> /*
>>>> * As movable pages are not isolated from LRU lists, concurrent
>>>> * compaction threads can race against page migration functions
>>>> @@ -125,14 +128,14 @@ bool isolate_movable_page(struct page *page, isolate_mode_t mode)
>>>> __SetPageIsolated(page);
>>>> unlock_page(page);
>>>>
>>>> - return true;
>>>> + return 0;
>>>>
>>>> out_no_isolated:
>>>> unlock_page(page);
>>>> out_putpage:
>>>> put_page(page);
>>>> out:
>>>> - return false;
>>>> + return ret;
>>>> }
>>>>
>>>> /* It should be called on page which is PG_movable */
>>>> --
>>>> 1.9.1
>>>>
>>>> --
>>>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>>>> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
>>>> see: http://www.linux-mm.org/ .
>>>> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
>>
>> --
>> Michal Hocko
>> SUSE Labs
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
>
> .
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
