Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1417175 > unrolled thread
| Started by | Geliang Tang <geliangtang@gmail.com> |
|---|---|
| First post | 2016-06-08 11:40 +0200 |
| Last post | 2016-06-09 03:50 +0200 |
| Articles | 4 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH] zram: add zpool support Geliang Tang <geliangtang@gmail.com> - 2016-06-08 11:40 +0200
Re: [PATCH] zram: add zpool support Dan Streetman <ddstreet@ieee.org> - 2016-06-08 17:00 +0200
Re: [PATCH] zram: add zpool support Minchan Kim <minchan@kernel.org> - 2016-06-09 03:40 +0200
Re: [PATCH] zram: add zpool support Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-06-09 03:50 +0200
| From | Geliang Tang <geliangtang@gmail.com> |
|---|---|
| Date | 2016-06-08 11:40 +0200 |
| Subject | [PATCH] zram: add zpool support |
| Message-ID | <rHI7U-6Pu-31@gated-at.bofh.it> |
This patch adds zpool support for zram, it will allow us to use both
the zpool api and directly zsmalloc api in zram.
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
drivers/block/zram/zram_drv.c | 97 +++++++++++++++++++++++++++++++++++++++++++
drivers/block/zram/zram_drv.h | 5 +++
2 files changed, 102 insertions(+)
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 9e2a83c..1f90bd0 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -43,6 +43,11 @@ static const char *default_compressor = "lzo";
/* Module params (documentation at end) */
static unsigned int num_devices = 1;
+#ifdef CONFIG_ZPOOL
+/* Compressed storage zpool to use */
+#define ZRAM_ZPOOL_DEFAULT "zsmalloc"
+#endif
+
static inline void deprecated_attr_warn(const char *name)
{
pr_warn_once("%d (%s) Attribute %s (and others) will be removed. %s\n",
@@ -228,7 +233,11 @@ static ssize_t mem_used_total_show(struct device *dev,
down_read(&zram->init_lock);
if (init_done(zram)) {
struct zram_meta *meta = zram->meta;
+#ifdef CONFIG_ZPOOL
+ val = zpool_get_total_size(meta->mem_pool) >> PAGE_SHIFT;
+#else
val = zs_get_total_pages(meta->mem_pool);
+#endif
}
up_read(&zram->init_lock);
@@ -296,8 +305,14 @@ static ssize_t mem_used_max_store(struct device *dev,
down_read(&zram->init_lock);
if (init_done(zram)) {
struct zram_meta *meta = zram->meta;
+#ifdef CONFIG_ZPOOL
+ atomic_long_set(&zram->stats.max_used_pages,
+ zpool_get_total_size(meta->mem_pool)
+ >> PAGE_SHIFT);
+#else
atomic_long_set(&zram->stats.max_used_pages,
zs_get_total_pages(meta->mem_pool));
+#endif
}
up_read(&zram->init_lock);
@@ -366,6 +381,18 @@ static ssize_t comp_algorithm_store(struct device *dev,
return len;
}
+#ifdef CONFIG_ZPOOL
+static void zpool_compact(void *pool)
+{
+ zs_compact(pool);
+}
+
+static void zpool_stats(void *pool, struct zs_pool_stats *stats)
+{
+ zs_pool_stats(pool, stats);
+}
+#endif
+
static ssize_t compact_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t len)
{
@@ -379,7 +406,11 @@ static ssize_t compact_store(struct device *dev,
}
meta = zram->meta;
+#ifdef CONFIG_ZPOOL
+ zpool_compact(meta->mem_pool);
+#else
zs_compact(meta->mem_pool);
+#endif
up_read(&zram->init_lock);
return len;
@@ -416,8 +447,14 @@ static ssize_t mm_stat_show(struct device *dev,
down_read(&zram->init_lock);
if (init_done(zram)) {
+#ifdef CONFIG_ZPOOL
+ mem_used = zpool_get_total_size(zram->meta->mem_pool)
+ >> PAGE_SHIFT;
+ zpool_stats(zram->meta->mem_pool, &pool_stats);
+#else
mem_used = zs_get_total_pages(zram->meta->mem_pool);
zs_pool_stats(zram->meta->mem_pool, &pool_stats);
+#endif
}
orig_size = atomic64_read(&zram->stats.pages_stored);
@@ -490,10 +527,18 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
if (!handle)
continue;
+#ifdef CONFIG_ZPOOL
+ zpool_free(meta->mem_pool, handle);
+#else
zs_free(meta->mem_pool, handle);
+#endif
}
+#ifdef CONFIG_ZPOOL
+ zpool_destroy_pool(meta->mem_pool);
+#else
zs_destroy_pool(meta->mem_pool);
+#endif
vfree(meta->table);
kfree(meta);
}
@@ -513,7 +558,17 @@ static struct zram_meta *zram_meta_alloc(char *pool_name, u64 disksize)
goto out_error;
}
+#ifdef CONFIG_ZPOOL
+ if (!zpool_has_pool(ZRAM_ZPOOL_DEFAULT)) {
+ pr_err("zpool %s not available\n", ZRAM_ZPOOL_DEFAULT);
+ goto out_error;
+ }
+
+ meta->mem_pool = zpool_create_pool(ZRAM_ZPOOL_DEFAULT,
+ pool_name, 0, NULL);
+#else
meta->mem_pool = zs_create_pool(pool_name);
+#endif
if (!meta->mem_pool) {
pr_err("Error creating memory pool\n");
goto out_error;
@@ -549,7 +604,11 @@ static void zram_free_page(struct zram *zram, size_t index)
return;
}
+#ifdef CONFIG_ZPOOL
+ zpool_free(meta->mem_pool, handle);
+#else
zs_free(meta->mem_pool, handle);
+#endif
atomic64_sub(zram_get_obj_size(meta, index),
&zram->stats.compr_data_size);
@@ -577,7 +636,11 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
return 0;
}
+#ifdef CONFIG_ZPOOL
+ cmem = zpool_map_handle(meta->mem_pool, handle, ZPOOL_MM_RO);
+#else
cmem = zs_map_object(meta->mem_pool, handle, ZS_MM_RO);
+#endif
if (size == PAGE_SIZE) {
copy_page(mem, cmem);
} else {
@@ -586,7 +649,11 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
ret = zcomp_decompress(zstrm, cmem, size, mem);
zcomp_stream_put(zram->comp);
}
+#ifdef CONFIG_ZPOOL
+ zpool_unmap_handle(meta->mem_pool, handle);
+#else
zs_unmap_object(meta->mem_pool, handle);
+#endif
bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
/* Should NEVER happen. Return bio error if it does. */
@@ -735,20 +802,34 @@ compress_again:
* from the slow path and handle has already been allocated.
*/
if (!handle)
+#ifdef CONFIG_ZPOOL
+ ret = zpool_malloc(meta->mem_pool, clen,
+ __GFP_KSWAPD_RECLAIM |
+ __GFP_NOWARN |
+ __GFP_HIGHMEM |
+ __GFP_MOVABLE, &handle);
+#else
handle = zs_malloc(meta->mem_pool, clen,
__GFP_KSWAPD_RECLAIM |
__GFP_NOWARN |
__GFP_HIGHMEM |
__GFP_MOVABLE);
+#endif
if (!handle) {
zcomp_stream_put(zram->comp);
zstrm = NULL;
atomic64_inc(&zram->stats.writestall);
+#ifdef CONFIG_ZPOOL
+ ret = zpool_malloc(meta->mem_pool, clen,
+ GFP_NOIO | __GFP_HIGHMEM |
+ __GFP_MOVABLE, &handle);
+#else
handle = zs_malloc(meta->mem_pool, clen,
GFP_NOIO | __GFP_HIGHMEM |
__GFP_MOVABLE);
+#endif
if (handle)
goto compress_again;
@@ -758,16 +839,28 @@ compress_again:
goto out;
}
+#ifdef CONFIG_ZPOOL
+ alloced_pages = zpool_get_total_size(meta->mem_pool) >> PAGE_SHIFT;
+#else
alloced_pages = zs_get_total_pages(meta->mem_pool);
+#endif
update_used_max(zram, alloced_pages);
if (zram->limit_pages && alloced_pages > zram->limit_pages) {
+#ifdef CONFIG_ZPOOL
+ zpool_free(meta->mem_pool, handle);
+#else
zs_free(meta->mem_pool, handle);
+#endif
ret = -ENOMEM;
goto out;
}
+#ifdef CONFIG_ZPOOL
+ cmem = zpool_map_handle(meta->mem_pool, handle, ZPOOL_MM_WO);
+#else
cmem = zs_map_object(meta->mem_pool, handle, ZS_MM_WO);
+#endif
if ((clen == PAGE_SIZE) && !is_partial_io(bvec)) {
src = kmap_atomic(page);
@@ -779,7 +872,11 @@ compress_again:
zcomp_stream_put(zram->comp);
zstrm = NULL;
+#ifdef CONFIG_ZPOOL
+ zpool_unmap_handle(meta->mem_pool, handle);
+#else
zs_unmap_object(meta->mem_pool, handle);
+#endif
/*
* Free memory associated with this sector
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 74fcf10..68f1222 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -17,6 +17,7 @@
#include <linux/rwsem.h>
#include <linux/zsmalloc.h>
+#include <linux/zpool.h>
#include <linux/crypto.h>
#include "zcomp.h"
@@ -91,7 +92,11 @@ struct zram_stats {
struct zram_meta {
struct zram_table_entry *table;
+#ifdef CONFIG_ZPOOL
+ struct zpool *mem_pool;
+#else
struct zs_pool *mem_pool;
+#endif
};
struct zram {
--
1.9.1
[toc] | [next] | [standalone]
| From | Dan Streetman <ddstreet@ieee.org> |
|---|---|
| Date | 2016-06-08 17:00 +0200 |
| Message-ID | <rHN7z-1ts-27@gated-at.bofh.it> |
| In reply to | #1417175 |
On Wed, Jun 8, 2016 at 5:39 AM, Geliang Tang <geliangtang@gmail.com> wrote:
> This patch adds zpool support for zram, it will allow us to use both
> the zpool api and directly zsmalloc api in zram.
besides the problems below, this was discussed a while ago and I
believe Minchan is still against it, as nobody has so far shown what
the benefit to zram would be; zram doesn't need the predictability, or
evictability, of zbud or z3fold.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> drivers/block/zram/zram_drv.c | 97 +++++++++++++++++++++++++++++++++++++++++++
> drivers/block/zram/zram_drv.h | 5 +++
> 2 files changed, 102 insertions(+)
>
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 9e2a83c..1f90bd0 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -43,6 +43,11 @@ static const char *default_compressor = "lzo";
> /* Module params (documentation at end) */
> static unsigned int num_devices = 1;
>
> +#ifdef CONFIG_ZPOOL
> +/* Compressed storage zpool to use */
> +#define ZRAM_ZPOOL_DEFAULT "zsmalloc"
> +#endif
It doesn't make sense for zram to conditionally use zpool; either it
uses it and thus has 'select ZPOOL' in its Kconfig entry, or it
doesn't use it at all.
> +
> static inline void deprecated_attr_warn(const char *name)
> {
> pr_warn_once("%d (%s) Attribute %s (and others) will be removed. %s\n",
> @@ -228,7 +233,11 @@ static ssize_t mem_used_total_show(struct device *dev,
> down_read(&zram->init_lock);
> if (init_done(zram)) {
> struct zram_meta *meta = zram->meta;
> +#ifdef CONFIG_ZPOOL
> + val = zpool_get_total_size(meta->mem_pool) >> PAGE_SHIFT;
> +#else
> val = zs_get_total_pages(meta->mem_pool);
> +#endif
> }
> up_read(&zram->init_lock);
>
> @@ -296,8 +305,14 @@ static ssize_t mem_used_max_store(struct device *dev,
> down_read(&zram->init_lock);
> if (init_done(zram)) {
> struct zram_meta *meta = zram->meta;
> +#ifdef CONFIG_ZPOOL
> + atomic_long_set(&zram->stats.max_used_pages,
> + zpool_get_total_size(meta->mem_pool)
> + >> PAGE_SHIFT);
> +#else
> atomic_long_set(&zram->stats.max_used_pages,
> zs_get_total_pages(meta->mem_pool));
> +#endif
> }
> up_read(&zram->init_lock);
>
> @@ -366,6 +381,18 @@ static ssize_t comp_algorithm_store(struct device *dev,
> return len;
> }
>
> +#ifdef CONFIG_ZPOOL
> +static void zpool_compact(void *pool)
> +{
> + zs_compact(pool);
> +}
> +
> +static void zpool_stats(void *pool, struct zs_pool_stats *stats)
> +{
> + zs_pool_stats(pool, stats);
> +}
> +#endif
first, no. this obviously makes using zpool in zram completely pointless.
second, did you test this? the pool you're passing is the zpool, not
the zs_pool. quite bad things will happen when this code runs. There
is no way to get the zs_pool from the zpool object (that's the point
of abstraction, of course).
The fact zpool doesn't have these apis (currently) is one of the
reasons against changing zram to use zpool.
> +
> static ssize_t compact_store(struct device *dev,
> struct device_attribute *attr, const char *buf, size_t len)
> {
> @@ -379,7 +406,11 @@ static ssize_t compact_store(struct device *dev,
> }
>
> meta = zram->meta;
> +#ifdef CONFIG_ZPOOL
> + zpool_compact(meta->mem_pool);
> +#else
> zs_compact(meta->mem_pool);
> +#endif
> up_read(&zram->init_lock);
>
> return len;
> @@ -416,8 +447,14 @@ static ssize_t mm_stat_show(struct device *dev,
>
> down_read(&zram->init_lock);
> if (init_done(zram)) {
> +#ifdef CONFIG_ZPOOL
> + mem_used = zpool_get_total_size(zram->meta->mem_pool)
> + >> PAGE_SHIFT;
> + zpool_stats(zram->meta->mem_pool, &pool_stats);
> +#else
> mem_used = zs_get_total_pages(zram->meta->mem_pool);
> zs_pool_stats(zram->meta->mem_pool, &pool_stats);
> +#endif
> }
>
> orig_size = atomic64_read(&zram->stats.pages_stored);
> @@ -490,10 +527,18 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
> if (!handle)
> continue;
>
> +#ifdef CONFIG_ZPOOL
> + zpool_free(meta->mem_pool, handle);
> +#else
> zs_free(meta->mem_pool, handle);
> +#endif
> }
>
> +#ifdef CONFIG_ZPOOL
> + zpool_destroy_pool(meta->mem_pool);
> +#else
> zs_destroy_pool(meta->mem_pool);
> +#endif
> vfree(meta->table);
> kfree(meta);
> }
> @@ -513,7 +558,17 @@ static struct zram_meta *zram_meta_alloc(char *pool_name, u64 disksize)
> goto out_error;
> }
>
> +#ifdef CONFIG_ZPOOL
> + if (!zpool_has_pool(ZRAM_ZPOOL_DEFAULT)) {
> + pr_err("zpool %s not available\n", ZRAM_ZPOOL_DEFAULT);
> + goto out_error;
> + }
> +
> + meta->mem_pool = zpool_create_pool(ZRAM_ZPOOL_DEFAULT,
> + pool_name, 0, NULL);
> +#else
> meta->mem_pool = zs_create_pool(pool_name);
> +#endif
> if (!meta->mem_pool) {
> pr_err("Error creating memory pool\n");
> goto out_error;
> @@ -549,7 +604,11 @@ static void zram_free_page(struct zram *zram, size_t index)
> return;
> }
>
> +#ifdef CONFIG_ZPOOL
> + zpool_free(meta->mem_pool, handle);
> +#else
> zs_free(meta->mem_pool, handle);
> +#endif
>
> atomic64_sub(zram_get_obj_size(meta, index),
> &zram->stats.compr_data_size);
> @@ -577,7 +636,11 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
> return 0;
> }
>
> +#ifdef CONFIG_ZPOOL
> + cmem = zpool_map_handle(meta->mem_pool, handle, ZPOOL_MM_RO);
> +#else
> cmem = zs_map_object(meta->mem_pool, handle, ZS_MM_RO);
> +#endif
> if (size == PAGE_SIZE) {
> copy_page(mem, cmem);
> } else {
> @@ -586,7 +649,11 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
> ret = zcomp_decompress(zstrm, cmem, size, mem);
> zcomp_stream_put(zram->comp);
> }
> +#ifdef CONFIG_ZPOOL
> + zpool_unmap_handle(meta->mem_pool, handle);
> +#else
> zs_unmap_object(meta->mem_pool, handle);
> +#endif
> bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
>
> /* Should NEVER happen. Return bio error if it does. */
> @@ -735,20 +802,34 @@ compress_again:
> * from the slow path and handle has already been allocated.
> */
> if (!handle)
> +#ifdef CONFIG_ZPOOL
> + ret = zpool_malloc(meta->mem_pool, clen,
> + __GFP_KSWAPD_RECLAIM |
> + __GFP_NOWARN |
> + __GFP_HIGHMEM |
> + __GFP_MOVABLE, &handle);
> +#else
> handle = zs_malloc(meta->mem_pool, clen,
> __GFP_KSWAPD_RECLAIM |
> __GFP_NOWARN |
> __GFP_HIGHMEM |
> __GFP_MOVABLE);
> +#endif
> if (!handle) {
> zcomp_stream_put(zram->comp);
> zstrm = NULL;
>
> atomic64_inc(&zram->stats.writestall);
>
> +#ifdef CONFIG_ZPOOL
> + ret = zpool_malloc(meta->mem_pool, clen,
> + GFP_NOIO | __GFP_HIGHMEM |
> + __GFP_MOVABLE, &handle);
> +#else
> handle = zs_malloc(meta->mem_pool, clen,
> GFP_NOIO | __GFP_HIGHMEM |
> __GFP_MOVABLE);
> +#endif
> if (handle)
> goto compress_again;
>
> @@ -758,16 +839,28 @@ compress_again:
> goto out;
> }
>
> +#ifdef CONFIG_ZPOOL
> + alloced_pages = zpool_get_total_size(meta->mem_pool) >> PAGE_SHIFT;
> +#else
> alloced_pages = zs_get_total_pages(meta->mem_pool);
> +#endif
> update_used_max(zram, alloced_pages);
>
> if (zram->limit_pages && alloced_pages > zram->limit_pages) {
> +#ifdef CONFIG_ZPOOL
> + zpool_free(meta->mem_pool, handle);
> +#else
> zs_free(meta->mem_pool, handle);
> +#endif
> ret = -ENOMEM;
> goto out;
> }
>
> +#ifdef CONFIG_ZPOOL
> + cmem = zpool_map_handle(meta->mem_pool, handle, ZPOOL_MM_WO);
> +#else
> cmem = zs_map_object(meta->mem_pool, handle, ZS_MM_WO);
> +#endif
>
> if ((clen == PAGE_SIZE) && !is_partial_io(bvec)) {
> src = kmap_atomic(page);
> @@ -779,7 +872,11 @@ compress_again:
>
> zcomp_stream_put(zram->comp);
> zstrm = NULL;
> +#ifdef CONFIG_ZPOOL
> + zpool_unmap_handle(meta->mem_pool, handle);
> +#else
> zs_unmap_object(meta->mem_pool, handle);
> +#endif
>
> /*
> * Free memory associated with this sector
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 74fcf10..68f1222 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -17,6 +17,7 @@
>
> #include <linux/rwsem.h>
> #include <linux/zsmalloc.h>
> +#include <linux/zpool.h>
> #include <linux/crypto.h>
>
> #include "zcomp.h"
> @@ -91,7 +92,11 @@ struct zram_stats {
>
> struct zram_meta {
> struct zram_table_entry *table;
> +#ifdef CONFIG_ZPOOL
> + struct zpool *mem_pool;
> +#else
> struct zs_pool *mem_pool;
> +#endif
> };
>
> struct zram {
> --
> 1.9.1
>
[toc] | [prev] | [next] | [standalone]
| From | Minchan Kim <minchan@kernel.org> |
|---|---|
| Date | 2016-06-09 03:40 +0200 |
| Message-ID | <rHX6V-7Ur-1@gated-at.bofh.it> |
| In reply to | #1417554 |
On Wed, Jun 08, 2016 at 10:51:28AM -0400, Dan Streetman wrote: > On Wed, Jun 8, 2016 at 5:39 AM, Geliang Tang <geliangtang@gmail.com> wrote: > > This patch adds zpool support for zram, it will allow us to use both > > the zpool api and directly zsmalloc api in zram. > > besides the problems below, this was discussed a while ago and I > believe Minchan is still against it, as nobody has so far shown what > the benefit to zram would be; zram doesn't need the predictability, or > evictability, of zbud or z3fold. Right. Geliang, I cannot ack without any *detail* that what's the problem of zram/zsmalloc, why we can't fix it in zsmalloc itself. The zbud and zsmalloc is otally different design to aim different goal determinism vs efficiency so you can choose what you want between zswap and zram rather than mixing the features.
[toc] | [prev] | [next] | [standalone]
| From | Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> |
|---|---|
| Date | 2016-06-09 03:50 +0200 |
| Message-ID | <rHXgB-7Yw-1@gated-at.bofh.it> |
| In reply to | #1418005 |
Hello, On (06/09/16 10:34), Minchan Kim wrote: > On Wed, Jun 08, 2016 at 10:51:28AM -0400, Dan Streetman wrote: > > On Wed, Jun 8, 2016 at 5:39 AM, Geliang Tang <geliangtang@gmail.com> wrote: > > > This patch adds zpool support for zram, it will allow us to use both > > > the zpool api and directly zsmalloc api in zram. > > > > besides the problems below, this was discussed a while ago and I > > believe Minchan is still against it, as nobody has so far shown what > > the benefit to zram would be; zram doesn't need the predictability, or > > evictability, of zbud or z3fold. > > Right. > > Geliang, I cannot ack without any *detail* that what's the problem of > zram/zsmalloc, why we can't fix it in zsmalloc itself. > The zbud and zsmalloc is otally different design to aim different goal > determinism vs efficiency so you can choose what you want between zswap > and zram rather than mixing the features. I'd also probably Cc Vitaly Wool on this (http://marc.info/?l=linux-mm&m=146537877415982&w=2) -ss
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web