From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752502AbdBGCyg (ORCPT ); Mon, 6 Feb 2017 21:54:36 -0500 Received: from LGEAMRELO12.lge.com ([156.147.23.52]:34582 "EHLO lgeamrelo12.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751592AbdBGCyf (ORCPT ); Mon, 6 Feb 2017 21:54:35 -0500 X-Original-SENDERIP: 156.147.1.126 X-Original-MAILFROM: minchan@kernel.org X-Original-SENDERIP: 165.244.249.26 X-Original-MAILFROM: minchan@kernel.org X-Original-SENDERIP: 10.177.223.161 X-Original-MAILFROM: minchan@kernel.org Date: Tue, 7 Feb 2017 11:54:26 +0900 From: Minchan Kim To: zhouxianrong CC: , , , , , , , , , , , Subject: Re: [PATCH] mm: extend zero pages to same element pages for zram Message-ID: <20170207025426.GA1528@bbox> References: <1483692145-75357-1-git-send-email-zhouxianrong@huawei.com> <1486111347-112972-1-git-send-email-zhouxianrong@huawei.com> <20170205142100.GA9611@bbox> <2f6e188c-5358-eeab-44ab-7634014af651@huawei.com> <20170206234805.GA12188@bbox> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-MIMETrack: Itemize by SMTP Server on LGEKRMHUB07/LGE/LG Group(Release 8.5.3FP6|November 21, 2013) at 2017/02/07 11:54:27, Serialize by Router on LGEKRMHUB07/LGE/LG Group(Release 8.5.3FP6|November 21, 2013) at 2017/02/07 11:54:27, Serialize complete at 2017/02/07 11:54:27 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 On Tue, Feb 07, 2017 at 10:20:57AM +0800, zhouxianrong wrote: < snip > > >>3. the below should be modified. > >> > >>static inline bool zram_meta_get(struct zram *zram) > >>@@ -495,11 +553,17 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize) > >> > >> /* Free all pages that are still in this zram device */ > >> for (index = 0; index < num_pages; index++) { > >>- unsigned long handle = meta->table[index].handle; > >>+ unsigned long handle; > >>+ > >>+ bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value); > >>+ handle = meta->table[index].handle; > >> > >>- if (!handle) > >>+ if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) { > >>+ bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value); > >> continue; > >>+ } > >> > >>+ bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value); > >> zs_free(meta->mem_pool, handle); > > > >Could you explain why we need this modification? > > > >> } > >> > >>@@ -511,7 +575,7 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize) > >> static struct zram_meta *zram_meta_alloc(char *pool_name, u64 disksize) > >> { > >> size_t num_pages; > >>- struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL); > >>+ struct zram_meta *meta = kzalloc(sizeof(*meta), GFP_KERNEL); > > > >Ditto > > > >> > >> > > > >. > > > > because of union of handle and element, i think a non-zero element (other than handle) is prevented from freeing. > if zram_meta_get was modified, zram_meta_alloc did so. Right. Thanks but I don't see why we need the locking in there and modification of zram_meta_alloc. Isn't it enough with this? diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index c20b05a84f21..a25d34a8af19 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -425,8 +425,11 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize) /* Free all pages that are still in this zram device */ for (index = 0; index < num_pages; index++) { unsigned long handle = meta->table[index].handle; - - if (!handle) + /* + * No memory is allocated for same element filled pages. + * Simply clear same page flag. + */ + if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) continue; zs_free(meta->mem_pool, handle);