Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933458AbeAKWAG (ORCPT + 1 other); Thu, 11 Jan 2018 17:00:06 -0500 Received: from mail-io0-f195.google.com ([209.85.223.195]:34832 "EHLO mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932790AbeAKWAD (ORCPT ); Thu, 11 Jan 2018 17:00:03 -0500 X-Google-Smtp-Source: ACJfBosgom6ewRNi1ZUXsG1t9JA2iA/qXUb0kJR8bJBF5U/PgOlsIJxGEK2Eq1xWG/ENL6F1IjSxWeoVAgo0temStJg= MIME-Version: 1.0 In-Reply-To: <20180110225626.110330-1-yuzhao@google.com> References: <20180110224741.83751-1-yuzhao@google.com> <20180110225626.110330-1-yuzhao@google.com> From: Dan Streetman Date: Thu, 11 Jan 2018 16:59:21 -0500 X-Google-Sender-Auth: f5rZDcUGA1gYlxhUftYM3OgBQnM Message-ID: Subject: Re: [PATCH v3] zswap: only save zswap header when necessary 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 Wed, Jan 10, 2018 at 5:56 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_evictable() to detect if zpool is potentially evictable, > 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 | 25 +++++++++++++++++++++++-- > mm/zsmalloc.c | 7 ------- > mm/zswap.c | 20 ++++++++++---------- > 4 files changed, 35 insertions(+), 19 deletions(-) > > diff --git a/include/linux/zpool.h b/include/linux/zpool.h > index 004ba807df96..7238865e75b0 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_evictable(struct zpool *pool); > + > #endif > diff --git a/mm/zpool.c b/mm/zpool.c > index fd3ff719c32c..e1e7aa6d1d06 100644 > --- a/mm/zpool.c > +++ b/mm/zpool.c > @@ -21,6 +21,7 @@ struct zpool { > struct zpool_driver *driver; > void *pool; > const struct zpool_ops *ops; > + bool evictable; > > struct list_head list; > }; > @@ -142,7 +143,7 @@ EXPORT_SYMBOL(zpool_has_pool); > * > * This creates a new zpool of the specified type. The gfp flags will be > * used when allocating memory, if the implementation supports it. If the > - * ops param is NULL, then the created zpool will not be shrinkable. > + * ops param is NULL, then the created zpool will not be evictable. > * > * Implementations must guarantee this to be thread-safe. > * > @@ -180,6 +181,7 @@ struct zpool *zpool_create_pool(const char *type, const char *name, gfp_t gfp, > zpool->driver = driver; > zpool->pool = driver->create(name, gfp, ops, zpool); > zpool->ops = ops; > + zpool->evictable = driver->shrink && ops && ops->evict; Since the ops->evict comes from zswap (and is never omitted), if we do restore zs_zpool_shrink() in the future to call the zsmalloc shrinker (and not do eviction), we'll have to add a driver->evictable bool to check here as well. But for now this is good, the zpools with driver->shrink do eviction, zsmalloc doesn't do eviction and won't have driver->shrink. Acked-by: Dan Streetman Thanks! > > if (!zpool->pool) { > pr_err("couldn't create %s pool\n", type); > @@ -296,7 +298,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->driver->shrink ? > + zpool->driver->shrink(zpool->pool, pages, reclaimed) : -EINVAL; > } > > /** > @@ -355,6 +358,24 @@ u64 zpool_get_total_size(struct zpool *zpool) > return zpool->driver->total_size(zpool->pool); > } > > +/** > + * zpool_evictable() - Test if zpool is potentially evictable > + * @pool The zpool to test > + * > + * Zpool is only potentially evictable when it's created with struct > + * zpool_ops.evict and its driver implements struct zpool_driver.shrink. > + * > + * However, it doesn't necessarily mean driver will use zpool_ops.evict > + * in its implementation of zpool_driver.shrink. It could do internal > + * defragmentation instead. > + * > + * Returns: true if potentially evictable; false otherwise. > + */ > +bool zpool_evictable(struct zpool *zpool) > +{ > + return zpool->evictable; > +} > + > 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..52a9ab519ab2 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_evictable(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_evictable(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.rc1.238.g530d649a79-goog >