From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from LGEAMRELO12.lge.com ([156.147.23.52]:43937 "EHLO lgeamrelo12.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760229AbdDSGUS (ORCPT ); Wed, 19 Apr 2017 02:20:18 -0400 Date: Wed, 19 Apr 2017 15:20:14 +0900 From: Minchan Kim To: Sergey Senozhatsky CC: , , , , Subject: Re: FAILED: patch "[PATCH] zram: do not use copy_page with non-page aligned address" failed to apply to 3.18-stable tree Message-ID: <20170419062014.GA1768@bbox> References: <149251790821547@kroah.com> <20170419004547.GB19929@bbox> <20170419050853.GB2881@jagdpanzerIV.localdomain> MIME-Version: 1.0 In-Reply-To: <20170419050853.GB2881@jagdpanzerIV.localdomain> Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline Sender: stable-owner@vger.kernel.org List-ID: On Wed, Apr 19, 2017 at 02:08:53PM +0900, Sergey Senozhatsky wrote: > Hello, > > On (04/19/17 09:45), Minchan Kim wrote: > > index 3920ee45aa59..7e94459a489a 100644 > > --- a/drivers/block/zram/zram_drv.c > > +++ b/drivers/block/zram/zram_drv.c > > @@ -431,13 +431,13 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index) > > > > if (!handle || zram_test_flag(meta, index, ZRAM_ZERO)) { > > bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value); > > - clear_page(mem); > > + memset(mem, 0, PAGE_SIZE); > > after a quick look I haven't found a clear_page() that would impose > alignment requirements. but ok, `mem' _can_ be kmalloc-ed there, yes. > > a side question: > how bad would it hurt to switch to page_size aligned allocator partial > IO instead of slab allocator? and just use potentially faster *_page() > functions instead of mem* functions? I think people normally don't see You meant alloc_page or wanted to create new own slab cache with PAGE_SIZE alighment requirement? Either way, it makes more changes so it would be not proper for -stable material, IMHO. As well, this path(zero-page) would be rare so it wouldn't hurt performance, either. > partial IOs (well, unless they use some 'weird' filesystems). while > copy_page()->memcpy() happens to everyone. > > > > cmem = zs_map_object(meta->mem_pool, handle, ZS_MM_RO); > > if (size == PAGE_SIZE) > > - copy_page(mem, cmem); > > + memcpy(mem, cmem, PAGE_SIZE); > > I think here we have both page_size aligned `mem' and page_size aligned > `cmem'. `mem' is guaranteed to be kmapp-ed ->bv_page, which is allocated > by alloc_page(), and zs_map_object() returns a kmapp-ed alloc_page()-ed > page for huge)class object (which is the case here, isn't it?). so we can > keep copy_page()? am I wrong? I'm afraid we can slow down a regularly No, you're right but as I wrote in description, I don't want to rely on zsmalloc's internal implementation. And this bad compress ratio path would be rare, too. > executed (semi-hot) path here. > > > > @@ -612,7 +612,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index, > > > > if ((clen == PAGE_SIZE) && !is_partial_io(bvec)) { > > src = kmap_atomic(page); > > - copy_page(cmem, src); > > + memcpy(cmem, src, PAGE_SIZE); > > here `src' is kmapp-ed ->bv_page, which is always page_size aligned. > it's allocated by alloc_page(). `cmem' is also page_size aligned, > isn't it? seems that we can use copy_page(). am I missing something? > I'm afraid we can slow down a regularly executed (semi-hot) path here. I understand your concern about slow down but I belive thre are not performance sensitive paths because they are rare so I wanted to bias to safety for -stable material and we can make it fast in mainline. :)