[BUG REPORT] ZSWAP: theoretical race condition issues

[Weijie Yang]

I found a few bugs in zswap when I review Linux-3.11-rc5, and I have
also some questions about it, described as following:

BUG:
1. A race condition when reclaim a page
when a handle alloced from zbud, zbud considers this handle is used
validly by upper(zswap) and can be a candidate for reclaim.
But zswap has to initialize it such as setting swapentry and addding
it to rbtree. so there is a race condition, such as:
thread 0: obtain handle x from zbud_alloc
thread 1: zbud_reclaim_page is called
thread 1: callback zswap_writeback_entry to reclaim handle x
thread 1: get swpentry from handle x (it is random value now)
thread 1: bad thing may happen
thread 0: initialize handle x with swapentry
Of course, this situation almost never happen, it is a "theoretical
race condition" issue.

2. Pollute swapcache data by add a pre-invalided swap page
when a swap_entry is invalidated, it will be reused by other anon
page. At the same time, zswap is reclaiming old page, pollute
swapcache of new page as a result, because old page and new page use
the same swap_entry, such as:
thread 1: zswap reclaim entry x
thread 0: zswap_frontswap_invalidate_page entry x
thread 0: entry x reused by other anon page
thread 1: add old data to swapcache of entry x
thread 0: swapcache of entry x is polluted
Of course, this situation almost never happen, it is another
"theoretical race condition" issue.

3. Frontswap uses frontswap_map bitmap to track page in "backend"
implementation, when zswap reclaim a
page, the corresponding bitmap record is not cleared.

4. zswap_tree is not freed when swapoff, and it got re-kzalloc in
swapon, memory leak occurs.

questions:
1. How about SetPageReclaim befor __swap_writepage, so that move it to
the tail of the inactive list?
2. zswap uses GFP_KERNEL flag to alloc things in store and reclaim
function, does this lead to these function called recursively?
3. for reclaiming one zbud page which contains two buddies, zswap
needs to alloc two pages. Does this reclaim cost-efficient?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Bob Liu]

Hi Weijie,

Post by Weijie Yang
I found a few bugs in zswap when I review Linux-3.11-rc5, and I have
1. A race condition when reclaim a page
when a handle alloced from zbud, zbud considers this handle is used
validly by upper(zswap) and can be a candidate for reclaim.
But zswap has to initialize it such as setting swapentry and addding
thread 0: obtain handle x from zbud_alloc
thread 1: zbud_reclaim_page is called
thread 1: callback zswap_writeback_entry to reclaim handle x
thread 1: get swpentry from handle x (it is random value now)
thread 1: bad thing may happen
thread 0: initialize handle x with swapentry

Yes, this may happen potentially but in rare case.
Because we have a LRU list for page frames, after Thread 0 called
zbud_alloc the corresponding page will be add to the head of LRU
list,While zbud_reclaim_page(Thread 1 called) is started from the tail
of LRU list.

Post by Weijie Yang
Of course, this situation almost never happen, it is a "theoretical
race condition" issue.
2. Pollute swapcache data by add a pre-invalided swap page
when a swap_entry is invalidated, it will be reused by other anon
page. At the same time, zswap is reclaiming old page, pollute
swapcache of new page as a result, because old page and new page use
thread 1: zswap reclaim entry x
thread 0: zswap_frontswap_invalidate_page entry x
thread 0: entry x reused by other anon page
thread 1: add old data to swapcache of entry x

I didn't get your idea here, why thread1 will add old data to entry x?

Post by Weijie Yang
thread 0: swapcache of entry x is polluted
Of course, this situation almost never happen, it is another
"theoretical race condition" issue.
3. Frontswap uses frontswap_map bitmap to track page in "backend"
implementation, when zswap reclaim a
page, the corresponding bitmap record is not cleared.

That's true, but I don't think it's a big problem.
Only waste little time to search rbtree during zswap_frontswap_load().

Post by Weijie Yang
4. zswap_tree is not freed when swapoff, and it got re-kzalloc in
swapon, memory leak occurs.

Nice catch! I think it should be freed in zswap_frontswap_invalidate_area().

Post by Weijie Yang
1. How about SetPageReclaim befor __swap_writepage, so that move it to
the tail of the inactive list?

It will be added to inactive now.

Post by Weijie Yang
2. zswap uses GFP_KERNEL flag to alloc things in store and reclaim
function, does this lead to these function called recursively?

Yes, that's a potential problem.

Post by Weijie Yang
3. for reclaiming one zbud page which contains two buddies, zswap
needs to alloc two pages. Does this reclaim cost-efficient?

