Skip to contentSkip to conversation

zswap: only save zswap header if zpool is shrinkable

PATCH In review Yu Zhao 3 participants 14 messages Jan 8, 2018 – Jan 11, 2018

Subsystems (3)
lore
Share & tools
lore

Yu Zhao

author
#0 lore raw

PATCH 1/1 zswap: only save zswap header if zpool is shrinkable

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.

---
 include/linux/zpool.h |  2 ++
 mm/zpool.c            | 17 ++++++++++++++++-
 mm/zsmalloc.c         |  7 -------
 mm/zswap.c            | 20 ++++++++++----------
 4 files changed, 28 insertions(+), 18 deletions(-)
include/linux/zpool.h +2 -0
index 004ba807df96..3f0ac2ab74aa 100644
@@ -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
mm/zpool.c +16 -1
index fd3ff719c32c..839d4234c540 100644
@@ -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;
+}
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Dan Streetman <[email protected]>");
 MODULE_DESCRIPTION("Common API for compressed memory storage");
mm/zsmalloc.c +0 -7
index 683c0651098c..9cc741bcdb32 100644
@@ -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,
mm/zswap.c +10 -10
index d39581a076c3..15d2ea29a6fa 100644
@@ -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);
Signature · 7 lines
2.16.0.rc0.223.g4a4ac83678-goog

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Reviewed-by on v1 1/1
#1 lore raw
Quoted 7 lines · Yu Zhao

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.

at a glance, looks good to me

	-ss

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#2 lore raw
In reply to Yu Zhao

On (01/08/18 14:51), Yu Zhao wrote:
[..]

Quoted 16 lines

int zpool_shrink(struct zpool *zpool, unsigned int pages,

mm/zpool.c +5 -1
 			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

mm/zpool.c u64 zpool_get_total_size(struct zpool *zpool) · line 371

Quoted 10 lines
mm/zpool.c +10 -0
+ *
+ * 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;
+}

just a side note,
it might be a bit confusing and maybe there is a better name for it. zsmalloc is shrinkable (we register a shrinker callback), but not in the way zpool defines it.

	-ss

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#3 lore raw
Quoted 25 lines · Sergey Senozhatsky

On (01/08/18 14:51), Yu Zhao wrote:
[..]

Quoted 23 lines

int zpool_shrink(struct zpool *zpool, unsigned int pages,

mm/zpool.c +12 -1
 			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)
Quoted 8 lines

mm/zpool.c u64 zpool_get_total_size(struct zpool *zpool) · line 371

Quoted 3 lines
mm/zpool.c +3 -0
+{
+	return zpool->ops && zpool->ops->evict && zpool->driver->shrink;
+}

just a side note,
it might be a bit confusing and maybe there is a better name for it. zsmalloc is shrinkable (we register a shrinker callback), but not in the way zpool defines it.

Thanks. Do zpool_evictable() and zpool->driver->evict make more sense?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#4 lore raw
In reply to Yu Zhao
Quoted 45 lines · Yu Zhao

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.

---
 include/linux/zpool.h |  2 ++
 mm/zpool.c            | 17 ++++++++++++++++-
 mm/zsmalloc.c         |  7 -------
 mm/zswap.c            | 20 ++++++++++----------
 4 files changed, 28 insertions(+), 18 deletions(-)
include/linux/zpool.h +2 -0
index 004ba807df96..3f0ac2ab74aa 100644
@@ -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
mm/zpool.c +4 -1
index fd3ff719c32c..839d4234c540 100644
@@ -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

mm/zpool.c u64 zpool_get_total_size(struct zpool *zpool) · line 370

Quoted 10 lines
mm/zpool.c +10 -0
+ * @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.

Quoted 86 lines
mm/zpool.c +2 -0
+}
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Dan Streetman <[email protected]>");
 MODULE_DESCRIPTION("Common API for compressed memory storage");
mm/zsmalloc.c +0 -7
index 683c0651098c..9cc741bcdb32 100644
@@ -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,
mm/zswap.c +10 -11
index d39581a076c3..15d2ea29a6fa 100644
@@ -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);
--
Quoted 2 lines

