Re: [Patch 3.11.7 1/1]mm: remove and free expired data in time in zswap

["changkun.li" <xfishcoder@xxxxxxxxx>]

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@xxxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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