Yes, that's a problem too. And that's why we use zbud as the default
allocator instead of zsmalloc.
I think improving the write back path of zswap is the next important
step for zswap.

[Minchan Kim]

Post by Bob Liu
Hi Weijie,

Post by Weijie Yang
I found a few bugs in zswap when I review Linux-3.11-rc5, and I have
1. A race condition when reclaim a page
when a handle alloced from zbud, zbud considers this handle is used
validly by upper(zswap) and can be a candidate for reclaim.
But zswap has to initialize it such as setting swapentry and addding
thread 0: obtain handle x from zbud_alloc
thread 1: zbud_reclaim_page is called
thread 1: callback zswap_writeback_entry to reclaim handle x
thread 1: get swpentry from handle x (it is random value now)
thread 1: bad thing may happen
thread 0: initialize handle x with swapentry

Nice catch!

Post by Bob Liu
Yes, this may happen potentially but in rare case.
Because we have a LRU list for page frames, after Thread 0 called
zbud_alloc the corresponding page will be add to the head of LRU
list,While zbud_reclaim_page(Thread 1 called) is started from the tail
of LRU list.

Post by Weijie Yang
Of course, this situation almost never happen, it is a "theoretical
race condition" issue.

But it's doable and we should prevent that although you feel it's rare
because system could go hang. When I look at the code, Why should zbud
have LRU logic instead of zswap? If I missed some history, sorry about that.
But at least to me, zbud is just allocator so it should have a role
to handle alloc/free object and how client of the allocator uses objects
depends on the upper layer so zbud should handle LRU. If so, we wouldn't
encounter this problem, either.

Post by Bob Liu

Post by Weijie Yang
2. Pollute swapcache data by add a pre-invalided swap page
when a swap_entry is invalidated, it will be reused by other anon
page. At the same time, zswap is reclaiming old page, pollute
swapcache of new page as a result, because old page and new page use
thread 1: zswap reclaim entry x
thread 0: zswap_frontswap_invalidate_page entry x
thread 0: entry x reused by other anon page
thread 1: add old data to swapcache of entry x

I didn't get your idea here, why thread1 will add old data to entry x?

Post by Weijie Yang
thread 0: swapcache of entry x is polluted
Of course, this situation almost never happen, it is another
"theoretical race condition" issue.

Don't swapcache_prepare close the race?

Post by Bob Liu

Post by Weijie Yang
3. Frontswap uses frontswap_map bitmap to track page in "backend"
implementation, when zswap reclaim a
page, the corresponding bitmap record is not cleared.

That's true, but I don't think it's a big problem.
Only waste little time to search rbtree during zswap_frontswap_load().

Post by Weijie Yang
4. zswap_tree is not freed when swapoff, and it got re-kzalloc in
swapon, memory leak occurs.

Nice catch! I think it should be freed in zswap_frontswap_invalidate_area().

Post by Weijie Yang
1. How about SetPageReclaim befor __swap_writepage, so that move it to
the tail of the inactive list?

It's a good idea to avoid unnecessary page scanning.

Post by Bob Liu
It will be added to inactive now.

Post by Weijie Yang
2. zswap uses GFP_KERNEL flag to alloc things in store and reclaim
function, does this lead to these function called recursively?

Yes, that's a potential problem.

It should use GFP_NOIO.

Post by Bob Liu

Post by Weijie Yang
3. for reclaiming one zbud page which contains two buddies, zswap
needs to alloc two pages. Does this reclaim cost-efficient?

It would be better to evict zpage which is a compressed sequence of
PAGE_SIZE bytes rather than decompresesed PAGE_SIZE bytes a page when
we are about to reclaim the page but it's hard part from frontswap API.

Post by Bob Liu
Yes, that's a problem too. And that's why we use zbud as the default
allocator instead of zsmalloc.
I think improving the write back path of zswap is the next important
step for zswap.
--
Regards,
-Bob
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
see: http://www.linux-mm.org/ .

[Weijie Yang]

Post by Bob Liu

Post by Bob Liu
Hi Weijie,

Post by Weijie Yang
I found a few bugs in zswap when I review Linux-3.11-rc5, and I have
1. A race condition when reclaim a page
when a handle alloced from zbud, zbud considers this handle is used
validly by upper(zswap) and can be a candidate for reclaim.
But zswap has to initialize it such as setting swapentry and addding
thread 0: obtain handle x from zbud_alloc
thread 1: zbud_reclaim_page is called
thread 1: callback zswap_writeback_entry to reclaim handle x
thread 1: get swpentry from handle x (it is random value now)
thread 1: bad thing may happen
thread 0: initialize handle x with swapentry

Nice catch!