2.16.0.rc0.223.g4a4ac83678-goog

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#5 lore raw
Quoted 54 lines · Dan Streetman
Quoted 53 lines · Yu Zhao

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.

---
 include/linux/zpool.h |  2 ++
 mm/zpool.c            | 17 ++++++++++++++++-
 mm/zsmalloc.c         |  7 -------
 mm/zswap.c            | 20 ++++++++++----------
 4 files changed, 28 insertions(+), 18 deletions(-)
include/linux/zpool.h +2 -0
index 004ba807df96..3f0ac2ab74aa 100644
@@ -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
mm/zpool.c +12 -1
index fd3ff719c32c..839d4234c540 100644
@@ -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)
Quoted 8 lines

mm/zpool.c u64 zpool_get_total_size(struct zpool *zpool) · line 370

Quoted 2 lines
mm/zpool.c +2 -0
+{
+       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.

Ack.

Quoted 8 lines

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.

I agree we keep zpool_shrink(). It could either shrink pool if driver supports slab shrinker by providing zpool->driver->shrink or evict pages from pool if driver supports zpool->driver->evict (which in turn calls ops->evict provided by zswap) or both.

We can't use a single zpool->driver->callback to achieve both because there will be no way for zswap to know if driver uses ops->evict thus no way to determine if zswap_header is needed.

So for now, I think it'd be better if we deleted zpool->driver->shrink from zsmalloc and renamed it to zpool->driver->evict in zbud. Later if we decide zpool_shrink should also call zsmalloc slab shrinker, we add a new callback.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#6 lore raw
Quoted 80 lines · Yu Zhao
Quoted 54 lines · Dan Streetman
Quoted 53 lines · Yu Zhao

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.

---
 include/linux/zpool.h |  2 ++
 mm/zpool.c            | 17 ++++++++++++++++-
 mm/zsmalloc.c         |  7 -------
 mm/zswap.c            | 20 ++++++++++----------
 4 files changed, 28 insertions(+), 18 deletions(-)
include/linux/zpool.h +2 -0
index 004ba807df96..3f0ac2ab74aa 100644
@@ -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
mm/zpool.c +12 -1
index fd3ff719c32c..839d4234c540 100644
@@ -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)
Quoted 8 lines

mm/zpool.c u64 zpool_get_total_size(struct zpool *zpool) · line 370

Quoted 2 lines
mm/zpool.c +2 -0
+{
+       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.

Ack.

Quoted 8 lines

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.

I agree we keep zpool_shrink(). It could either shrink pool if driver supports slab shrinker by providing zpool->driver->shrink or evict pages from pool if driver supports zpool->driver->evict (which in turn calls ops->evict provided by zswap) or both.

Quoted 8 lines

We can't use a single zpool->driver->callback to achieve both because there will be no way for zswap to know if driver uses ops->evict thus no way to determine if zswap_header is needed.

So for now, I think it'd be better if we deleted zpool->driver->shrink from zsmalloc and renamed it to zpool->driver->evict in zbud. Later if we decide zpool_shrink should also call zsmalloc slab shrinker, we add a new callback.

Well, I think shrink vs evict an implementation detail, isn't it? That is, from zswap's perspective, there should be:

zpool_evictable()
if true, zswap needs to include the header on each compressed page, because the zpool may callback zpool->ops->evict() which calls zswap_writeback_entry() which expects the entry to start with a zswap header.
if false, zswap doesn't need to include the header, because the zpool will never, ever call zpool->ops->evict

zpool_shrink()
this will try to shrink the zpool, using whatever zpool-implementation-specific shrinking method. If zpool_evictable() is true for this zpool, then zpool_shrink() *might* callback to zpool->ops->evict(), although it doesn't have to if it can shrink without evictions. If zpool_evictable() is false, then zpool_shrink() will never callback to zpool->ops->evict().

There is really no need for zswap to call different functions based on whether the pool is evictable or not...is there?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#7 lore raw
Quoted 109 lines · Dan Streetman
Quoted 80 lines · Yu Zhao
Quoted 54 lines · Dan Streetman
Quoted 53 lines · Yu Zhao

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.

---
 include/linux/zpool.h |  2 ++
 mm/zpool.c            | 17 ++++++++++++++++-
 mm/zsmalloc.c         |  7 -------
 mm/zswap.c            | 20 ++++++++++----------
 4 files changed, 28 insertions(+), 18 deletions(-)
include/linux/zpool.h +2 -0
index 004ba807df96..3f0ac2ab74aa 100644
@@ -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
mm/zpool.c +12 -1
index fd3ff719c32c..839d4234c540 100644
@@ -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)
Quoted 8 lines

