Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760316AbeAIS0G (ORCPT + 1 other); Tue, 9 Jan 2018 13:26:06 -0500 Received: from mail-io0-f195.google.com ([209.85.223.195]:33626 "EHLO mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758201AbeAIS0A (ORCPT ); Tue, 9 Jan 2018 13:26:00 -0500 X-Google-Smtp-Source: ACJfBoufpxdYZ6HjzzFjgK389J6KoBo5Iaf0JJ4MuSQLRVrQmKvza7Qasrz833uDvQNuB0E5VuycUU3rzG+5i5ysLjY= MIME-Version: 1.0 In-Reply-To: <20180108225101.15790-1-yuzhao@google.com> References: <20180108225101.15790-1-yuzhao@google.com> From: Dan Streetman Date: Tue, 9 Jan 2018 13:25:18 -0500 X-Google-Sender-Auth: r0oYPygr_r8rY3NatGjtRkx4I14 Message-ID: Subject: Re: [PATCH] zswap: only save zswap header if zpool is shrinkable To: Yu Zhao Cc: Seth Jennings , Sergey Senozhatsky , Minchan Kim , Nitin Gupta , Andrew Morton , Linux-MM , linux-kernel Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Mon, Jan 8, 2018 at 5:51 PM, Yu Zhao wrote: > We waste sizeof(swp_entry_t) for zswap header when using zsmalloc > as zpool driver because zsmalloc doesn't support eviction. > > Add zpool_shrinkable() to detect if zpool is shrinkable, and use > it in zswap to avoid waste memory for zswap header. > > Signed-off-by: Yu Zhao > --- > include/linux/zpool.h | 2 ++ > mm/zpool.c | 17 ++++++++++++++++- > mm/zsmalloc.c | 7 ------- > mm/zswap.c | 20 ++++++++++---------- > 4 files changed, 28 insertions(+), 18 deletions(-) > > diff --git a/include/linux/zpool.h b/include/linux/zpool.h > index 004ba807df96..3f0ac2ab74aa 100644 > --- a/include/linux/zpool.h > +++ b/include/linux/zpool.h > @@ -108,4 +108,6 @@ void zpool_register_driver(struct zpool_driver *driver); > > int zpool_unregister_driver(struct zpool_driver *driver); > > +bool zpool_shrinkable(struct zpool *pool); > + > #endif > diff --git a/mm/zpool.c b/mm/zpool.c > index fd3ff719c32c..839d4234c540 100644 > --- a/mm/zpool.c > +++ b/mm/zpool.c > @@ -296,7 +296,8 @@ void zpool_free(struct zpool *zpool, unsigned long handle) > int zpool_shrink(struct zpool *zpool, unsigned int pages, > unsigned int *reclaimed) > { > - return zpool->driver->shrink(zpool->pool, pages, reclaimed); > + return zpool_shrinkable(zpool) ? > + zpool->driver->shrink(zpool->pool, pages, reclaimed) : -EINVAL; > } > > /** > @@ -355,6 +356,20 @@ u64 zpool_get_total_size(struct zpool *zpool) > return zpool->driver->total_size(zpool->pool); > } > > +/** > + * zpool_shrinkable() - Test if zpool is shrinkable > + * @pool The zpool to test > + * > + * Zpool is only shrinkable when it's created with struct > + * zpool_ops.evict and its driver implements struct zpool_driver.shrink. > + * > + * Returns: true if shrinkable; false otherwise. > + */ > +bool zpool_shrinkable(struct zpool *zpool) > +{ > + return zpool->ops && zpool->ops->evict && zpool->driver->shrink; as these things won't ever change for the life of the zpool, it would probably be better to just check them at zpool creation time and set a single new zpool param, like 'zpool->shrinkable'. since this function will be called for every page that's swapped in or out, that may save a bit of time. also re: calling it 'shrinkable' or 'evictable', the real thing zswap is interested in is if it needs to include the header info that zswap_writeback_entry (i.e. ops->evict) later needs, so yeah it does make more sense to call it zpool_evictable() and zpool->evictable. However, I think the function should still be zpool_shrink() and zpool->driver->shrink(), because it should be possible for zs_pool_shrink() to call the normal zsmalloc shrinker, instead of doing the zswap-style eviction, even if it doesn't do that currently. > +} > + > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Dan Streetman "); > MODULE_DESCRIPTION("Common API for compressed memory storage"); > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c > index 683c0651098c..9cc741bcdb32 100644 > --- a/mm/zsmalloc.c > +++ b/mm/zsmalloc.c > @@ -407,12 +407,6 @@ static void zs_zpool_free(void *pool, unsigned long handle) > zs_free(pool, handle); > } > > -static int zs_zpool_shrink(void *pool, unsigned int pages, > - unsigned int *reclaimed) > -{ > - return -EINVAL; > -} > - > static void *zs_zpool_map(void *pool, unsigned long handle, > enum zpool_mapmode mm) > { > @@ -450,7 +444,6 @@ static struct zpool_driver zs_zpool_driver = { > .destroy = zs_zpool_destroy, > .malloc = zs_zpool_malloc, > .free = zs_zpool_free, > - .shrink = zs_zpool_shrink, > .map = zs_zpool_map, > .unmap = zs_zpool_unmap, > .total_size = zs_zpool_total_size, > diff --git a/mm/zswap.c b/mm/zswap.c > index d39581a076c3..15d2ea29a6fa 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -964,11 +964,11 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset, > struct zswap_entry *entry, *dupentry; > struct crypto_comp *tfm; > int ret; > - unsigned int dlen = PAGE_SIZE, len; > + unsigned int hlen, dlen = PAGE_SIZE; > unsigned long handle; > char *buf; > u8 *src, *dst; > - struct zswap_header *zhdr; > + struct zswap_header zhdr = { .swpentry = swp_entry(type, offset) }; > > if (!zswap_enabled || !tree) { > ret = -ENODEV; > @@ -1013,8 +1013,8 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset, > } > > /* store */ > - len = dlen + sizeof(struct zswap_header); > - ret = zpool_malloc(entry->pool->zpool, len, > + hlen = zpool_shrinkable(entry->pool->zpool) ? sizeof(zhdr) : 0; > + ret = zpool_malloc(entry->pool->zpool, hlen + dlen, > __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM, > &handle); > if (ret == -ENOSPC) { > @@ -1025,10 +1025,9 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset, > zswap_reject_alloc_fail++; > goto put_dstmem; > } > - zhdr = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_RW); > - zhdr->swpentry = swp_entry(type, offset); > - buf = (u8 *)(zhdr + 1); > - memcpy(buf, dst, dlen); > + buf = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_RW); > + memcpy(buf, &zhdr, hlen); > + memcpy(buf + hlen, dst, dlen); > zpool_unmap_handle(entry->pool->zpool, handle); > put_cpu_var(zswap_dstmem); > > @@ -1091,8 +1090,9 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, > > /* decompress */ > dlen = PAGE_SIZE; > - src = (u8 *)zpool_map_handle(entry->pool->zpool, entry->handle, > - ZPOOL_MM_RO) + sizeof(struct zswap_header); > + src = zpool_map_handle(entry->pool->zpool, entry->handle, ZPOOL_MM_RO); > + if (zpool_shrinkable(entry->pool->zpool)) > + src += sizeof(struct zswap_header); > dst = kmap_atomic(page); > tfm = *get_cpu_ptr(entry->pool->tfm); > ret = crypto_comp_decompress(tfm, src, entry->length, dst, &dlen); > -- > 2.16.0.rc0.223.g4a4ac83678-goog >