Post by Bob Liu
Yes, this may happen potentially but in rare case.
Because we have a LRU list for page frames, after Thread 0 called
zbud_alloc the corresponding page will be add to the head of LRU
list,While zbud_reclaim_page(Thread 1 called) is started from the tail
of LRU list.

Post by Weijie Yang
Of course, this situation almost never happen, it is a "theoretical
race condition" issue.

But it's doable and we should prevent that although you feel it's rare
because system could go hang. When I look at the code, Why should zbud
have LRU logic instead of zswap? If I missed some history, sorry about that.
But at least to me, zbud is just allocator so it should have a role
to handle alloc/free object and how client of the allocator uses objects
depends on the upper layer so zbud should handle LRU. If so, we wouldn't
encounter this problem, either.

Post by Bob Liu

Post by Weijie Yang
2. Pollute swapcache data by add a pre-invalided swap page
when a swap_entry is invalidated, it will be reused by other anon
page. At the same time, zswap is reclaiming old page, pollute
swapcache of new page as a result, because old page and new page use
thread 1: zswap reclaim entry x
thread 0: zswap_frontswap_invalidate_page entry x
thread 0: entry x reused by other anon page
thread 1: add old data to swapcache of entry x

I didn't get your idea here, why thread1 will add old data to entry x?

Post by Weijie Yang
thread 0: swapcache of entry x is polluted
Of course, this situation almost never happen, it is another
"theoretical race condition" issue.

Don't swapcache_prepare close the race?

Yes, I made a mistake, there is not a race here.
However, I find another bug here after my more careful review. It is
not only "theoretical", it will happen really. as:
thread 1: zswap reclaim entry x (get the refcount, but not call
zswap_get_swap_cache_page yet)
thread 0: zswap_frontswap_invalidate_page entry x (finished, entry x
and its zbud is not freed as its refcount != 0)
now, the swap_map[x] = 0
thread 1: zswap_get_swap_cache_page called, swapcache_prepare return
-ENOENT because entry x is not used any more
zswap_get_swap_cache_page return ZSWAP_SWAPCACHE_NOMEM
zswap_writeback_entry do nothing except put refcount
now, the memory of zswap_entry x leaks and its zpage become a zombie

Best Regards,
Weijie Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Minchan Kim]

Hello,

Post by Weijie Yang

Post by Bob Liu

Post by Bob Liu
Hi Weijie,

Post by Weijie Yang
I found a few bugs in zswap when I review Linux-3.11-rc5, and I have
1. A race condition when reclaim a page
when a handle alloced from zbud, zbud considers this handle is used
validly by upper(zswap) and can be a candidate for reclaim.
But zswap has to initialize it such as setting swapentry and addding
thread 0: obtain handle x from zbud_alloc
thread 1: zbud_reclaim_page is called
thread 1: callback zswap_writeback_entry to reclaim handle x
thread 1: get swpentry from handle x (it is random value now)
thread 1: bad thing may happen
thread 0: initialize handle x with swapentry

Nice catch!

Post by Bob Liu
Yes, this may happen potentially but in rare case.
Because we have a LRU list for page frames, after Thread 0 called
zbud_alloc the corresponding page will be add to the head of LRU
list,While zbud_reclaim_page(Thread 1 called) is started from the tail
of LRU list.

Post by Weijie Yang
Of course, this situation almost never happen, it is a "theoretical
race condition" issue.

But it's doable and we should prevent that although you feel it's rare
because system could go hang. When I look at the code, Why should zbud
have LRU logic instead of zswap? If I missed some history, sorry about that.
But at least to me, zbud is just allocator so it should have a role
to handle alloc/free object and how client of the allocator uses objects
depends on the upper layer so zbud should handle LRU. If so, we wouldn't
encounter this problem, either.

Post by Bob Liu

Post by Weijie Yang
2. Pollute swapcache data by add a pre-invalided swap page
when a swap_entry is invalidated, it will be reused by other anon
page. At the same time, zswap is reclaiming old page, pollute
swapcache of new page as a result, because old page and new page use
thread 1: zswap reclaim entry x
thread 0: zswap_frontswap_invalidate_page entry x
thread 0: entry x reused by other anon page
thread 1: add old data to swapcache of entry x

I didn't get your idea here, why thread1 will add old data to entry x?

Post by Weijie Yang
thread 0: swapcache of entry x is polluted
Of course, this situation almost never happen, it is another
"theoretical race condition" issue.

Don't swapcache_prepare close the race?