mm/zpool.c u64 zpool_get_total_size(struct zpool *zpool) · line 370

Quoted 2 lines
mm/zpool.c +2 -0
+{
+       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.

Ack.

Quoted 8 lines

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.

I agree we keep zpool_shrink(). It could either shrink pool if driver supports slab shrinker by providing zpool->driver->shrink or evict pages from pool if driver supports zpool->driver->evict (which in turn calls ops->evict provided by zswap) or both.

Quoted 8 lines

We can't use a single zpool->driver->callback to achieve both because there will be no way for zswap to know if driver uses ops->evict thus no way to determine if zswap_header is needed.

So for now, I think it'd be better if we deleted zpool->driver->shrink from zsmalloc and renamed it to zpool->driver->evict in zbud. Later if we decide zpool_shrink should also call zsmalloc slab shrinker, we add a new callback.

Well, I think shrink vs evict an implementation detail, isn't it? That is, from zswap's perspective, there should be:

zpool_evictable()
if true, zswap needs to include the header on each compressed page, because the zpool may callback zpool->ops->evict() which calls zswap_writeback_entry() which expects the entry to start with a zswap header.
if false, zswap doesn't need to include the header, because the zpool will never, ever call zpool->ops->evict

zpool_shrink()
this will try to shrink the zpool, using whatever zpool-implementation-specific shrinking method. If zpool_evictable() is true for this zpool, then zpool_shrink() *might* callback to zpool->ops->evict(), although it doesn't have to if it can shrink without evictions. If zpool_evictable() is false, then zpool_shrink() will never callback to zpool->ops->evict().

Quoted 2 lines

There is really no need for zswap to call different functions based on whether the pool is evictable or not...is there?

Thanks. I'd prefer if driver drew a clear line between its defrag and eviction capabilities. But I have no objection to leaving them as its internal implementation details and keeping current name of zpool->driver->shrink.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#8 lore raw
In reply to Dan Streetman

Hello,

Yu Zhao, Dan, sorry for the delay

On (01/10/18 15:06), Dan Streetman wrote:
[..]

Quoted 11 lines

Well, I think shrink vs evict an implementation detail, isn't it? That is, from zswap's perspective, there should be:

zpool_evictable()
if true, zswap needs to include the header on each compressed page, because the zpool may callback zpool->ops->evict() which calls zswap_writeback_entry() which expects the entry to start with a zswap header.
if false, zswap doesn't need to include the header, because the zpool will never, ever call zpool->ops->evict

Quoted 7 lines

zpool_shrink()
this will try to shrink the zpool, using whatever zpool-implementation-specific shrinking method. If zpool_evictable() is true for this zpool, then zpool_shrink() *might* callback to zpool->ops->evict(), although it doesn't have to if it can shrink without evictions. If zpool_evictable() is false, then zpool_shrink() will never callback to zpool->ops->evict().

ACK on this!

	-ss

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#9 lore raw

PATCH v2 1/1 zswap: only save zswap header when necessary

In reply to Yu Zhao

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.

---
 include/linux/zpool.h |  2 ++
 mm/zpool.c            | 26 ++++++++++++++++++++++++--
 mm/zsmalloc.c         |  7 -------
 mm/zswap.c            | 20 ++++++++++----------
 4 files changed, 36 insertions(+), 19 deletions(-)
include/linux/zpool.h +2 -0
index 004ba807df96..7238865e75b0 100644
@@ -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
mm/zpool.c +24 -2
index fd3ff719c32c..ec63ef32d73d 100644
@@ -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,8 @@ 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 = zpool->driver->shrink &&
+			   zpool->ops && zpool->ops->evict;
 
 	if (!zpool->pool) {
 		pr_err("couldn't create %s pool\n", type);
@@ -296,7 +299,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 +359,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 <[email protected]>");
 MODULE_DESCRIPTION("Common API for compressed memory storage");
mm/zsmalloc.c +0 -7
index 683c0651098c..9cc741bcdb32 100644
@@ -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,
mm/zswap.c +10 -10
index d39581a076c3..52a9ab519ab2 100644
@@ -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);
Signature · 7 lines
2.16.0.rc1.238.g530d649a79-goog

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#10 lore raw

PATCH v3 1/1 zswap: only save zswap header when necessary

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.

---
 include/linux/zpool.h |  2 ++
 mm/zpool.c            | 25 +++++++++++++++++++++++--
 mm/zsmalloc.c         |  7 -------
 mm/zswap.c            | 20 ++++++++++----------
 4 files changed, 35 insertions(+), 19 deletions(-)
include/linux/zpool.h +2 -0
index 004ba807df96..7238865e75b0 100644
@@ -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
mm/zpool.c +23 -2
index fd3ff719c32c..e1e7aa6d1d06 100644
@@ -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;
 
 	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 <[email protected]>");
 MODULE_DESCRIPTION("Common API for compressed memory storage");
mm/zsmalloc.c +0 -7
index 683c0651098c..9cc741bcdb32 100644
@@ -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,
mm/zswap.c +10 -10
index d39581a076c3..52a9ab519ab2 100644
@@ -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);
Signature · 7 lines
2.16.0.rc1.238.g530d649a79-goog

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Reviewed-by on v3 1/1
#11 lore raw

