After commit e2846124f9a2 ("zsmalloc: zs_page_migrate: skip unnecessary
loops but not return -EBUSY if zspage is not inuse") zs_page_migrate
can handle the ZS_EMPTY zspage.
But I got some false in zs_page_isolate:
if (get_zspage_inuse(zspage) == 0) {
spin_unlock(&class->lock);
return false;
}
The page of this zspage was migrated in before.
The reason is commit e2846124f9a2 ("zsmalloc: zs_page_migrate: skip
unnecessary loops but not return -EBUSY if zspage is not inuse") just
handle the "page" but not "newpage" then it keep the "newpage" with
a empty zspage inside system.
Root cause is zs_page_isolate remove it from ZS_EMPTY list but not
call zs_page_putback "schedule_work(&pool->free_work);". Because
zs_page_migrate done the job without "schedule_work(&pool->free_work);"
Make this patch let zs_page_migrate wake up free_work if need.
Fixes: e2846124f9a2 ("zsmalloc: zs_page_migrate: skip unnecessary loops but not return -EBUSY if zspage is not inuse")
Signed-off-by: Hui Zhu <[email protected]>
---
mm/zsmalloc.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 62457eb..c6cc77c 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -2035,8 +2035,17 @@ int zs_page_migrate(struct address_space *mapping, struct page *newpage,
* Page migration is done so let's putback isolated zspage to
* the list if @page is final isolated subpage in the zspage.
*/
- if (!is_zspage_isolated(zspage))
- putback_zspage(class, zspage);
+ if (!is_zspage_isolated(zspage)) {
+ /*
+ * Page will be freed in following part. But newpage and
+ * zspage will stay in system if zspage is in ZS_EMPTY
+ * list. So call free_work to free it.
+ * The page and class is locked, we cannot free zspage
+ * immediately so let's defer.
+ */
+ if (putback_zspage(class, zspage) == ZS_EMPTY)
+ schedule_work(&pool->free_work);
+ }
reset_page(page);
put_page(page);
--
1.9.1
Hi Hui,
On Mon, Aug 14, 2017 at 05:56:30PM +0800, Hui Zhu wrote:
> After commit e2846124f9a2 ("zsmalloc: zs_page_migrate: skip unnecessary
This patch is not merged yet so the hash is invalid.
That means we may fold this patch to [1] in current mmotm.
[1] zsmalloc-zs_page_migrate-skip-unnecessary-loops-but-not-return-ebusy-if-zspage-is-not-inuse-fix.patch
> loops but not return -EBUSY if zspage is not inuse") zs_page_migrate
> can handle the ZS_EMPTY zspage.
>
> But I got some false in zs_page_isolate:
> if (get_zspage_inuse(zspage) == 0) {
> spin_unlock(&class->lock);
> return false;
> }
I also realized we should make zs_page_isolate succeed on empty zspage
because we allow the empty zspage migration from now on.
Could you send a patch for that as well?
> The page of this zspage was migrated in before.
>
> The reason is commit e2846124f9a2 ("zsmalloc: zs_page_migrate: skip
> unnecessary loops but not return -EBUSY if zspage is not inuse") just
> handle the "page" but not "newpage" then it keep the "newpage" with
> a empty zspage inside system.
> Root cause is zs_page_isolate remove it from ZS_EMPTY list but not
> call zs_page_putback "schedule_work(&pool->free_work);". Because
> zs_page_migrate done the job without "schedule_work(&pool->free_work);"
>
> Make this patch let zs_page_migrate wake up free_work if need.
>
> Fixes: e2846124f9a2 ("zsmalloc: zs_page_migrate: skip unnecessary loops but not return -EBUSY if zspage is not inuse")
> Signed-off-by: Hui Zhu <[email protected]>
> ---
> mm/zsmalloc.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 62457eb..c6cc77c 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -2035,8 +2035,17 @@ int zs_page_migrate(struct address_space *mapping, struct page *newpage,
> * Page migration is done so let's putback isolated zspage to
> * the list if @page is final isolated subpage in the zspage.
> */
> - if (!is_zspage_isolated(zspage))
> - putback_zspage(class, zspage);
> + if (!is_zspage_isolated(zspage)) {
> + /*
> + * Page will be freed in following part. But newpage and
> + * zspage will stay in system if zspage is in ZS_EMPTY
> + * list. So call free_work to free it.
> + * The page and class is locked, we cannot free zspage
> + * immediately so let's defer.
> + */
How about this?
/*
* Since we allow empty zspage migration, putback of zspage
* should free empty zspage. Otherwise, it could make a leak
* until upcoming free_work is done, which isn't guaranteed.
*/
> + if (putback_zspage(class, zspage) == ZS_EMPTY)
> + schedule_work(&pool->free_work);
> + }
>
> reset_page(page);
> put_page(page);
> --
> 1.9.1
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Hi Minchan,
2017-08-16 10:13 GMT+08:00 Minchan Kim <[email protected]>:
> Hi Hui,
>
> On Mon, Aug 14, 2017 at 05:56:30PM +0800, Hui Zhu wrote:
>> After commit e2846124f9a2 ("zsmalloc: zs_page_migrate: skip unnecessary
>
> This patch is not merged yet so the hash is invalid.
> That means we may fold this patch to [1] in current mmotm.
>
> [1] zsmalloc-zs_page_migrate-skip-unnecessary-loops-but-not-return-ebusy-if-zspage-is-not-inuse-fix.patch
>
>> loops but not return -EBUSY if zspage is not inuse") zs_page_migrate
>> can handle the ZS_EMPTY zspage.
>>
>> But I got some false in zs_page_isolate:
>> if (get_zspage_inuse(zspage) == 0) {
>> spin_unlock(&class->lock);
>> return false;
>> }
>
> I also realized we should make zs_page_isolate succeed on empty zspage
> because we allow the empty zspage migration from now on.
> Could you send a patch for that as well?
OK. I will make a patch for that later.
Thanks,
Hui
>
>> The page of this zspage was migrated in before.
>>
>> The reason is commit e2846124f9a2 ("zsmalloc: zs_page_migrate: skip
>> unnecessary loops but not return -EBUSY if zspage is not inuse") just
>> handle the "page" but not "newpage" then it keep the "newpage" with
>> a empty zspage inside system.
>> Root cause is zs_page_isolate remove it from ZS_EMPTY list but not
>> call zs_page_putback "schedule_work(&pool->free_work);". Because
>> zs_page_migrate done the job without "schedule_work(&pool->free_work);"
>>
>> Make this patch let zs_page_migrate wake up free_work if need.
>>
>> Fixes: e2846124f9a2 ("zsmalloc: zs_page_migrate: skip unnecessary loops but not return -EBUSY if zspage is not inuse")
>> Signed-off-by: Hui Zhu <[email protected]>
>> ---
>> mm/zsmalloc.c | 13 +++++++++++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
>> index 62457eb..c6cc77c 100644
>> --- a/mm/zsmalloc.c
>> +++ b/mm/zsmalloc.c
>> @@ -2035,8 +2035,17 @@ int zs_page_migrate(struct address_space *mapping, struct page *newpage,
>> * Page migration is done so let's putback isolated zspage to
>> * the list if @page is final isolated subpage in the zspage.
>> */
>> - if (!is_zspage_isolated(zspage))
>> - putback_zspage(class, zspage);
>> + if (!is_zspage_isolated(zspage)) {
>> + /*
>> + * Page will be freed in following part. But newpage and
>> + * zspage will stay in system if zspage is in ZS_EMPTY
>> + * list. So call free_work to free it.
>> + * The page and class is locked, we cannot free zspage
>> + * immediately so let's defer.
>> + */
>
> How about this?
>
> /*
> * Since we allow empty zspage migration, putback of zspage
> * should free empty zspage. Otherwise, it could make a leak
> * until upcoming free_work is done, which isn't guaranteed.
> */
>> + if (putback_zspage(class, zspage) == ZS_EMPTY)
>> + schedule_work(&pool->free_work);
>> + }
>>
>> reset_page(page);
>> put_page(page);
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to [email protected]. For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
On Wed, Aug 16, 2017 at 10:49:14AM +0800, Hui Zhu wrote:
> Hi Minchan,
>
> 2017-08-16 10:13 GMT+08:00 Minchan Kim <[email protected]>:
> > Hi Hui,
> >
> > On Mon, Aug 14, 2017 at 05:56:30PM +0800, Hui Zhu wrote:
> >> After commit e2846124f9a2 ("zsmalloc: zs_page_migrate: skip unnecessary
> >
> > This patch is not merged yet so the hash is invalid.
> > That means we may fold this patch to [1] in current mmotm.
> >
> > [1] zsmalloc-zs_page_migrate-skip-unnecessary-loops-but-not-return-ebusy-if-zspage-is-not-inuse-fix.patch
> >
> >> loops but not return -EBUSY if zspage is not inuse") zs_page_migrate
> >> can handle the ZS_EMPTY zspage.
> >>
> >> But I got some false in zs_page_isolate:
> >> if (get_zspage_inuse(zspage) == 0) {
> >> spin_unlock(&class->lock);
> >> return false;
> >> }
> >
> > I also realized we should make zs_page_isolate succeed on empty zspage
> > because we allow the empty zspage migration from now on.
> > Could you send a patch for that as well?
>
> OK. I will make a patch for that later.
Please send the patch so I want to fold it to [1] before Andrew is going
to send [1] to Linus.
Thanks.
2017-08-16 12:51 GMT+08:00 Minchan Kim <[email protected]>:
> On Wed, Aug 16, 2017 at 10:49:14AM +0800, Hui Zhu wrote:
>> Hi Minchan,
>>
>> 2017-08-16 10:13 GMT+08:00 Minchan Kim <[email protected]>:
>> > Hi Hui,
>> >
>> > On Mon, Aug 14, 2017 at 05:56:30PM +0800, Hui Zhu wrote:
>> >> After commit e2846124f9a2 ("zsmalloc: zs_page_migrate: skip unnecessary
>> >
>> > This patch is not merged yet so the hash is invalid.
>> > That means we may fold this patch to [1] in current mmotm.
>> >
>> > [1] zsmalloc-zs_page_migrate-skip-unnecessary-loops-but-not-return-ebusy-if-zspage-is-not-inuse-fix.patch
>> >
>> >> loops but not return -EBUSY if zspage is not inuse") zs_page_migrate
>> >> can handle the ZS_EMPTY zspage.
>> >>
>> >> But I got some false in zs_page_isolate:
>> >> if (get_zspage_inuse(zspage) == 0) {
>> >> spin_unlock(&class->lock);
>> >> return false;
>> >> }
>> >
>> > I also realized we should make zs_page_isolate succeed on empty zspage
>> > because we allow the empty zspage migration from now on.
>> > Could you send a patch for that as well?
>>
>> OK. I will make a patch for that later.
>
> Please send the patch so I want to fold it to [1] before Andrew is going
> to send [1] to Linus.
>
> Thanks.
Done.
Thanks,
Hui