* Re: [Patch 3.11.7 1/1]mm: remove and free expired data in time in zswap
@ 2013-11-08 10:28 ` Bob Liu
0 siblings, 0 replies; 9+ messages in thread
From: Bob Liu @ 2013-11-08 10:28 UTC (permalink / raw)
To: changkun.li; +Cc: sjenning, linux-mm, luyi, lichangkun, linux-kernel
On 11/08/2013 05:50 PM, changkun.li wrote:
> In zswap, store page A to zbud if the compression ratio is high, insert
> its entry into rbtree. if there is a entry B which has the same offset
> in the rbtree.Remove and free B before insert the entry of A.
>
> case:
> if the compression ratio of page A is not high, return without checking
> the same offset one in rbtree.
>
> if there is a entry B which has the same offset in the rbtree. Now, we
> make sure B is invalid or expired. But the entry and compressed memory
> of B are not freed in time.
>
> Because zswap spaces data in memory, it makes the utilization of memory
> lower. the other valid data in zbud is writeback to swap device more
> possibility, when zswap is full.
>
> So if we make sure a entry is expired, free it in time.
>
> Signed-off-by: changkun.li<xfishcoder@gmail.com>
> ---
> mm/zswap.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index cbd9578..90a2813 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -596,6 +596,7 @@ fail:
> return ret;
> }
>
> +static void zswap_frontswap_invalidate_page(unsigned type, pgoff_t
> offset);
> /*********************************
> * frontswap hooks
> **********************************/
> @@ -614,7 +615,7 @@ static int zswap_frontswap_store(unsigned type,
> pgoff_t offset,
>
> if (!tree) {
> ret = -ENODEV;
> - goto reject;
> + goto nodev;
> }
>
> /* reclaim space if needed */
> @@ -695,6 +696,8 @@ freepage:
> put_cpu_var(zswap_dstmem);
> zswap_entry_cache_free(entry);
> reject:
> + zswap_frontswap_invalidate_page(type, offset);
I'm afraid when arrives here zswap_rb_search(offset) will always return
NULL entry. So most of the time, it's just waste time to call
zswap_frontswap_invalidate_page() to search rbtree.
--
Regards,
-Bob
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Patch 3.11.7 1/1]mm: remove and free expired data in time in zswap
2013-11-08 10:28 ` Bob Liu
(?)
@ 2013-11-08 10:53 ` changkun.li
-1 siblings, 0 replies; 9+ messages in thread
From: changkun.li @ 2013-11-08 10:53 UTC (permalink / raw)
To: Bob Liu; +Cc: sjenning, linux-mm, luyi, lichangkun, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3594 bytes --]
I agree with you, I see the __frontswap_store in 'frontswap.c', it knows
the same offset entry exist in zswap. we could modify like this?:
int __frontswap_store(struct page *page)
{
int ret = -1, dup = 0;
swp_entry_t entry = { .val = page_private(page), };
int type = swp_type(entry);
struct swap_info_struct *sis = swap_info[type];
pgoff_t offset = swp_offset(entry);
/*
* Return if no backend registed.
* Don't need to inc frontswap_failed_stores here.
*/
if (!frontswap_ops)
return ret;
BUG_ON(!PageLocked(page));
BUG_ON(sis == NULL);
if (__frontswap_test(sis, offset))
dup = 1;
ret = frontswap_ops->store(type, offset, page);
if (ret == 0) {
set_bit(offset, sis->frontswap_map);
inc_frontswap_succ_stores();
if (!dup)
atomic_inc(&sis->frontswap_pages);
} else {
/*
failed dup always results in automatic invalidate of
the (older) page from frontswap
*/
inc_frontswap_failed_stores();
++ if (dup) {
-- if (dup)
__frontswap_clear(sis, offset);
++ __frontswap_invalidate_page(type, offset);
++ }
}
if (frontswap_writethrough_enabled)
/* report failure so swap also writes to swap device */
ret = -1;
return ret;
}
but maybe the other frontswap modules is not space memory, they need not
call __frontswap_invalidate_page. so the code in here is only better for
zswap.
On Fri, Nov 8, 2013 at 6:28 PM, Bob Liu <bob.liu@oracle.com> wrote:
> On 11/08/2013 05:50 PM, changkun.li wrote:
> > In zswap, store page A to zbud if the compression ratio is high, insert
> > its entry into rbtree. if there is a entry B which has the same offset
> > in the rbtree.Remove and free B before insert the entry of A.
> >
> > case:
> > if the compression ratio of page A is not high, return without checking
> > the same offset one in rbtree.
> >
> > if there is a entry B which has the same offset in the rbtree. Now, we
> > make sure B is invalid or expired. But the entry and compressed memory
> > of B are not freed in time.
> >
> > Because zswap spaces data in memory, it makes the utilization of memory
> > lower. the other valid data in zbud is writeback to swap device more
> > possibility, when zswap is full.
> >
> > So if we make sure a entry is expired, free it in time.
> >
> > Signed-off-by: changkun.li<xfishcoder@gmail.com>
> > ---
> > mm/zswap.c | 5 ++++-
> > 1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index cbd9578..90a2813 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -596,6 +596,7 @@ fail:
> > return ret;
> > }
> >
> > +static void zswap_frontswap_invalidate_page(unsigned type, pgoff_t
> > offset);
> > /*********************************
> > * frontswap hooks
> > **********************************/
> > @@ -614,7 +615,7 @@ static int zswap_frontswap_store(unsigned type,
> > pgoff_t offset,
> >
> > if (!tree) {
> > ret = -ENODEV;
> > - goto reject;
> > + goto nodev;
> > }
> >
> > /* reclaim space if needed */
> > @@ -695,6 +696,8 @@ freepage:
> > put_cpu_var(zswap_dstmem);
> > zswap_entry_cache_free(entry);
> > reject:
> > + zswap_frontswap_invalidate_page(type, offset);
>
> I'm afraid when arrives here zswap_rb_search(offset) will always return
> NULL entry. So most of the time, it's just waste time to call
> zswap_frontswap_invalidate_page() to search rbtree.
>
> --
> Regards,
> -Bob
>
[-- Attachment #2: Type: text/html, Size: 4663 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Patch 3.11.7 1/1]mm: remove and free expired data in time in zswap
2013-11-08 10:28 ` Bob Liu
@ 2013-11-18 7:06 ` Weijie Yang
-1 siblings, 0 replies; 9+ messages in thread
From: Weijie Yang @ 2013-11-18 7:06 UTC (permalink / raw)
To: Bob Liu
Cc: changkun.li, sjenning, linux-mm, luyi, lichangkun, linux-kernel,
Seth Jennings
add cc Seth 's new email address.
On Fri, Nov 8, 2013 at 6:28 PM, Bob Liu <bob.liu@oracle.com> wrote:
> On 11/08/2013 05:50 PM, changkun.li wrote:
>> In zswap, store page A to zbud if the compression ratio is high, insert
>> its entry into rbtree. if there is a entry B which has the same offset
>> in the rbtree.Remove and free B before insert the entry of A.
>>
>> case:
>> if the compression ratio of page A is not high, return without checking
>> the same offset one in rbtree.
>>
>> if there is a entry B which has the same offset in the rbtree. Now, we
>> make sure B is invalid or expired. But the entry and compressed memory
>> of B are not freed in time.
>>
>> Because zswap spaces data in memory, it makes the utilization of memory
>> lower. the other valid data in zbud is writeback to swap device more
>> possibility, when zswap is full.
>>
>> So if we make sure a entry is expired, free it in time.
>>
>> Signed-off-by: changkun.li<xfishcoder@gmail.com>
>> ---
>> mm/zswap.c | 5 ++++-
>> 1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/zswap.c b/mm/zswap.c
>> index cbd9578..90a2813 100644
>> --- a/mm/zswap.c
>> +++ b/mm/zswap.c
>> @@ -596,6 +596,7 @@ fail:
>> return ret;
>> }
>>
>> +static void zswap_frontswap_invalidate_page(unsigned type, pgoff_t
>> offset);
>> /*********************************
>> * frontswap hooks
>> **********************************/
>> @@ -614,7 +615,7 @@ static int zswap_frontswap_store(unsigned type,
>> pgoff_t offset,
>>
>> if (!tree) {
>> ret = -ENODEV;
>> - goto reject;
>> + goto nodev;
>> }
>>
>> /* reclaim space if needed */
>> @@ -695,6 +696,8 @@ freepage:
>> put_cpu_var(zswap_dstmem);
>> zswap_entry_cache_free(entry);
>> reject:
>> + zswap_frontswap_invalidate_page(type, offset);
>
> I'm afraid when arrives here zswap_rb_search(offset) will always return
> NULL entry. So most of the time, it's just waste time to call
> zswap_frontswap_invalidate_page() to search rbtree.
Yes, it is a bug.
But I agree with Bob, this patch is not efficient.
How about like this?
diff --git a/mm/frontswap.c b/mm/frontswap.c
index 1b24bdc..1227896
--- a/mm/frontswap.c
+++ b/mm/frontswap.c
@@ -244,8 +244,10 @@ int __frontswap_store(struct page *page)
the (older) page from frontswap
*/
inc_frontswap_failed_stores();
- if (dup)
+ if (dup) {
+ frontswap_ops->invalidate_page(type, offset);
__frontswap_clear(sis, offset);
+ }
}
if (frontswap_writethrough_enabled)
/* report failure so swap also writes to swap device */
> --
> Regards,
> -Bob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Patch 3.11.7 1/1]mm: remove and free expired data in time in zswap
@ 2013-11-18 7:06 ` Weijie Yang
0 siblings, 0 replies; 9+ messages in thread
From: Weijie Yang @ 2013-11-18 7:06 UTC (permalink / raw)
To: Bob Liu
Cc: changkun.li, sjenning, linux-mm, luyi, lichangkun, linux-kernel,
Seth Jennings
add cc Seth 's new email address.
On Fri, Nov 8, 2013 at 6:28 PM, Bob Liu <bob.liu@oracle.com> wrote:
> On 11/08/2013 05:50 PM, changkun.li wrote:
>> In zswap, store page A to zbud if the compression ratio is high, insert
>> its entry into rbtree. if there is a entry B which has the same offset
>> in the rbtree.Remove and free B before insert the entry of A.
>>
>> case:
>> if the compression ratio of page A is not high, return without checking
>> the same offset one in rbtree.
>>
>> if there is a entry B which has the same offset in the rbtree. Now, we
>> make sure B is invalid or expired. But the entry and compressed memory
>> of B are not freed in time.
>>
>> Because zswap spaces data in memory, it makes the utilization of memory
>> lower. the other valid data in zbud is writeback to swap device more
>> possibility, when zswap is full.
>>
>> So if we make sure a entry is expired, free it in time.
>>
>> Signed-off-by: changkun.li<xfishcoder@gmail.com>
>> ---
>> mm/zswap.c | 5 ++++-
>> 1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/zswap.c b/mm/zswap.c
>> index cbd9578..90a2813 100644
>> --- a/mm/zswap.c
>> +++ b/mm/zswap.c
>> @@ -596,6 +596,7 @@ fail:
>> return ret;
>> }
>>
>> +static void zswap_frontswap_invalidate_page(unsigned type, pgoff_t
>> offset);
>> /*********************************
>> * frontswap hooks
>> **********************************/
>> @@ -614,7 +615,7 @@ static int zswap_frontswap_store(unsigned type,
>> pgoff_t offset,
>>
>> if (!tree) {
>> ret = -ENODEV;
>> - goto reject;
>> + goto nodev;
>> }
>>
>> /* reclaim space if needed */
>> @@ -695,6 +696,8 @@ freepage:
>> put_cpu_var(zswap_dstmem);
>> zswap_entry_cache_free(entry);
>> reject:
>> + zswap_frontswap_invalidate_page(type, offset);
>
> I'm afraid when arrives here zswap_rb_search(offset) will always return
> NULL entry. So most of the time, it's just waste time to call
> zswap_frontswap_invalidate_page() to search rbtree.
Yes, it is a bug.
But I agree with Bob, this patch is not efficient.
How about like this?
diff --git a/mm/frontswap.c b/mm/frontswap.c
index 1b24bdc..1227896
--- a/mm/frontswap.c
+++ b/mm/frontswap.c
@@ -244,8 +244,10 @@ int __frontswap_store(struct page *page)
the (older) page from frontswap
*/
inc_frontswap_failed_stores();
- if (dup)
+ if (dup) {
+ frontswap_ops->invalidate_page(type, offset);
__frontswap_clear(sis, offset);
+ }
}
if (frontswap_writethrough_enabled)
/* report failure so swap also writes to swap device */
> --
> Regards,
> -Bob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Patch 3.11.7 1/1]mm: remove and free expired data in time in zswap
2013-11-18 7:06 ` Weijie Yang
@ 2014-01-10 9:43 ` Weijie Yang
-1 siblings, 0 replies; 9+ messages in thread
From: Weijie Yang @ 2014-01-10 9:43 UTC (permalink / raw)
To: Seth Jennings
Cc: changkun.li, Linux-MM, luyi, lichangkun, Linux-Kernel, Bob Liu,
Seth Jennings
Hi, Seth
Would you please check this issue? Thanks
On Mon, Nov 18, 2013 at 3:06 PM, Weijie Yang <weijie.yang.kh@gmail.com> wrote:
> add cc Seth 's new email address.
>
> On Fri, Nov 8, 2013 at 6:28 PM, Bob Liu <bob.liu@oracle.com> wrote:
>> On 11/08/2013 05:50 PM, changkun.li wrote:
>>> In zswap, store page A to zbud if the compression ratio is high, insert
>>> its entry into rbtree. if there is a entry B which has the same offset
>>> in the rbtree.Remove and free B before insert the entry of A.
>>>
>>> case:
>>> if the compression ratio of page A is not high, return without checking
>>> the same offset one in rbtree.
>>>
>>> if there is a entry B which has the same offset in the rbtree. Now, we
>>> make sure B is invalid or expired. But the entry and compressed memory
>>> of B are not freed in time.
>>>
>>> Because zswap spaces data in memory, it makes the utilization of memory
>>> lower. the other valid data in zbud is writeback to swap device more
>>> possibility, when zswap is full.
>>>
>>> So if we make sure a entry is expired, free it in time.
>>>
>>> Signed-off-by: changkun.li<xfishcoder@gmail.com>
>>> ---
>>> mm/zswap.c | 5 ++++-
>>> 1 files changed, 4 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/mm/zswap.c b/mm/zswap.c
>>> index cbd9578..90a2813 100644
>>> --- a/mm/zswap.c
>>> +++ b/mm/zswap.c
>>> @@ -596,6 +596,7 @@ fail:
>>> return ret;
>>> }
>>>
>>> +static void zswap_frontswap_invalidate_page(unsigned type, pgoff_t
>>> offset);
>>> /*********************************
>>> * frontswap hooks
>>> **********************************/
>>> @@ -614,7 +615,7 @@ static int zswap_frontswap_store(unsigned type,
>>> pgoff_t offset,
>>>
>>> if (!tree) {
>>> ret = -ENODEV;
>>> - goto reject;
>>> + goto nodev;
>>> }
>>>
>>> /* reclaim space if needed */
>>> @@ -695,6 +696,8 @@ freepage:
>>> put_cpu_var(zswap_dstmem);
>>> zswap_entry_cache_free(entry);
>>> reject:
>>> + zswap_frontswap_invalidate_page(type, offset);
>>
>> I'm afraid when arrives here zswap_rb_search(offset) will always return
>> NULL entry. So most of the time, it's just waste time to call
>> zswap_frontswap_invalidate_page() to search rbtree.
>
> Yes, it is a bug.
>
> But I agree with Bob, this patch is not efficient.
>
> How about like this?
>
> diff --git a/mm/frontswap.c b/mm/frontswap.c
> index 1b24bdc..1227896
> --- a/mm/frontswap.c
> +++ b/mm/frontswap.c
> @@ -244,8 +244,10 @@ int __frontswap_store(struct page *page)
> the (older) page from frontswap
> */
> inc_frontswap_failed_stores();
> - if (dup)
> + if (dup) {
> + frontswap_ops->invalidate_page(type, offset);
> __frontswap_clear(sis, offset);
> + }
> }
> if (frontswap_writethrough_enabled)
> /* report failure so swap also writes to swap device */
>
>
>> --
>> Regards,
>> -Bob
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Patch 3.11.7 1/1]mm: remove and free expired data in time in zswap
@ 2014-01-10 9:43 ` Weijie Yang
0 siblings, 0 replies; 9+ messages in thread
From: Weijie Yang @ 2014-01-10 9:43 UTC (permalink / raw)
To: Seth Jennings
Cc: changkun.li, Linux-MM, luyi, lichangkun, Linux-Kernel, Bob Liu,
Seth Jennings
Hi, Seth
Would you please check this issue? Thanks
On Mon, Nov 18, 2013 at 3:06 PM, Weijie Yang <weijie.yang.kh@gmail.com> wrote:
> add cc Seth 's new email address.
>
> On Fri, Nov 8, 2013 at 6:28 PM, Bob Liu <bob.liu@oracle.com> wrote:
>> On 11/08/2013 05:50 PM, changkun.li wrote:
>>> In zswap, store page A to zbud if the compression ratio is high, insert
>>> its entry into rbtree. if there is a entry B which has the same offset
>>> in the rbtree.Remove and free B before insert the entry of A.
>>>
>>> case:
>>> if the compression ratio of page A is not high, return without checking
>>> the same offset one in rbtree.
>>>
>>> if there is a entry B which has the same offset in the rbtree. Now, we
>>> make sure B is invalid or expired. But the entry and compressed memory
>>> of B are not freed in time.
>>>
>>> Because zswap spaces data in memory, it makes the utilization of memory
>>> lower. the other valid data in zbud is writeback to swap device more
>>> possibility, when zswap is full.
>>>
>>> So if we make sure a entry is expired, free it in time.
>>>
>>> Signed-off-by: changkun.li<xfishcoder@gmail.com>
>>> ---
>>> mm/zswap.c | 5 ++++-
>>> 1 files changed, 4 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/mm/zswap.c b/mm/zswap.c
>>> index cbd9578..90a2813 100644
>>> --- a/mm/zswap.c
>>> +++ b/mm/zswap.c
>>> @@ -596,6 +596,7 @@ fail:
>>> return ret;
>>> }
>>>
>>> +static void zswap_frontswap_invalidate_page(unsigned type, pgoff_t
>>> offset);
>>> /*********************************
>>> * frontswap hooks
>>> **********************************/
>>> @@ -614,7 +615,7 @@ static int zswap_frontswap_store(unsigned type,
>>> pgoff_t offset,
>>>
>>> if (!tree) {
>>> ret = -ENODEV;
>>> - goto reject;
>>> + goto nodev;
>>> }
>>>
>>> /* reclaim space if needed */
>>> @@ -695,6 +696,8 @@ freepage:
>>> put_cpu_var(zswap_dstmem);
>>> zswap_entry_cache_free(entry);
>>> reject:
>>> + zswap_frontswap_invalidate_page(type, offset);
>>
>> I'm afraid when arrives here zswap_rb_search(offset) will always return
>> NULL entry. So most of the time, it's just waste time to call
>> zswap_frontswap_invalidate_page() to search rbtree.
>
> Yes, it is a bug.
>
> But I agree with Bob, this patch is not efficient.
>
> How about like this?
>
> diff --git a/mm/frontswap.c b/mm/frontswap.c
> index 1b24bdc..1227896
> --- a/mm/frontswap.c
> +++ b/mm/frontswap.c
> @@ -244,8 +244,10 @@ int __frontswap_store(struct page *page)
> the (older) page from frontswap
> */
> inc_frontswap_failed_stores();
> - if (dup)
> + if (dup) {
> + frontswap_ops->invalidate_page(type, offset);
> __frontswap_clear(sis, offset);
> + }
> }
> if (frontswap_writethrough_enabled)
> /* report failure so swap also writes to swap device */
>
>
>> --
>> Regards,
>> -Bob
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 9+ messages in thread