Yes, I made a mistake, there is not a race here.
However, I find another bug here after my more careful review. It is
thread 1: zswap reclaim entry x (get the refcount, but not call
zswap_get_swap_cache_page yet)
thread 0: zswap_frontswap_invalidate_page entry x (finished, entry x
and its zbud is not freed as its refcount != 0)
now, the swap_map[x] = 0
thread 1: zswap_get_swap_cache_page called, swapcache_prepare return
-ENOENT because entry x is not used any more
zswap_get_swap_cache_page return ZSWAP_SWAPCACHE_NOMEM
zswap_writeback_entry do nothing except put refcount
now, the memory of zswap_entry x leaks and its zpage become a zombie

It makes sense to me.
Maybe we should introduce ZSWAP_SWAPCACHE_NOENT and free the entry in
the case. Would you mind to send patches on the problems you found?

Post by Weijie Yang
Best Regards,
Weijie Yang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
see: http://www.linux-mm.org/ .

[Weijie Yang]

I think I find a new issue, for integrity of this mail thread, I reply
to this mail.

It is a concurrence issue either, when duplicate store and reclaim
concurrentlly.

zswap entry x with offset A is already stored in zswap backend.
Consider the following scenario:

thread 0: reclaim entry x (get refcount, but not call zswap_get_swap_cache_page)

thread 1: store new page with the same offset A, alloc a new zswap entry y.
store finished. shrink_page_list() call __remove_mapping(), and now
it is not in swap_cache

thread 0: zswap_get_swap_cache_page called. old page data is added to swap_cache

Now, swap cache has old data rather than new data for offset A.
error will happen If do_swap_page() get page from swap_cache.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Bob Liu]

Post by Weijie Yang
I think I find a new issue, for integrity of this mail thread, I reply
to this mail.
It is a concurrence issue either, when duplicate store and reclaim
concurrentlly.
zswap entry x with offset A is already stored in zswap backend.
thread 0: reclaim entry x (get refcount, but not call zswap_get_swap_cache_page)
thread 1: store new page with the same offset A, alloc a new zswap entry y.
store finished. shrink_page_list() call __remove_mapping(), and now
it is not in swap_cache

But I don't think swap layer will call zswap with the same offset A.

Post by Weijie Yang
thread 0: zswap_get_swap_cache_page called. old page data is added to swap_cache
Now, swap cache has old data rather than new data for offset A.
error will happen If do_swap_page() get page from swap_cache.

[Weijie Yang]

Post by Bob Liu

Post by Weijie Yang
I think I find a new issue, for integrity of this mail thread, I reply
to this mail.
It is a concurrence issue either, when duplicate store and reclaim
concurrentlly.
zswap entry x with offset A is already stored in zswap backend.
thread 0: reclaim entry x (get refcount, but not call zswap_get_swap_cache_page)
thread 1: store new page with the same offset A, alloc a new zswap entry y.
store finished. shrink_page_list() call __remove_mapping(), and now
it is not in swap_cache

But I don't think swap layer will call zswap with the same offset A.

1. store page of offset A in zswap
2. some time later, pagefault occur, load page data from zswap.
But notice that zswap entry x is still in zswap because it is not
frontswap_tmem_exclusive_gets_enabled.
this page is with PageSwapCache(page) and page_private(page) = entry.val
3. change this page data, and it become dirty
4. some time later again, swap this page on the same offset A.

so, a duplicate store happens.

what I can think is that use flags and CAS to protect store and reclaim on
the same offset happens concurrentlly.

Post by Bob Liu

Post by Weijie Yang
thread 0: zswap_get_swap_cache_page called. old page data is added to swap_cache
Now, swap cache has old data rather than new data for offset A.
error will happen If do_swap_page() get page from swap_cache.

--
Regards,
--Bob

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Bob Liu]

Post by Weijie Yang

Post by Bob Liu

Post by Weijie Yang
I think I find a new issue, for integrity of this mail thread, I reply
to this mail.
It is a concurrence issue either, when duplicate store and reclaim
concurrentlly.
zswap entry x with offset A is already stored in zswap backend.
thread 0: reclaim entry x (get refcount, but not call zswap_get_swap_cache_page)
thread 1: store new page with the same offset A, alloc a new zswap entry y.
store finished. shrink_page_list() call __remove_mapping(), and now
it is not in swap_cache

But I don't think swap layer will call zswap with the same offset A.

1. store page of offset A in zswap
2. some time later, pagefault occur, load page data from zswap.
But notice that zswap entry x is still in zswap because it is not

Sorry I didn't notice that zswap_frontswap_load() doesn't call rb_erase().

Post by Weijie Yang
frontswap_tmem_exclusive_gets_enabled.
this page is with PageSwapCache(page) and page_private(page) = entry.val
3. change this page data, and it become dirty
4. some time later again, swap this page on the same offset A.
so, a duplicate store happens.

