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. :)