Re: [PATCH v3] zswap: only save zswap header when necessary

On (01/10/18 14:56), Yu Zhao wrote:
[..]

Quoted 7 lines

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.

looks good to me.

FWIW,

	-ss

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Acked-by on v3 1/1
#12 lore raw

Re: [PATCH v3] zswap: only save zswap header when necessary

In reply to Yu Zhao
Quoted 47 lines · Yu Zhao

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.

---
 include/linux/zpool.h |  2 ++
 mm/zpool.c            | 25 +++++++++++++++++++++++--
 mm/zsmalloc.c         |  7 -------
 mm/zswap.c            | 20 ++++++++++----------
 4 files changed, 35 insertions(+), 19 deletions(-)
include/linux/zpool.h +2 -0
index 004ba807df96..7238865e75b0 100644
@@ -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
mm/zpool.c +2 -1
index fd3ff719c32c..e1e7aa6d1d06 100644
@@ -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.
  *

mm/zpool.c struct zpool *zpool_create_pool(const char *type, const char *name, gfp_t gfp, · line 184

Quoted 4 lines
mm/zpool.c +1 -0
@@ -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.

Thanks!

Quoted 119 lines
mm/zpool.c +20 -1
        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 <[email protected]>");
 MODULE_DESCRIPTION("Common API for compressed memory storage");
mm/zsmalloc.c +0 -7
index 683c0651098c..9cc741bcdb32 100644
@@ -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,
mm/zswap.c +10 -11
index d39581a076c3..52a9ab519ab2 100644
@@ -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);
--
Quoted 2 lines

2.16.0.rc1.238.g530d649a79-goog

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#13 lore raw

Re: [PATCH v2] zswap: only save zswap header when necessary

In reply to Yu Zhao
Quoted 47 lines · Yu Zhao

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.

---
 include/linux/zpool.h |  2 ++
 mm/zpool.c            | 26 ++++++++++++++++++++++++--
 mm/zsmalloc.c         |  7 -------
 mm/zswap.c            | 20 ++++++++++----------
 4 files changed, 36 insertions(+), 19 deletions(-)
include/linux/zpool.h +2 -0
index 004ba807df96..7238865e75b0 100644
@@ -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
mm/zpool.c +2 -1
index fd3ff719c32c..ec63ef32d73d 100644
@@ -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.
  *

mm/zpool.c struct zpool *zpool_create_pool(const char *type, const char *name, gfp_t gfp, · line 185

Quoted 5 lines
mm/zpool.c +2 -0
@@ -180,6 +181,8 @@ 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 = zpool->driver->shrink &&
+			   zpool->ops && zpool->ops->evict;

The zpool->" prefix is a result out copy & paste. Fixed in the next version. Sorry for the spam.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>