Then I think we should erase the entry from rbtree in zswap_frontswap_load().
After the page is decompressed and loaded from zswap, still storing
the compressed data in zswap is meanless.

[Weijie Yang]

Post by Bob Liu

Post by Weijie Yang

Post by Bob Liu

Post by Weijie Yang
I think I find a new issue, for integrity of this mail thread, I reply
to this mail.
It is a concurrence issue either, when duplicate store and reclaim
concurrentlly.
zswap entry x with offset A is already stored in zswap backend.
thread 0: reclaim entry x (get refcount, but not call zswap_get_swap_cache_page)
thread 1: store new page with the same offset A, alloc a new zswap entry y.
store finished. shrink_page_list() call __remove_mapping(), and now
it is not in swap_cache

But I don't think swap layer will call zswap with the same offset A.

1. store page of offset A in zswap
2. some time later, pagefault occur, load page data from zswap.
But notice that zswap entry x is still in zswap because it is not

Sorry I didn't notice that zswap_frontswap_load() doesn't call rb_erase().

Post by Weijie Yang
frontswap_tmem_exclusive_gets_enabled.
this page is with PageSwapCache(page) and page_private(page) = entry.val
3. change this page data, and it become dirty
4. some time later again, swap this page on the same offset A.
so, a duplicate store happens.

Then I think we should erase the entry from rbtree in zswap_frontswap_load().
After the page is decompressed and loaded from zswap, still storing
the compressed data in zswap is meanless.

Of cause, erasing the entry after load() can resolve this.
However, this problem is not simple and interesting.

If we drop the entry after load(), we should SetPageDirty, it will
generate more swap
even if we don't change this page data.
I think that is why frontswap has two load mode: default(used now) and
exclusive_gets.

I don't have test data, but I think which mode is better is decided by
corresponding workload.
It's better to realize these two modes, I will try it.

Post by Bob Liu
--
Regards,
--Bob

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Minchan Kim]

Hello Weigie,

Post by Weijie Yang

Post by Bob Liu

Post by Weijie Yang
I think I find a new issue, for integrity of this mail thread, I reply
to this mail.
It is a concurrence issue either, when duplicate store and reclaim
concurrentlly.
zswap entry x with offset A is already stored in zswap backend.
thread 0: reclaim entry x (get refcount, but not call zswap_get_swap_cache_page)
thread 1: store new page with the same offset A, alloc a new zswap entry y.
store finished. shrink_page_list() call __remove_mapping(), and now
it is not in swap_cache

But I don't think swap layer will call zswap with the same offset A.

1. store page of offset A in zswap
2. some time later, pagefault occur, load page data from zswap.
But notice that zswap entry x is still in zswap because it is not
frontswap_tmem_exclusive_gets_enabled.

frontswap_tmem_exclusive_gets_enabled is just option to see tradeoff
between CPU burining by frequent swapout and memory footprint by duplicate
copy in swap cache and frontswap backend so it shouldn't affect the stability.

Post by Weijie Yang
this page is with PageSwapCache(page) and page_private(page) = entry.val
3. change this page data, and it become dirty

If non-shared swapin page become redirty, it should remove the page from
swapcache. If shared swapin page become redirty, it should do CoW so it's a
new page so that it doesn't live in swap cache. It means it should have new
offset which is different with old's one for swap out.

What's wrong with that?

Post by Weijie Yang
4. some time later again, swap this page on the same offset A.
so, a duplicate store happens.
what I can think is that use flags and CAS to protect store and reclaim on
the same offset happens concurrentlly.

Post by Bob Liu

Post by Weijie Yang
thread 0: zswap_get_swap_cache_page called. old page data is added to swap_cache
Now, swap cache has old data rather than new data for offset A.
error will happen If do_swap_page() get page from swap_cache.

--
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
see: http://www.linux-mm.org/ .

[Weijie Yang]

Post by Minchan Kim
Hello Weigie,

Post by Weijie Yang

Post by Bob Liu

Post by Weijie Yang
I think I find a new issue, for integrity of this mail thread, I reply
to this mail.
It is a concurrence issue either, when duplicate store and reclaim
concurrentlly.
zswap entry x with offset A is already stored in zswap backend.
thread 0: reclaim entry x (get refcount, but not call zswap_get_swap_cache_page)
thread 1: store new page with the same offset A, alloc a new zswap entry y.
store finished. shrink_page_list() call __remove_mapping(), and now
it is not in swap_cache

But I don't think swap layer will call zswap with the same offset A.

