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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAL1ERfMTFYH8sw8x4F9BgcVBsEiXG2=DK2FCt3of-QW4nUQBeQ@mail.gmail.com>
Date:	Fri, 10 Jan 2014 17:43:32 +0800
From:	Weijie Yang <weijie.yang.kh@...il.com>
To:	Seth Jennings <sjennings@...iantweb.net>
Cc:	"changkun.li" <xfishcoder@...il.com>,
	Linux-MM <linux-mm@...ck.org>, luyi@....cn, lichangkun@....cn,
	Linux-Kernel <linux-kernel@...r.kernel.org>,
	Bob Liu <bob.liu@...cle.com>,
	Seth Jennings <sjenning@...ux.vnet.ibm.com>
Subject: Re: [Patch 3.11.7 1/1]mm: remove and free expired data in time in zswap

Hi, Seth

Would you please check this issue?  Thanks

On Mon, Nov 18, 2013 at 3:06 PM, Weijie Yang <weijie.yang.kh@...il.com> wrote:
> add cc Seth 's new email address.
>
> On Fri, Nov 8, 2013 at 6:28 PM, Bob Liu <bob.liu@...cle.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@...il.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@...r.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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/