Re: [PATCH v5 1/4] mm/migration: make isolate_movable_page() return int type

[Yisheng Xie]

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@xxxxxxxxxx  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@xxxxxxxxxx  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@xxxxxxxxxx  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
> 
> .
>