1. store page of offset A in zswap
2. some time later, pagefault occur, load page data from zswap.
But notice that zswap entry x is still in zswap because it is not
frontswap_tmem_exclusive_gets_enabled.

frontswap_tmem_exclusive_gets_enabled is just option to see tradeoff
between CPU burining by frequent swapout and memory footprint by duplicate
copy in swap cache and frontswap backend so it shouldn't affect the stability.

Thanks for explain this.
I don't mean to say this option affects the stability, but that zswap
only realize
one option. Maybe it's better to realize both options for different workloads.

Post by Minchan Kim

Post by Weijie Yang
this page is with PageSwapCache(page) and page_private(page) = entry.val
3. change this page data, and it become dirty

If non-shared swapin page become redirty, it should remove the page from
swapcache. If shared swapin page become redirty, it should do CoW so it's a
new page so that it doesn't live in swap cache. It means it should have new
offset which is different with old's one for swap out.
What's wrong with that?

It is really not a right scene for duplicate store. And I can not think out one.
If duplicate store is impossible, How about delete the handle code in zswap?
If it does exist, I think there is a potential issue as I described.

Post by Minchan Kim

Post by Weijie Yang
4. some time later again, swap this page on the same offset A.
so, a duplicate store happens.
what I can think is that use flags and CAS to protect store and reclaim on
the same offset happens concurrentlly.

Post by Bob Liu

Post by Weijie Yang
thread 0: zswap_get_swap_cache_page called. old page data is added to swap_cache
Now, swap cache has old data rather than new data for offset A.
error will happen If do_swap_page() get page from swap_cache.

--
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
see: http://www.linux-mm.org/ .

--
Kind regards,
Minchan Kim

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Minchan Kim]

Post by Weijie Yang

Post by Minchan Kim
Hello Weigie,

Post by Weijie Yang

Post by Bob Liu

Post by Weijie Yang
I think I find a new issue, for integrity of this mail thread, I reply
to this mail.
It is a concurrence issue either, when duplicate store and reclaim
concurrentlly.
zswap entry x with offset A is already stored in zswap backend.
thread 0: reclaim entry x (get refcount, but not call zswap_get_swap_cache_page)
thread 1: store new page with the same offset A, alloc a new zswap entry y.
store finished. shrink_page_list() call __remove_mapping(), and now
it is not in swap_cache

But I don't think swap layer will call zswap with the same offset A.

1. store page of offset A in zswap
2. some time later, pagefault occur, load page data from zswap.
But notice that zswap entry x is still in zswap because it is not
frontswap_tmem_exclusive_gets_enabled.

frontswap_tmem_exclusive_gets_enabled is just option to see tradeoff
between CPU burining by frequent swapout and memory footprint by duplicate
copy in swap cache and frontswap backend so it shouldn't affect the stability.

Thanks for explain this.
I don't mean to say this option affects the stability, but that zswap
only realize
one option. Maybe it's better to realize both options for different workloads.

"zswap only relize one option"
What does it mena? Sorry. I couldn't parse your intention. :)
You mean zswap should do something special to support frontswap_tmem_exclusive_gets?

Post by Weijie Yang

Post by Minchan Kim

Post by Weijie Yang
this page is with PageSwapCache(page) and page_private(page) = entry.val
3. change this page data, and it become dirty

If non-shared swapin page become redirty, it should remove the page from
swapcache. If shared swapin page become redirty, it should do CoW so it's a
new page so that it doesn't live in swap cache. It means it should have new
offset which is different with old's one for swap out.
What's wrong with that?

It is really not a right scene for duplicate store. And I can not think out one.
If duplicate store is impossible, How about delete the handle code in zswap?
If it does exist, I think there is a potential issue as I described.

You mean "zswap_duplicate_entry"?
AFAIR, I already had a question to Seth when zswap was born but AFAIRC,
he said that he didn't know exact reason but he saw that case during
experiement so copy the code peice from zcache.

Do you see the case, too?

Anyway, we need to dive into that to know what happens and then open
our eyes for clear solution before dumping meaningless patch.

I hope Seth or Bob already know it.

Post by Weijie Yang

Post by Minchan Kim

Post by Weijie Yang
4. some time later again, swap this page on the same offset A.
so, a duplicate store happens.
what I can think is that use flags and CAS to protect store and reclaim on
the same offset happens concurrentlly.

Post by Bob Liu

Post by Weijie Yang
thread 0: zswap_get_swap_cache_page called. old page data is added to swap_cache
Now, swap cache has old data rather than new data for offset A.
error will happen If do_swap_page() get page from swap_cache.

--
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
see: http://www.linux-mm.org/ .

