Date: Tue, 28 Mar 2017 11:50:45 +0900
From: Minchan Kim <minchan@kernel.org>
To: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
CC: Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        <linux-kernel@vger.kernel.org>, <kernel-team@lge.com>,
        Seth Jennings <sjenning@redhat.com>, Dan Streetman <ddstreet@ieee.org>
Subject: Re: [PATCH 4/4] zram: make deduplication feature optional
Message-ID: <20170328025045.GA8573@bbox>
References: <1489632398-31501-1-git-send-email-iamjoonsoo.kim@lge.com>
 <1489632398-31501-5-git-send-email-iamjoonsoo.kim@lge.com>
 <20170322000059.GB30149@bbox>
 <20170323030530.GC17486@js1304-P5Q-DELUXE>
 <20170327081105.GA390@jagdpanzerIV.localdomain>
 <20170328010217.GB8462@js1304-P5Q-DELUXE>
 <20170328022244.GB10573@jagdpanzerIV.localdomain>
MIME-Version: 1.0
In-Reply-To: <20170328022244.GB10573@jagdpanzerIV.localdomain>
User-Agent: Mutt/1.5.24 (2015-08-30)
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2875
Lines: 69

Hi Sergey,

On Tue, Mar 28, 2017 at 11:22:45AM +0900, Sergey Senozhatsky wrote:
> Cc Seth and Dan, just in case
> 
> 
> Hello Joonsoo,
> 
> On (03/28/17 10:02), Joonsoo Kim wrote:
> [..]
> > > so I was thinking for a moment -- do we want to keep this
> > > functionality in zram or may be it belongs to allocator (zsmalloc)?
> > > what do you think? just a question.
> > 
> > I think that zram is more appropriate layer to implement this feature.
> > I may be wrong so please let me know if I'm missing something.
> > 
> > First, I'd like to leave allocator to just allocator. If it awares the
> > contents, further improvement would be restricted. For example, we
> > should use map/unmap semantic to store contents, since, without them,
> > we can't know when the content is changed and when deduplication check
> > should be done. I know that zsmalloc is already implemented by that
> > way but I guess that similar issue could happen in the future.
> > 
> > Second, we always need to compress the page to check duplication
> > if it is implemented in zsmalloc since we store compressed page to
> > zsmalloc. I guess that less compression would be better in performance
> > wise.
> > 
> > Third, in case of zsmalloc dedup, we always need to allocate zs memory
> > before checking duplication and need to free it if duplication is
> > found. It's also undesirable.
> > 
> > If you are okay with above arguments, I will send v2 soon.
> 
> thanks.
> I'm OK with your arguments.
> 
> 
> to explain my point a bit further (zsmalloc was a bad call,
> I guess I meant zpool):
> 
> the reason I asked was that both zram and zswap sort of trying to
> have same optimizations - zero filled pages handling, for example.
> zram is a bit ahead now (to the best of my knowledge), because of
> the recent 'same element' filled pages. zswap, probably, will have
> something like this as well some day. or may be it won't, up to Seth
> and Dan. de-duplication definitely can improve both zram and zswap,
> which, once again, suggests that at some point zswap will have its
> own implementation. well, or it won't.

As I pointed out, at least, dedup was no benefit for the swap case.
I don't want to disrupt zsmalloc without any *proved* benefit.
Even though it *might* have benefit, it shouldn't be in allocator
layer unless it's really huge benefit like performance.
It makes hard zram's allocator change in future.
And please consider zswap is born for the latency in server workload
while zram is memory efficiency in embedded world.
dedup feature is trade-off for them and zram is perfectly matched.

> 
> so I though that may be we could have zero filled pages handling/same
> element pages handling/de-duplication somewhere in the "middle" layer.
> like zpool for instance (zram does not support zpool as of now) so we
> could unify things.
> 
> just an idea. no pressure.
> 
> 	-ss