Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754384AbdCWDDm (ORCPT ); Wed, 22 Mar 2017 23:03:42 -0400 Received: from LGEAMRELO13.lge.com ([156.147.23.53]:36338 "EHLO lgeamrelo13.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751296AbdCWDDf (ORCPT ); Wed, 22 Mar 2017 23:03:35 -0400 X-Original-SENDERIP: 156.147.1.151 X-Original-MAILFROM: iamjoonsoo.kim@lge.com X-Original-SENDERIP: 165.244.249.23 X-Original-MAILFROM: iamjoonsoo.kim@lge.com X-Original-SENDERIP: 10.177.222.138 X-Original-MAILFROM: iamjoonsoo.kim@lge.com Date: Thu, 23 Mar 2017 12:05:30 +0900 From: Joonsoo Kim To: Minchan Kim CC: Andrew Morton , Sergey Senozhatsky , , Subject: Re: [PATCH 4/4] zram: make deduplication feature optional Message-ID: <20170323030530.GC17486@js1304-P5Q-DELUXE> References: <1489632398-31501-1-git-send-email-iamjoonsoo.kim@lge.com> <1489632398-31501-5-git-send-email-iamjoonsoo.kim@lge.com> <20170322000059.GB30149@bbox> MIME-Version: 1.0 In-Reply-To: <20170322000059.GB30149@bbox> User-Agent: Mutt/1.5.21 (2010-09-15) X-MIMETrack: Itemize by SMTP Server on LGEKRMHUB05/LGE/LG Group(Release 8.5.3FP6|November 21, 2013) at 2017/03/23 12:03:31, Serialize by Router on LGEKRMHUB05/LGE/LG Group(Release 8.5.3FP6|November 21, 2013) at 2017/03/23 12:03:31, Serialize complete at 2017/03/23 12:03:31 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8639 Lines: 264 On Wed, Mar 22, 2017 at 09:00:59AM +0900, Minchan Kim wrote: > On Thu, Mar 16, 2017 at 11:46:38AM +0900, js1304@gmail.com wrote: > > From: Joonsoo Kim > > > > Benefit of deduplication is dependent on the workload so it's not > > preferable to always enable. Therefore, make it optional. > > Please make it to Kconfig, too. And write down the description to impress > "help a lot for users who uses zram to build output directory" > And the feature should be disabled as default. Okay. > > > > > Signed-off-by: Joonsoo Kim > > --- > > drivers/block/zram/zram_drv.c | 80 ++++++++++++++++++++++++++++++++++++++----- > > drivers/block/zram/zram_drv.h | 1 + > > 2 files changed, 73 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > > index 012425f..e45aa9f 100644 > > --- a/drivers/block/zram/zram_drv.c > > +++ b/drivers/block/zram/zram_drv.c > > @@ -328,6 +328,39 @@ static ssize_t comp_algorithm_store(struct device *dev, > > return len; > > } > > > > +static ssize_t use_dedup_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + int val; > > + struct zram *zram = dev_to_zram(dev); > > + > > + down_read(&zram->init_lock); > > + val = zram->use_dedup; > > + up_read(&zram->init_lock); > > + > > + return scnprintf(buf, PAGE_SIZE, "%d\n", val); > > +} > > + > > +static ssize_t use_dedup_store(struct device *dev, > > + struct device_attribute *attr, const char *buf, size_t len) > > +{ > > + int val; > > + struct zram *zram = dev_to_zram(dev); > > + > > + if (kstrtoint(buf, 10, &val) || (val != 0 && val != 1)) > > + return -EINVAL; > > + > > + down_write(&zram->init_lock); > > + if (init_done(zram)) { > > + up_write(&zram->init_lock); > > + pr_info("Can't change dedup usage for initialized device\n"); > > + return -EBUSY; > > + } > > + zram->use_dedup = val; > > + up_write(&zram->init_lock); > > + return len; > > +} > > + > > static ssize_t compact_store(struct device *dev, > > struct device_attribute *attr, const char *buf, size_t len) > > { > > @@ -422,11 +455,23 @@ static ssize_t debug_stat_show(struct device *dev, > > static DEVICE_ATTR_RO(mm_stat); > > static DEVICE_ATTR_RO(debug_stat); > > > > -static u32 zram_calc_checksum(unsigned char *mem) > > +static u32 zram_calc_checksum(struct zram *zram, unsigned char *mem) > > { > > + if (!zram->use_dedup) > > + return 0; > > + > > Hmm, I don't like this style which every dedup functions have the check > "use_dedup". > > Can't we abstract like this? I will try but I'm not sure it can be. > > I want to find more simple way to no need to add the check when new dedup > functions pop up. > > bool zram_dedup_check > if (!zram->dedup) > return false; > > zram_dedup_checksum > entry = zram_dedup_get > if (!entry) { > zram_dedup_add > return false; > } > .. > return true; > > > zram_bvec_write: > ... > ... > > if (zram_dedup_match()) > goto found_dup; > > > > > return jhash(mem, PAGE_SIZE, 0); > > } > > > > +static unsigned long zram_entry_handle(struct zram *zram, > > + struct zram_entry *entry) > > +{ > > + if (!zram->use_dedup) > > + return (unsigned long)entry; > > + > > + return entry->handle; > > +} > > + > > static struct zram_entry *zram_entry_alloc(struct zram *zram, > > unsigned int len, gfp_t flags) > > { > > @@ -438,6 +483,9 @@ static struct zram_entry *zram_entry_alloc(struct zram *zram, > > if (!handle) > > return NULL; > > > > + if (!zram->use_dedup) > > + return (struct zram_entry *)handle; > > + > > entry = kzalloc(sizeof(*entry), flags); > > if (!entry) { > > zs_free(meta->mem_pool, handle); > > @@ -462,6 +510,9 @@ static void zram_entry_insert(struct zram *zram, struct zram_entry *new, > > struct rb_node **rb_node, *parent = NULL; > > struct zram_entry *entry; > > > > + if (!zram->use_dedup) > > + return; > > + > > new->checksum = checksum; > > hash = &meta->hash[checksum % meta->hash_size]; > > rb_root = &hash->rb_root; > > @@ -492,7 +543,8 @@ static bool zram_entry_match(struct zram *zram, struct zram_entry *entry, > > struct zram_meta *meta = zram->meta; > > struct zcomp_strm *zstrm; > > > > - cmem = zs_map_object(meta->mem_pool, entry->handle, ZS_MM_RO); > > + cmem = zs_map_object(meta->mem_pool, > > + zram_entry_handle(zram, entry), ZS_MM_RO); > > if (entry->len == PAGE_SIZE) { > > match = !memcmp(mem, cmem, PAGE_SIZE); > > } else { > > @@ -501,7 +553,7 @@ static bool zram_entry_match(struct zram *zram, struct zram_entry *entry, > > match = !memcmp(mem, zstrm->buffer, PAGE_SIZE); > > zcomp_stream_put(zram->comp); > > } > > - zs_unmap_object(meta->mem_pool, entry->handle); > > + zs_unmap_object(meta->mem_pool, zram_entry_handle(zram, entry)); > > > > return match; > > } > > @@ -521,6 +573,11 @@ static bool zram_entry_put(struct zram *zram, struct zram_meta *meta, > > struct zram_hash *hash; > > u32 checksum; > > > > + if (!zram->use_dedup) { > > + zs_free(meta->mem_pool, zram_entry_handle(zram, entry)); > > + return false; > > + } > > + > > if (!populated) > > goto free; > > > > @@ -551,6 +608,9 @@ static struct zram_entry *zram_entry_get(struct zram *zram, > > struct zram_entry *entry; > > struct rb_node *rb_node; > > > > + if (!zram->use_dedup) > > + return NULL; > > + > > hash = &meta->hash[checksum % meta->hash_size]; > > > > spin_lock(&hash->lock); > > @@ -713,7 +773,8 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index) > > return 0; > > } > > > > - cmem = zs_map_object(meta->mem_pool, entry->handle, ZS_MM_RO); > > + cmem = zs_map_object(meta->mem_pool, > > + zram_entry_handle(zram, entry), ZS_MM_RO); > > if (size == PAGE_SIZE) { > > copy_page(mem, cmem); > > } else { > > @@ -722,7 +783,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index) > > ret = zcomp_decompress(zstrm, cmem, size, mem); > > zcomp_stream_put(zram->comp); > > } > > - zs_unmap_object(meta->mem_pool, entry->handle); > > + zs_unmap_object(meta->mem_pool, zram_entry_handle(zram, entry)); > > bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value); > > > > /* Should NEVER happen. Return bio error if it does. */ > > @@ -840,7 +901,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index, > > goto out; > > } > > > > - checksum = zram_calc_checksum(uncmem); > > + checksum = zram_calc_checksum(zram, uncmem); > > if (!entry) { > > entry = zram_entry_get(zram, uncmem, checksum); > > if (entry) { > > @@ -915,7 +976,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index, > > goto out; > > } > > > > - cmem = zs_map_object(meta->mem_pool, entry->handle, ZS_MM_WO); > > + cmem = zs_map_object(meta->mem_pool, > > + zram_entry_handle(zram, entry), ZS_MM_WO); > > > > if ((clen == PAGE_SIZE) && !is_partial_io(bvec)) { > > src = kmap_atomic(page); > > @@ -927,7 +989,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index, > > > > zcomp_stream_put(zram->comp); > > zstrm = NULL; > > - zs_unmap_object(meta->mem_pool, entry->handle); > > + zs_unmap_object(meta->mem_pool, zram_entry_handle(zram, entry)); > > zram_entry_insert(zram, entry, checksum); > > > > found_duplication: > > @@ -1310,6 +1372,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode) > > static DEVICE_ATTR_WO(mem_used_max); > > static DEVICE_ATTR_RW(max_comp_streams); > > static DEVICE_ATTR_RW(comp_algorithm); > > +static DEVICE_ATTR_RW(use_dedup); > > > > static struct attribute *zram_disk_attrs[] = { > > &dev_attr_disksize.attr, > > @@ -1320,6 +1383,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode) > > &dev_attr_mem_used_max.attr, > > &dev_attr_max_comp_streams.attr, > > &dev_attr_comp_algorithm.attr, > > + &dev_attr_use_dedup.attr, > > &dev_attr_io_stat.attr, > > &dev_attr_mm_stat.attr, > > &dev_attr_debug_stat.attr, > > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h > > index 07d1f8d..b823555 100644 > > --- a/drivers/block/zram/zram_drv.h > > +++ b/drivers/block/zram/zram_drv.h > > @@ -141,5 +141,6 @@ struct zram { > > * zram is claimed so open request will be failed > > */ > > bool claim; /* Protected by bdev->bd_mutex */ > > + int use_dedup; > > For binary result, I want to use 'bool' Okay. Thanks.