--
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
see: http://www.linux-mm.org/ .

[Weijie Yang]

Post by Minchan Kim

Post by Weijie Yang

Post by Minchan Kim
Hello Weigie,

Post by Weijie Yang

Post by Bob Liu

Post by Weijie Yang
I think I find a new issue, for integrity of this mail thread, I reply
to this mail.
It is a concurrence issue either, when duplicate store and reclaim
concurrentlly.
zswap entry x with offset A is already stored in zswap backend.
thread 0: reclaim entry x (get refcount, but not call zswap_get_swap_cache_page)
thread 1: store new page with the same offset A, alloc a new zswap entry y.
store finished. shrink_page_list() call __remove_mapping(), and now
it is not in swap_cache

But I don't think swap layer will call zswap with the same offset A.

1. store page of offset A in zswap
2. some time later, pagefault occur, load page data from zswap.
But notice that zswap entry x is still in zswap because it is not
frontswap_tmem_exclusive_gets_enabled.

frontswap_tmem_exclusive_gets_enabled is just option to see tradeoff
between CPU burining by frequent swapout and memory footprint by duplicate
copy in swap cache and frontswap backend so it shouldn't affect the stability.

Thanks for explain this.
I don't mean to say this option affects the stability, but that zswap
only realize
one option. Maybe it's better to realize both options for different workloads.

"zswap only relize one option"
What does it mena? Sorry. I couldn't parse your intention. :)
You mean zswap should do something special to support frontswap_tmem_exclusive_gets?

Yes. But I am not sure whether it is worth.

Post by Minchan Kim

Post by Weijie Yang

Post by Minchan Kim

Post by Weijie Yang
this page is with PageSwapCache(page) and page_private(page) = entry.val
3. change this page data, and it become dirty

If non-shared swapin page become redirty, it should remove the page from
swapcache. If shared swapin page become redirty, it should do CoW so it's a
new page so that it doesn't live in swap cache. It means it should have new
offset which is different with old's one for swap out.
What's wrong with that?

It is really not a right scene for duplicate store. And I can not think out one.
If duplicate store is impossible, How about delete the handle code in zswap?
If it does exist, I think there is a potential issue as I described.

You mean "zswap_duplicate_entry"?
AFAIR, I already had a question to Seth when zswap was born but AFAIRC,
he said that he didn't know exact reason but he saw that case during
experiement so copy the code peice from zcache.
Do you see the case, too?

Yes, I mean duplicate store.
I check the /Documentation/vm/frontswap.txt, it mentions "duplicate stores",
but I am still confused.

I wrote a zcache varietas which swap out compressed page to swapfile.
I did see that case when I test it on andorid smartphone(arm v7),
and it happens rarely and occasionally.
In one test, only 1 duplicate store occur in about 3157162 times stores.

Post by Minchan Kim
Anyway, we need to dive into that to know what happens and then open
our eyes for clear solution before dumping meaningless patch.
I hope Seth or Bob already know it.

Post by Weijie Yang

Post by Minchan Kim

Post by Weijie Yang
4. some time later again, swap this page on the same offset A.
so, a duplicate store happens.
what I can think is that use flags and CAS to protect store and reclaim on
the same offset happens concurrentlly.

Post by Bob Liu

Post by Weijie Yang
thread 0: zswap_get_swap_cache_page called. old page data is added to swap_cache
Now, swap cache has old data rather than new data for offset A.
error will happen If do_swap_page() get page from swap_cache.

--
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
see: http://www.linux-mm.org/ .

--
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
see: http://www.linux-mm.org/ .

--
Kind regards,
Minchan Kim

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Bob Liu]

Post by Weijie Yang

Post by Minchan Kim

Post by Weijie Yang

Post by Minchan Kim
Hello Weigie,

Post by Weijie Yang

Post by Bob Liu

Post by Weijie Yang
I think I find a new issue, for integrity of this mail thread, I reply
to this mail.
It is a concurrence issue either, when duplicate store and reclaim
concurrentlly.
zswap entry x with offset A is already stored in zswap backend.
thread 0: reclaim entry x (get refcount, but not call zswap_get_swap_cache_page)
thread 1: store new page with the same offset A, alloc a new zswap entry y.
store finished. shrink_page_list() call __remove_mapping(), and now
it is not in swap_cache

But I don't think swap layer will call zswap with the same offset A.

1. store page of offset A in zswap
2. some time later, pagefault occur, load page data from zswap.
But notice that zswap entry x is still in zswap because it is not
frontswap_tmem_exclusive_gets_enabled.

