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

[Dan Streetman]

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;
>  

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

[Dan Streetman]

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 

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

[Sergey Senozhatsky]

On (01/10/18 14:56), 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 

looks good to me.

FWIW,
Reviewed-by: Sergey Senozhatsky 

-ss

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

[Sergey Senozhatsky]

On (01/10/18 14:56), 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 

looks good to me.

FWIW,
Reviewed-by: Sergey Senozhatsky 

-ss

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

[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.

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;
 
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 = 

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

[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.

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;
 
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,