frontswap_tmem_exclusive_gets_enabled is just option to see tradeoff
between CPU burining by frequent swapout and memory footprint by duplicate
copy in swap cache and frontswap backend so it shouldn't affect the stability.

Thanks for explain this.
I don't mean to say this option affects the stability, but that zswap
only realize
one option. Maybe it's better to realize both options for different workloads.

"zswap only relize one option"
What does it mena? Sorry. I couldn't parse your intention. :)
You mean zswap should do something special to support frontswap_tmem_exclusive_gets?

Yes. But I am not sure whether it is worth.

Post by Minchan Kim

Post by Weijie Yang

Post by Minchan Kim

Post by Weijie Yang
this page is with PageSwapCache(page) and page_private(page) = entry.val
3. change this page data, and it become dirty

If non-shared swapin page become redirty, it should remove the page from
swapcache. If shared swapin page become redirty, it should do CoW so it's a
new page so that it doesn't live in swap cache. It means it should have new
offset which is different with old's one for swap out.
What's wrong with that?

It is really not a right scene for duplicate store. And I can not think out one.
If duplicate store is impossible, How about delete the handle code in zswap?
If it does exist, I think there is a potential issue as I described.

You mean "zswap_duplicate_entry"?
AFAIR, I already had a question to Seth when zswap was born but AFAIRC,
he said that he didn't know exact reason but he saw that case during
experiement so copy the code peice from zcache.
Do you see the case, too?

Yes, I mean duplicate store.
I check the /Documentation/vm/frontswap.txt, it mentions "duplicate stores",
but I am still confused.
I wrote a zcache varietas which swap out compressed page to swapfile.
I did see that case when I test it on andorid smartphone(arm v7),

Why not test it based on zswap directly?
My suggestion is do more and more stress testing against current
zswap, if there is really any bug triggered or unusual performance
issue then we dig it out and fix it.

[Minchan Kim]

Hello Weijie,

Post by Weijie Yang

Post by Minchan Kim

Post by Weijie Yang

Post by Minchan Kim
Hello Weigie,

Post by Weijie Yang

Post by Bob Liu

Post by Weijie Yang
I think I find a new issue, for integrity of this mail thread, I reply
to this mail.
It is a concurrence issue either, when duplicate store and reclaim
concurrentlly.
zswap entry x with offset A is already stored in zswap backend.
thread 0: reclaim entry x (get refcount, but not call zswap_get_swap_cache_page)
thread 1: store new page with the same offset A, alloc a new zswap entry y.
store finished. shrink_page_list() call __remove_mapping(), and now
it is not in swap_cache

But I don't think swap layer will call zswap with the same offset A.

1. store page of offset A in zswap
2. some time later, pagefault occur, load page data from zswap.
But notice that zswap entry x is still in zswap because it is not
frontswap_tmem_exclusive_gets_enabled.

frontswap_tmem_exclusive_gets_enabled is just option to see tradeoff
between CPU burining by frequent swapout and memory footprint by duplicate
copy in swap cache and frontswap backend so it shouldn't affect the stability.

Thanks for explain this.
I don't mean to say this option affects the stability, but that zswap
only realize
one option. Maybe it's better to realize both options for different workloads.

"zswap only relize one option"
What does it mena? Sorry. I couldn't parse your intention. :)
You mean zswap should do something special to support frontswap_tmem_exclusive_gets?

Yes. But I am not sure whether it is worth.

Post by Minchan Kim

Post by Weijie Yang

Post by Minchan Kim

Post by Weijie Yang
this page is with PageSwapCache(page) and page_private(page) = entry.val
3. change this page data, and it become dirty

If non-shared swapin page become redirty, it should remove the page from
swapcache. If shared swapin page become redirty, it should do CoW so it's a
new page so that it doesn't live in swap cache. It means it should have new
offset which is different with old's one for swap out.
What's wrong with that?

It is really not a right scene for duplicate store. And I can not think out one.
If duplicate store is impossible, How about delete the handle code in zswap?
If it does exist, I think there is a potential issue as I described.

You mean "zswap_duplicate_entry"?
AFAIR, I already had a question to Seth when zswap was born but AFAIRC,
he said that he didn't know exact reason but he saw that case during
experiement so copy the code peice from zcache.
Do you see the case, too?

Yes, I mean duplicate store.
I check the /Documentation/vm/frontswap.txt, it mentions "duplicate stores",
but I am still confused.

It seems that there are two Minchan in LKML.
Other Minchan, not me who have a horrible memory, already was first to
figure it out a few month ago.

https://lkml.org/lkml/2013/1/31/3

/me slaps self.
I'd like to look into that issue more but now I don't have a time.
Just FYI. ;-)