[PATCH 1/3] zsmalloc: move pages_allocated to zs_pool

[PATCH 1/3] zsmalloc: move pages_allocated to zs_pool

[Minchan Kim]

Pages_allocated has counted in size_class structure and when user
want to see total_size_bytes, it gathers all of value from each
size_class to report the sum.

It's not bad if user don't see the value often but if user start
to see the value frequently, it would be not a good deal for
performance POV.

Even, this patch moves the variable from size_class to zs_pool
so it reduces memory footprint (from [255 * 8byte] to
[sizeof(atomic_t)]) but it introduce new atomic opearation
but it's not a big deal because atomic operation is called on
slow path of zsmalloc where it allocates/free zspage unit,
not object.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/zsmalloc.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 4e2fc83cb394..2f21ea8921dc 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -199,9 +199,6 @@ struct size_class {
 
 	spinlock_t lock;
 
-	/* stats */
-	u64 pages_allocated;
-
 	struct page *fullness_list[_ZS_NR_FULLNESS_GROUPS];
 };
 
@@ -220,6 +217,7 @@ struct zs_pool {
 	struct size_class size_class[ZS_SIZE_CLASSES];
 
 	gfp_t flags;	/* allocation flags used when growing pool */
+	atomic_t pages_allocated;
 };
 
 /*
@@ -1027,8 +1025,8 @@ unsigned long zs_malloc(struct zs_pool *pool, size_t size)
 			return 0;
 
 		set_zspage_mapping(first_page, class->index, ZS_EMPTY);
+		atomic_add(class->pages_per_zspage, &pool->pages_allocated);
 		spin_lock(&class->lock);
-		class->pages_allocated += class->pages_per_zspage;
 	}
 
 	obj = (unsigned long)first_page->freelist;
@@ -1081,14 +1079,12 @@ void zs_free(struct zs_pool *pool, unsigned long obj)
 
 	first_page->inuse--;
 	fullness = fix_fullness_group(pool, first_page);
-
-	if (fullness == ZS_EMPTY)
-		class->pages_allocated -= class->pages_per_zspage;
-
 	spin_unlock(&class->lock);
 
-	if (fullness == ZS_EMPTY)
+	if (fullness == ZS_EMPTY) {
+		atomic_sub(class->pages_per_zspage, &pool->pages_allocated);
 		free_zspage(first_page);
+	}
 }
 EXPORT_SYMBOL_GPL(zs_free);
 
@@ -1184,12 +1180,9 @@ EXPORT_SYMBOL_GPL(zs_unmap_object);
 
 u64 zs_get_total_size_bytes(struct zs_pool *pool)
 {
-	int i;
-	u64 npages = 0;
-
-	for (i = 0; i < ZS_SIZE_CLASSES; i++)
-		npages += pool->size_class[i].pages_allocated;
+	u64 npages;
 
+	npages = atomic_read(&pool->pages_allocated);
 	return npages << PAGE_SHIFT;
 }
 EXPORT_SYMBOL_GPL(zs_get_total_size_bytes);
-- 
2.0.0

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

[PATCH 2/3] zram: limit memory size for zram

[Minchan Kim]

Since zram has no control feature to limit memory usage,
it makes hard to manage system memrory.

This patch adds new knob "mem_limit" via sysfs to set up the
limit.

Note: I added the logic in zram, not zsmalloc because the limit
is requirement of zram, not zsmalloc so I'd like to avoid
unnecessary branch in zsmalloc.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 Documentation/blockdev/zram.txt | 20 +++++++++++++++----
 drivers/block/zram/zram_drv.c   | 43 +++++++++++++++++++++++++++++++++++++++++
 drivers/block/zram/zram_drv.h   |  1 +
 3 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
index 0595c3f56ccf..9f239ff8c444 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -74,14 +74,26 @@ There is little point creating a zram of greater than twice the size of memory
 since we expect a 2:1 compression ratio. Note that zram uses about 0.1% of the
 size of the disk when not in use so a huge zram is wasteful.
 
-5) Activate:
+5) Set memory limit: Optional
+	Set memory limit by writing the value to sysfs node 'mem_limit'.
+	The value can be either in bytes or you can use mem suffixes.
+	Examples:
+	    # limit /dev/zram0 with 50MB memory
+	    echo $((50*1024*1024)) > /sys/block/zram0/mem_limit
+
+	    # Using mem suffixes
+	    echo 256K > /sys/block/zram0/mem_limit
+	    echo 512M > /sys/block/zram0/mem_limit
+	    echo 1G > /sys/block/zram0/mem_limit
+
+6) Activate:
 	mkswap /dev/zram0
 	swapon /dev/zram0
 
 	mkfs.ext4 /dev/zram1
 	mount /dev/zram1 /tmp
 
-6) Stats:
+7) Stats:
 	Per-device statistics are exported as various nodes under
 	/sys/block/zram<id>/
 		disksize
@@ -96,11 +108,11 @@ size of the disk when not in use so a huge zram is wasteful.
 		compr_data_size
 		mem_used_total
 
-7) Deactivate:
+8) Deactivate:
 	swapoff /dev/zram0
 	umount /dev/zram1
 
-8) Reset:
+9) Reset:
 	Write any positive value to 'reset' sysfs node
 	echo 1 > /sys/block/zram0/reset
 	echo 1 > /sys/block/zram1/reset
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index d00831c3d731..b48a3d0e9031 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -122,6 +122,35 @@ static ssize_t max_comp_streams_show(struct device *dev,
 	return scnprintf(buf, PAGE_SIZE, "%d\n", val);
 }
 
+static ssize_t mem_limit_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	u64 val;
+	struct zram *zram = dev_to_zram(dev);
+
+	down_read(&zram->init_lock);
+	val = zram->limit_bytes;
+	up_read(&zram->init_lock);
+
+	return scnprintf(buf, PAGE_SIZE, "%llu\n", val);
+}
+
+static ssize_t mem_limit_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
+{
+	u64 limit;
+	struct zram *zram = dev_to_zram(dev);
+
+	limit = memparse(buf, NULL);
+	if (!limit)
+		return -EINVAL;
+
+	down_write(&zram->init_lock);
+	zram->limit_bytes = limit;
+	up_write(&zram->init_lock);
+	return len;
+}
+
 static ssize_t max_comp_streams_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t len)
 {
@@ -513,6 +542,14 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		ret = -ENOMEM;
 		goto out;
 	}
+
+	if (zram->limit_bytes &&
+		zs_get_total_size_bytes(meta->mem_pool) > zram->limit_bytes) {
+		zs_free(meta->mem_pool, handle);
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	cmem = zs_map_object(meta->mem_pool, handle, ZS_MM_WO);
 
 	if ((clen == PAGE_SIZE) && !is_partial_io(bvec)) {
@@ -617,6 +654,9 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
 	struct zram_meta *meta;
 
 	down_write(&zram->init_lock);
+
+	zram->limit_bytes = 0;
+
 	if (!init_done(zram)) {
 		up_write(&zram->init_lock);
 		return;
@@ -857,6 +897,8 @@ static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
 static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
 static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
 static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
+static DEVICE_ATTR(mem_limit, S_IRUGO | S_IWUSR, mem_limit_show,
+		mem_limit_store);
 static DEVICE_ATTR(max_comp_streams, S_IRUGO | S_IWUSR,
 		max_comp_streams_show, max_comp_streams_store);
 static DEVICE_ATTR(comp_algorithm, S_IRUGO | S_IWUSR,
@@ -885,6 +927,7 @@ static struct attribute *zram_disk_attrs[] = {
 	&dev_attr_orig_data_size.attr,
 	&dev_attr_compr_data_size.attr,
 	&dev_attr_mem_used_total.attr,
+	&dev_attr_mem_limit.attr,
 	&dev_attr_max_comp_streams.attr,
 	&dev_attr_comp_algorithm.attr,
 	NULL,
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index e0f725c87cc6..086c51782e75 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -110,6 +110,7 @@ struct zram {
 	 * we can store in a disk.
 	 */
 	u64 disksize;	/* bytes */
+	u64 limit_bytes;
 	int max_comp_streams;
 	struct zram_stats stats;
 	char compressor[10];
-- 
2.0.0

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

[PATCH 3/3] zram: add mem_used_max via sysfs

[Minchan Kim]

Normally, zram user can get maximum memory zsmalloc consumed via
polling mem_used_total with sysfs in userspace.

But it has a critical problem because user can miss peak memory
usage during update interval of polling. For avoiding that,
user should poll it frequently with mlocking to avoid delay
when memory pressure is heavy so it would be handy if the
kernel supports the function.

This patch adds mem_used_max via sysfs.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 Documentation/blockdev/zram.txt |  1 +
 drivers/block/zram/zram_drv.c   | 35 +++++++++++++++++++++++++++++++++--
 drivers/block/zram/zram_drv.h   |  2 ++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
index 9f239ff8c444..3b2247c2d4cf 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -107,6 +107,7 @@ size of the disk when not in use so a huge zram is wasteful.
 		orig_data_size
 		compr_data_size
 		mem_used_total
+		mem_used_max
 
 8) Deactivate:
 	swapoff /dev/zram0
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index b48a3d0e9031..311699f18bd5 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -109,6 +109,30 @@ static ssize_t mem_used_total_show(struct device *dev,
 	return scnprintf(buf, PAGE_SIZE, "%llu\n", val);
 }
 
+static ssize_t mem_used_max_reset(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
+{
+	struct zram *zram = dev_to_zram(dev);
+
+	down_write(&zram->init_lock);
+	zram->max_used_bytes = 0;
+	up_write(&zram->init_lock);
+	return len;
+}
+
+static ssize_t mem_used_max_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	u64 max_used_bytes;
+	struct zram *zram = dev_to_zram(dev);
+
+	down_read(&zram->init_lock);
+	max_used_bytes = zram->max_used_bytes;
+	up_read(&zram->init_lock);
+
+	return scnprintf(buf, PAGE_SIZE, "%llu\n", max_used_bytes);
+}
+
 static ssize_t max_comp_streams_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
@@ -474,6 +498,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	struct zram_meta *meta = zram->meta;
 	struct zcomp_strm *zstrm;
 	bool locked = false;
+	u64 total_bytes;
 
 	page = bvec->bv_page;
 	if (is_partial_io(bvec)) {
@@ -543,8 +568,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		goto out;
 	}
 
-	if (zram->limit_bytes &&
-		zs_get_total_size_bytes(meta->mem_pool) > zram->limit_bytes) {
+	total_bytes = zs_get_total_size_bytes(meta->mem_pool);
+	if (zram->limit_bytes && total_bytes > zram->limit_bytes) {
 		zs_free(meta->mem_pool, handle);
 		ret = -ENOMEM;
 		goto out;
@@ -578,6 +603,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	/* Update stats */
 	atomic64_add(clen, &zram->stats.compr_data_size);
 	atomic64_inc(&zram->stats.pages_stored);
+
+	zram->max_used_bytes = max(zram->max_used_bytes, total_bytes);
 out:
 	if (locked)
 		zcomp_strm_release(zram->comp, zstrm);
@@ -656,6 +683,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
 	down_write(&zram->init_lock);
 
 	zram->limit_bytes = 0;
+	zram->max_used_bytes = 0;
 
 	if (!init_done(zram)) {
 		up_write(&zram->init_lock);
@@ -897,6 +925,8 @@ static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
 static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
 static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
 static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
+static DEVICE_ATTR(mem_used_max, S_IRUGO | S_IWUSR, mem_used_max_show,
+		mem_used_max_reset);
 static DEVICE_ATTR(mem_limit, S_IRUGO | S_IWUSR, mem_limit_show,
 		mem_limit_store);
 static DEVICE_ATTR(max_comp_streams, S_IRUGO | S_IWUSR,
@@ -927,6 +957,7 @@ static struct attribute *zram_disk_attrs[] = {
 	&dev_attr_orig_data_size.attr,
 	&dev_attr_compr_data_size.attr,
 	&dev_attr_mem_used_total.attr,
+	&dev_attr_mem_used_max.attr,
 	&dev_attr_mem_limit.attr,
 	&dev_attr_max_comp_streams.attr,
 	&dev_attr_comp_algorithm.attr,
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 086c51782e75..aca09b18fcbd 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -111,6 +111,8 @@ struct zram {
 	 */
 	u64 disksize;	/* bytes */
 	u64 limit_bytes;
+	u64 max_used_bytes;
+
 	int max_comp_streams;
 	struct zram_stats stats;
 	char compressor[10];
-- 
2.0.0

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

Re: [PATCH 3/3] zram: add mem_used_max via sysfs

[David Horner]

The introduction of a reset can cause the stale zero value to be
retained in the show.
Instead reset to current value.

On Wed, Aug 13, 2014 at 9:12 PM, Minchan Kim <minchan@kernel.org> wrote:
> Normally, zram user can get maximum memory zsmalloc consumed via
> polling mem_used_total with sysfs in userspace.
>
> But it has a critical problem because user can miss peak memory
> usage during update interval of polling. For avoiding that,
> user should poll it frequently with mlocking to avoid delay
> when memory pressure is heavy so it would be handy if the
> kernel supports the function.
>
> This patch adds mem_used_max via sysfs.
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  Documentation/blockdev/zram.txt |  1 +
>  drivers/block/zram/zram_drv.c   | 35 +++++++++++++++++++++++++++++++++--
>  drivers/block/zram/zram_drv.h   |  2 ++
>  3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> index 9f239ff8c444..3b2247c2d4cf 100644
> --- a/Documentation/blockdev/zram.txt
> +++ b/Documentation/blockdev/zram.txt
> @@ -107,6 +107,7 @@ size of the disk when not in use so a huge zram is wasteful.
>                 orig_data_size
>                 compr_data_size
>                 mem_used_total
> +               mem_used_max
>
>  8) Deactivate:
>         swapoff /dev/zram0
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index b48a3d0e9031..311699f18bd5 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -109,6 +109,30 @@ static ssize_t mem_used_total_show(struct device *dev,
>         return scnprintf(buf, PAGE_SIZE, "%llu\n", val);
>  }
>
> +static ssize_t mem_used_max_reset(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t len)

perhaps these are local functions, but wouldn't the zs_ prefix still
be appropriate?
> +{
> +       struct zram *zram = dev_to_zram(dev);
> +
> +       down_write(&zram->init_lock);
> +       zram->max_used_bytes = 0;

           zram->max_used_bytes = zs_get_total_size_bytes(meta->mem_pool);

           (where meta is set up as below  (beyond my skill level at
the moment)).

> +       up_write(&zram->init_lock);
> +       return len;
> +}
> +
> +static ssize_t mem_used_max_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       u64 max_used_bytes;
> +       struct zram *zram = dev_to_zram(dev);
> +
> +       down_read(&zram->init_lock);

if these are atomic operations, why the (read and write) locks?

> +       max_used_bytes = zram->max_used_bytes;
> +       up_read(&zram->init_lock);
> +
> +       return scnprintf(buf, PAGE_SIZE, "%llu\n", max_used_bytes);
> +}
> +
>  static ssize_t max_comp_streams_show(struct device *dev,
>                 struct device_attribute *attr, char *buf)
>  {
> @@ -474,6 +498,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>         struct zram_meta *meta = zram->meta;
>         struct zcomp_strm *zstrm;
>         bool locked = false;
> +       u64 total_bytes;
>
>         page = bvec->bv_page;
>         if (is_partial_io(bvec)) {
> @@ -543,8 +568,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>                 goto out;
>         }
>
> -       if (zram->limit_bytes &&
> -               zs_get_total_size_bytes(meta->mem_pool) > zram->limit_bytes) {
> +       total_bytes = zs_get_total_size_bytes(meta->mem_pool);
> +       if (zram->limit_bytes && total_bytes > zram->limit_bytes) {
>                 zs_free(meta->mem_pool, handle);
>                 ret = -ENOMEM;
>                 goto out;
> @@ -578,6 +603,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>         /* Update stats */
>         atomic64_add(clen, &zram->stats.compr_data_size);
>         atomic64_inc(&zram->stats.pages_stored);
> +
> +       zram->max_used_bytes = max(zram->max_used_bytes, total_bytes);
>  out:
>         if (locked)
>                 zcomp_strm_release(zram->comp, zstrm);
> @@ -656,6 +683,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
>         down_write(&zram->init_lock);
>
>         zram->limit_bytes = 0;
> +       zram->max_used_bytes = 0;
>
>         if (!init_done(zram)) {
>                 up_write(&zram->init_lock);
> @@ -897,6 +925,8 @@ static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
>  static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
>  static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
>  static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
> +static DEVICE_ATTR(mem_used_max, S_IRUGO | S_IWUSR, mem_used_max_show,
> +               mem_used_max_reset);
>  static DEVICE_ATTR(mem_limit, S_IRUGO | S_IWUSR, mem_limit_show,
>                 mem_limit_store);
>  static DEVICE_ATTR(max_comp_streams, S_IRUGO | S_IWUSR,
> @@ -927,6 +957,7 @@ static struct attribute *zram_disk_attrs[] = {
>         &dev_attr_orig_data_size.attr,
>         &dev_attr_compr_data_size.attr,
>         &dev_attr_mem_used_total.attr,
> +       &dev_attr_mem_used_max.attr,
>         &dev_attr_mem_limit.attr,
>         &dev_attr_max_comp_streams.attr,
>         &dev_attr_comp_algorithm.attr,
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 086c51782e75..aca09b18fcbd 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -111,6 +111,8 @@ struct zram {
>          */
>         u64 disksize;   /* bytes */
>         u64 limit_bytes;
> +       u64 max_used_bytes;
> +
>         int max_comp_streams;
>         struct zram_stats stats;
>         char compressor[10];
> --
> 2.0.0
>

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

Re: [PATCH 3/3] zram: add mem_used_max via sysfs

[Dan Streetman]

On Wed, Aug 13, 2014 at 9:12 PM, Minchan Kim <minchan@kernel.org> wrote:
> Normally, zram user can get maximum memory zsmalloc consumed via
> polling mem_used_total with sysfs in userspace.
>
> But it has a critical problem because user can miss peak memory
> usage during update interval of polling. For avoiding that,
> user should poll it frequently with mlocking to avoid delay
> when memory pressure is heavy so it would be handy if the
> kernel supports the function.
>
> This patch adds mem_used_max via sysfs.
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  Documentation/blockdev/zram.txt |  1 +
>  drivers/block/zram/zram_drv.c   | 35 +++++++++++++++++++++++++++++++++--
>  drivers/block/zram/zram_drv.h   |  2 ++
>  3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> index 9f239ff8c444..3b2247c2d4cf 100644
> --- a/Documentation/blockdev/zram.txt
> +++ b/Documentation/blockdev/zram.txt
> @@ -107,6 +107,7 @@ size of the disk when not in use so a huge zram is wasteful.
>                 orig_data_size
>                 compr_data_size
>                 mem_used_total
> +               mem_used_max
>
>  8) Deactivate:
>         swapoff /dev/zram0
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index b48a3d0e9031..311699f18bd5 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -109,6 +109,30 @@ static ssize_t mem_used_total_show(struct device *dev,
>         return scnprintf(buf, PAGE_SIZE, "%llu\n", val);
>  }
>
> +static ssize_t mem_used_max_reset(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t len)
> +{
> +       struct zram *zram = dev_to_zram(dev);
> +
> +       down_write(&zram->init_lock);
> +       zram->max_used_bytes = 0;
> +       up_write(&zram->init_lock);
> +       return len;
> +}
> +
> +static ssize_t mem_used_max_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       u64 max_used_bytes;
> +       struct zram *zram = dev_to_zram(dev);
> +
> +       down_read(&zram->init_lock);
> +       max_used_bytes = zram->max_used_bytes;
> +       up_read(&zram->init_lock);
> +
> +       return scnprintf(buf, PAGE_SIZE, "%llu\n", max_used_bytes);
> +}
> +
>  static ssize_t max_comp_streams_show(struct device *dev,
>                 struct device_attribute *attr, char *buf)
>  {
> @@ -474,6 +498,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>         struct zram_meta *meta = zram->meta;
>         struct zcomp_strm *zstrm;
>         bool locked = false;
> +       u64 total_bytes;
>
>         page = bvec->bv_page;
>         if (is_partial_io(bvec)) {
> @@ -543,8 +568,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>                 goto out;
>         }
>
> -       if (zram->limit_bytes &&
> -               zs_get_total_size_bytes(meta->mem_pool) > zram->limit_bytes) {
> +       total_bytes = zs_get_total_size_bytes(meta->mem_pool);
> +       if (zram->limit_bytes && total_bytes > zram->limit_bytes) {

do you need to take the init_lock to read limit_bytes here?  It could
be getting changed between these checks...

>                 zs_free(meta->mem_pool, handle);
>                 ret = -ENOMEM;
>                 goto out;
> @@ -578,6 +603,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>         /* Update stats */
>         atomic64_add(clen, &zram->stats.compr_data_size);
>         atomic64_inc(&zram->stats.pages_stored);
> +
> +       zram->max_used_bytes = max(zram->max_used_bytes, total_bytes);

shouldn't max_used_bytes be atomic64_t?  Or take the init_lock here?

>  out:
>         if (locked)
>                 zcomp_strm_release(zram->comp, zstrm);
> @@ -656,6 +683,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
>         down_write(&zram->init_lock);
>
>         zram->limit_bytes = 0;
> +       zram->max_used_bytes = 0;
>
>         if (!init_done(zram)) {
>                 up_write(&zram->init_lock);
> @@ -897,6 +925,8 @@ static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
>  static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
>  static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
>  static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
> +static DEVICE_ATTR(mem_used_max, S_IRUGO | S_IWUSR, mem_used_max_show,
> +               mem_used_max_reset);
>  static DEVICE_ATTR(mem_limit, S_IRUGO | S_IWUSR, mem_limit_show,
>                 mem_limit_store);
>  static DEVICE_ATTR(max_comp_streams, S_IRUGO | S_IWUSR,
> @@ -927,6 +957,7 @@ static struct attribute *zram_disk_attrs[] = {
>         &dev_attr_orig_data_size.attr,
>         &dev_attr_compr_data_size.attr,
>         &dev_attr_mem_used_total.attr,
> +       &dev_attr_mem_used_max.attr,
>         &dev_attr_mem_limit.attr,
>         &dev_attr_max_comp_streams.attr,
>         &dev_attr_comp_algorithm.attr,
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 086c51782e75..aca09b18fcbd 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -111,6 +111,8 @@ struct zram {
>          */
>         u64 disksize;   /* bytes */
>         u64 limit_bytes;
> +       u64 max_used_bytes;
> +
>         int max_comp_streams;
>         struct zram_stats stats;
>         char compressor[10];
> --
> 2.0.0
>

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

Re: [PATCH 3/3] zram: add mem_used_max via sysfs

[David Horner]

On Thu, Aug 14, 2014 at 11:09 AM, Dan Streetman <ddstreet@ieee.org> wrote:
> On Wed, Aug 13, 2014 at 9:12 PM, Minchan Kim <minchan@kernel.org> wrote:
>> -       if (zram->limit_bytes &&
>> -               zs_get_total_size_bytes(meta->mem_pool) > zram->limit_bytes) {
>> +       total_bytes = zs_get_total_size_bytes(meta->mem_pool);
>> +       if (zram->limit_bytes && total_bytes > zram->limit_bytes) {
>
> do you need to take the init_lock to read limit_bytes here?  It could
> be getting changed between these checks...

There is no real danger in freeing with an error.
It is more timing than a race.

The max calculation is still ok because committed allocations are
added atomically.

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

Re: [PATCH 3/3] zram: add mem_used_max via sysfs

[David Horner]

On Thu, Aug 14, 2014 at 11:32 AM, David Horner <ds2horner@gmail.com> wrote:
> On Thu, Aug 14, 2014 at 11:09 AM, Dan Streetman <ddstreet@ieee.org> wrote:
>> On Wed, Aug 13, 2014 at 9:12 PM, Minchan Kim <minchan@kernel.org> wrote:
>>> -       if (zram->limit_bytes &&
>>> -               zs_get_total_size_bytes(meta->mem_pool) > zram->limit_bytes) {
>>> +       total_bytes = zs_get_total_size_bytes(meta->mem_pool);
>>> +       if (zram->limit_bytes && total_bytes > zram->limit_bytes) {
>>
>> do you need to take the init_lock to read limit_bytes here?  It could
>> be getting changed between these checks...
>
> There is no real danger in freeing with an error.
> It is more timing than a race.
I probably should explain my reasoning.

any changes between getting the total value and the limit test are not
problematic (From race perspective).

1) If the actual total increases and the value returned under rates it, then
a) if this.total exceeds the limit - no problem it is rolled back as
it would if the actual total were used.
b) if this.total <= limit OK - as other process will be dinged (it
will see its own allocation)

2)  If the actual total decreases and the value returned overrates
rates it, then
a) if this.value <= limit then allocation great (actual has even more room)
b) if this.value > max it will be rolled back (as the other might be
as well) and process can compete again.

Is there a denial of service possible if 2.b repeats indefinitely.
Yes, but how to set it up reliably? And it is no different than a
single user exhausting the limit before any other users.
Yes, it is potentially a live false limit exhaustion, with one process
requesting an amount exceeding the limit but able to be allocated.
 But this is no worse than the rollback load we already have at the limit.

It would be better to check before the zs_malloc if the concern is
avoiding heavy processing in that function (as an optimization),  as
well as here.after allocation

But I see no real race or danger doing this unlocked.

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

Re: [PATCH 3/3] zram: add mem_used_max via sysfs

[Dan Streetman]

On Thu, Aug 14, 2014 at 12:23 PM, David Horner <ds2horner@gmail.com> wrote:
> On Thu, Aug 14, 2014 at 11:32 AM, David Horner <ds2horner@gmail.com> wrote:
>> On Thu, Aug 14, 2014 at 11:09 AM, Dan Streetman <ddstreet@ieee.org> wrote:
>>> On Wed, Aug 13, 2014 at 9:12 PM, Minchan Kim <minchan@kernel.org> wrote:
>>>> -       if (zram->limit_bytes &&
>>>> -               zs_get_total_size_bytes(meta->mem_pool) > zram->limit_bytes) {
>>>> +       total_bytes = zs_get_total_size_bytes(meta->mem_pool);
>>>> +       if (zram->limit_bytes && total_bytes > zram->limit_bytes) {
>>>
>>> do you need to take the init_lock to read limit_bytes here?  It could
>>> be getting changed between these checks...
>>
>> There is no real danger in freeing with an error.
>> It is more timing than a race.
> I probably should explain my reasoning.
>
> any changes between getting the total value and the limit test are not
> problematic (From race perspective).
>
> 1) If the actual total increases and the value returned under rates it, then
> a) if this.total exceeds the limit - no problem it is rolled back as
> it would if the actual total were used.
> b) if this.total <= limit OK - as other process will be dinged (it
> will see its own allocation)
>
> 2)  If the actual total decreases and the value returned overrates
> rates it, then
> a) if this.value <= limit then allocation great (actual has even more room)
> b) if this.value > max it will be rolled back (as the other might be
> as well) and process can compete again.

actually I wasn't thinking of total_bytes changing, i think it's ok to
check the total at that specific point in time, for the reasons you
point out above.

I was thinking about zram->limit_bytes changing, especially if it's
possible to disable the limit (i.e. set it to 0), e.g.:

assume currently total_bytes == 1G and limit_bytes == 2G, so there is
not currently any danger of going over the limit.  Then:


thread 1 : if (zram->limit_bytes  ...this is true

thread 2 : zram->limit_bytes = limit;    ...where limit == 0

thread 1 : && total_bytes > zram->limit_bytes) {   ...this is now also true

thread 1 : incorrectly return -ENOMEM failure

It's very unlikely, and a single failure isn't a big deal here since
the caller must be prepared to handle a failure.  And of course the
compiler might reorder those checks.  And if it's not possible to
disable the limit by setting it to 0 (besides a complete reset of the
zram device, which wouldn't happen while this function is running),
then there's not an issue here (although, I think being able to
disable the limit without having to reset the zram device is useful).


Also for setting the max_used_bytes, isn't non-atomically setting a
u64 value dangerous (on <64 bit systems) when it's not synchronized
between threads?

That is, unless the entire zram_bvec_write() function or this section
is already thread-safe, and i missed it (which i may have :-)


>
> Is there a denial of service possible if 2.b repeats indefinitely.
> Yes, but how to set it up reliably? And it is no different than a
> single user exhausting the limit before any other users.
> Yes, it is potentially a live false limit exhaustion, with one process
> requesting an amount exceeding the limit but able to be allocated.
>  But this is no worse than the rollback load we already have at the limit.
>
> It would be better to check before the zs_malloc if the concern is
> avoiding heavy processing in that function (as an optimization),  as
> well as here.after allocation
>
> But I see no real race or danger doing this unlocked.

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

Re: [PATCH 3/3] zram: add mem_used_max via sysfs

[David Horner]

On Thu, Aug 14, 2014 at 3:11 PM, Dan Streetman <ddstreet@ieee.org> wrote:
> On Thu, Aug 14, 2014 at 12:23 PM, David Horner <ds2horner@gmail.com> wrote:
>> On Thu, Aug 14, 2014 at 11:32 AM, David Horner <ds2horner@gmail.com> wrote:
>>> On Thu, Aug 14, 2014 at 11:09 AM, Dan Streetman <ddstreet@ieee.org> wrote:
>>>> On Wed, Aug 13, 2014 at 9:12 PM, Minchan Kim <minchan@kernel.org> wrote:
>>>>> -       if (zram->limit_bytes &&
>>>>> -               zs_get_total_size_bytes(meta->mem_pool) > zram->limit_bytes) {
>>>>> +       total_bytes = zs_get_total_size_bytes(meta->mem_pool);
>>>>> +       if (zram->limit_bytes && total_bytes > zram->limit_bytes) {
>>>>
>>>> do you need to take the init_lock to read limit_bytes here?  It could
>>>> be getting changed between these checks...
>>>
>>> There is no real danger in freeing with an error.
>>> It is more timing than a race.
>> I probably should explain my reasoning.
>>
>> any changes between getting the total value and the limit test are not
>> problematic (From race perspective).
>>
>> 1) If the actual total increases and the value returned under rates it, then
>> a) if this.total exceeds the limit - no problem it is rolled back as
>> it would if the actual total were used.
>> b) if this.total <= limit OK - as other process will be dinged (it
>> will see its own allocation)
>>
>> 2)  If the actual total decreases and the value returned overrates
>> rates it, then
>> a) if this.value <= limit then allocation great (actual has even more room)
>> b) if this.value > max it will be rolled back (as the other might be
>> as well) and process can compete again.
>

for completeness I should have mentioned the normal decrease case of
deallocation
and not roll back.
(and of course it is also not a problem and does not race).

Are these typical situations in documentation folder
(I know the related memory barriers is)
It would be so much better to say scenario 23 is a potential problem
rather than rewriting the essays.


> actually I wasn't thinking of total_bytes changing, i think it's ok to
> check the total at that specific point in time, for the reasons you
> point out above.
>
> I was thinking about zram->limit_bytes changing, especially if it's
> possible to disable the limit (i.e. set it to 0), e.g.:
>
> assume currently total_bytes == 1G and limit_bytes == 2G, so there is
> not currently any danger of going over the limit.  Then:
>
>
> thread 1 : if (zram->limit_bytes  ...this is true
>
> thread 2 : zram->limit_bytes = limit;    ...where limit == 0
>
> thread 1 : && total_bytes > zram->limit_bytes) {   ...this is now also true
>
> thread 1 : incorrectly return -ENOMEM failure
>
> It's very unlikely, and a single failure isn't a big deal here since
> the caller must be prepared to handle a failure.  And of course the
> compiler might reorder those checks.  And if it's not possible to
> disable the limit by setting it to 0 (besides a complete reset of the
> zram device, which wouldn't happen while this function is running),
> then there's not an issue here (although, I think being able to
> disable the limit without having to reset the zram device is useful).

agreed on 7 of 8 assertions
 (not yet sure about reset not happening while function running).

That issue then arises in [PATCH 2/2] zram: limit memory size for zram
and as you mention reordering the zero check after the limit comparison
in the if statement could be reordered by the compiler

As I see it this is also a timing issue - as you explained, and not a race.

Perhaps we name it scenario 24?

And especially I agree with allowing zero limit reset without device reset.
The equivalent is currently possible (for all practical purposes)
anyway by setting
the limit to max_u64.
So allowing zero is cleaner.


>
>
> Also for setting the max_used_bytes, isn't non-atomically setting a
> u64 value dangerous (on <64 bit systems) when it's not synchronized
> between threads?

perhaps it needs an atomic function - I will think some more on it.

>
> That is, unless the entire zram_bvec_write() function or this section
> is already thread-safe, and i missed it (which i may have :-)

nor have I.checked.(on the to do).

>
>
>>

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

Re: [PATCH 3/3] zram: add mem_used_max via sysfs

[Minchan Kim]

Hi David,

On Thu, Aug 14, 2014 at 06:29:17AM -0400, David Horner wrote:
> The introduction of a reset can cause the stale zero value to be
> retained in the show.
> Instead reset to current value.

It's better. I will do.
Thanks!

> 
> On Wed, Aug 13, 2014 at 9:12 PM, Minchan Kim <minchan@kernel.org> wrote:
> > Normally, zram user can get maximum memory zsmalloc consumed via
> > polling mem_used_total with sysfs in userspace.
> >
> > But it has a critical problem because user can miss peak memory
> > usage during update interval of polling. For avoiding that,
> > user should poll it frequently with mlocking to avoid delay
> > when memory pressure is heavy so it would be handy if the
> > kernel supports the function.
> >
> > This patch adds mem_used_max via sysfs.
> >
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  Documentation/blockdev/zram.txt |  1 +
> >  drivers/block/zram/zram_drv.c   | 35 +++++++++++++++++++++++++++++++++--
> >  drivers/block/zram/zram_drv.h   |  2 ++
> >  3 files changed, 36 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> > index 9f239ff8c444..3b2247c2d4cf 100644
> > --- a/Documentation/blockdev/zram.txt
> > +++ b/Documentation/blockdev/zram.txt
> > @@ -107,6 +107,7 @@ size of the disk when not in use so a huge zram is wasteful.
> >                 orig_data_size
> >                 compr_data_size
> >                 mem_used_total
> > +               mem_used_max
> >
> >  8) Deactivate:
> >         swapoff /dev/zram0
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index b48a3d0e9031..311699f18bd5 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -109,6 +109,30 @@ static ssize_t mem_used_total_show(struct device *dev,
> >         return scnprintf(buf, PAGE_SIZE, "%llu\n", val);
> >  }
> >
> > +static ssize_t mem_used_max_reset(struct device *dev,
> > +               struct device_attribute *attr, const char *buf, size_t len)
> 
> perhaps these are local functions, but wouldn't the zs_ prefix still
> be appropriate?
> > +{
> > +       struct zram *zram = dev_to_zram(dev);
> > +
> > +       down_write(&zram->init_lock);
> > +       zram->max_used_bytes = 0;
> 
>            zram->max_used_bytes = zs_get_total_size_bytes(meta->mem_pool);
> 
>            (where meta is set up as below  (beyond my skill level at
> the moment)).
> 
> > +       up_write(&zram->init_lock);
> > +       return len;
> > +}
> > +
> > +static ssize_t mem_used_max_show(struct device *dev,
> > +               struct device_attribute *attr, char *buf)
> > +{
> > +       u64 max_used_bytes;
> > +       struct zram *zram = dev_to_zram(dev);
> > +
> > +       down_read(&zram->init_lock);
> 
> if these are atomic operations, why the (read and write) locks?
> 
> > +       max_used_bytes = zram->max_used_bytes;
> > +       up_read(&zram->init_lock);
> > +
> > +       return scnprintf(buf, PAGE_SIZE, "%llu\n", max_used_bytes);
> > +}
> > +
> >  static ssize_t max_comp_streams_show(struct device *dev,
> >                 struct device_attribute *attr, char *buf)
> >  {
> > @@ -474,6 +498,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >         struct zram_meta *meta = zram->meta;
> >         struct zcomp_strm *zstrm;
> >         bool locked = false;
> > +       u64 total_bytes;
> >
> >         page = bvec->bv_page;
> >         if (is_partial_io(bvec)) {
> > @@ -543,8 +568,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >                 goto out;
> >         }
> >
> > -       if (zram->limit_bytes &&
> > -               zs_get_total_size_bytes(meta->mem_pool) > zram->limit_bytes) {
> > +       total_bytes = zs_get_total_size_bytes(meta->mem_pool);
> > +       if (zram->limit_bytes && total_bytes > zram->limit_bytes) {
> >                 zs_free(meta->mem_pool, handle);
> >                 ret = -ENOMEM;
> >                 goto out;
> > @@ -578,6 +603,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >         /* Update stats */
> >         atomic64_add(clen, &zram->stats.compr_data_size);
> >         atomic64_inc(&zram->stats.pages_stored);
> > +
> > +       zram->max_used_bytes = max(zram->max_used_bytes, total_bytes);
> >  out:
> >         if (locked)
> >                 zcomp_strm_release(zram->comp, zstrm);
> > @@ -656,6 +683,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> >         down_write(&zram->init_lock);
> >
> >         zram->limit_bytes = 0;
> > +       zram->max_used_bytes = 0;
> >
> >         if (!init_done(zram)) {
> >                 up_write(&zram->init_lock);
> > @@ -897,6 +925,8 @@ static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
> >  static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
> >  static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
> >  static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
> > +static DEVICE_ATTR(mem_used_max, S_IRUGO | S_IWUSR, mem_used_max_show,
> > +               mem_used_max_reset);
> >  static DEVICE_ATTR(mem_limit, S_IRUGO | S_IWUSR, mem_limit_show,
> >                 mem_limit_store);
> >  static DEVICE_ATTR(max_comp_streams, S_IRUGO | S_IWUSR,
> > @@ -927,6 +957,7 @@ static struct attribute *zram_disk_attrs[] = {
> >         &dev_attr_orig_data_size.attr,
> >         &dev_attr_compr_data_size.attr,
> >         &dev_attr_mem_used_total.attr,
> > +       &dev_attr_mem_used_max.attr,
> >         &dev_attr_mem_limit.attr,
> >         &dev_attr_max_comp_streams.attr,
> >         &dev_attr_comp_algorithm.attr,
> > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > index 086c51782e75..aca09b18fcbd 100644
> > --- a/drivers/block/zram/zram_drv.h
> > +++ b/drivers/block/zram/zram_drv.h
> > @@ -111,6 +111,8 @@ struct zram {
> >          */
> >         u64 disksize;   /* bytes */
> >         u64 limit_bytes;
> > +       u64 max_used_bytes;
> > +
> >         int max_comp_streams;
> >         struct zram_stats stats;
> >         char compressor[10];
> > --
> > 2.0.0
> >
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Kind regards,
Minchan Kim

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

Re: [PATCH 3/3] zram: add mem_used_max via sysfs

[Minchan Kim]

Hello Dan,

On Thu, Aug 14, 2014 at 11:09:05AM -0400, Dan Streetman wrote:
> On Wed, Aug 13, 2014 at 9:12 PM, Minchan Kim <minchan@kernel.org> wrote:
> > Normally, zram user can get maximum memory zsmalloc consumed via
> > polling mem_used_total with sysfs in userspace.
> >
> > But it has a critical problem because user can miss peak memory
> > usage during update interval of polling. For avoiding that,
> > user should poll it frequently with mlocking to avoid delay
> > when memory pressure is heavy so it would be handy if the
> > kernel supports the function.
> >
> > This patch adds mem_used_max via sysfs.
> >
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  Documentation/blockdev/zram.txt |  1 +
> >  drivers/block/zram/zram_drv.c   | 35 +++++++++++++++++++++++++++++++++--
> >  drivers/block/zram/zram_drv.h   |  2 ++
> >  3 files changed, 36 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> > index 9f239ff8c444..3b2247c2d4cf 100644
> > --- a/Documentation/blockdev/zram.txt
> > +++ b/Documentation/blockdev/zram.txt
> > @@ -107,6 +107,7 @@ size of the disk when not in use so a huge zram is wasteful.
> >                 orig_data_size
> >                 compr_data_size
> >                 mem_used_total
> > +               mem_used_max
> >
> >  8) Deactivate:
> >         swapoff /dev/zram0
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index b48a3d0e9031..311699f18bd5 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -109,6 +109,30 @@ static ssize_t mem_used_total_show(struct device *dev,
> >         return scnprintf(buf, PAGE_SIZE, "%llu\n", val);
> >  }
> >
> > +static ssize_t mem_used_max_reset(struct device *dev,
> > +               struct device_attribute *attr, const char *buf, size_t len)
> > +{
> > +       struct zram *zram = dev_to_zram(dev);
> > +
> > +       down_write(&zram->init_lock);
> > +       zram->max_used_bytes = 0;
> > +       up_write(&zram->init_lock);
> > +       return len;
> > +}
> > +
> > +static ssize_t mem_used_max_show(struct device *dev,
> > +               struct device_attribute *attr, char *buf)
> > +{
> > +       u64 max_used_bytes;
> > +       struct zram *zram = dev_to_zram(dev);
> > +
> > +       down_read(&zram->init_lock);
> > +       max_used_bytes = zram->max_used_bytes;
> > +       up_read(&zram->init_lock);
> > +
> > +       return scnprintf(buf, PAGE_SIZE, "%llu\n", max_used_bytes);
> > +}
> > +
> >  static ssize_t max_comp_streams_show(struct device *dev,
> >                 struct device_attribute *attr, char *buf)
> >  {
> > @@ -474,6 +498,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >         struct zram_meta *meta = zram->meta;
> >         struct zcomp_strm *zstrm;
> >         bool locked = false;
> > +       u64 total_bytes;
> >
> >         page = bvec->bv_page;
> >         if (is_partial_io(bvec)) {
> > @@ -543,8 +568,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >                 goto out;
> >         }
> >
> > -       if (zram->limit_bytes &&
> > -               zs_get_total_size_bytes(meta->mem_pool) > zram->limit_bytes) {
> > +       total_bytes = zs_get_total_size_bytes(meta->mem_pool);
> > +       if (zram->limit_bytes && total_bytes > zram->limit_bytes) {
> 
> do you need to take the init_lock to read limit_bytes here?  It could
> be getting changed between these checks...

The zram_bvec_write is protected by read-side init_lock while mem_limit_store
is proteced by write-side init_lock.

> 
> >                 zs_free(meta->mem_pool, handle);
> >                 ret = -ENOMEM;
> >                 goto out;
> > @@ -578,6 +603,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >         /* Update stats */
> >         atomic64_add(clen, &zram->stats.compr_data_size);
> >         atomic64_inc(&zram->stats.pages_stored);
> > +
> > +       zram->max_used_bytes = max(zram->max_used_bytes, total_bytes);
> 
> shouldn't max_used_bytes be atomic64_t?  Or take the init_lock here?
> 
> >  out:
> >         if (locked)
> >                 zcomp_strm_release(zram->comp, zstrm);
> > @@ -656,6 +683,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> >         down_write(&zram->init_lock);
> >
> >         zram->limit_bytes = 0;
> > +       zram->max_used_bytes = 0;
> >
> >         if (!init_done(zram)) {
> >                 up_write(&zram->init_lock);
> > @@ -897,6 +925,8 @@ static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
> >  static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
> >  static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
> >  static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
> > +static DEVICE_ATTR(mem_used_max, S_IRUGO | S_IWUSR, mem_used_max_show,
> > +               mem_used_max_reset);
> >  static DEVICE_ATTR(mem_limit, S_IRUGO | S_IWUSR, mem_limit_show,
> >                 mem_limit_store);
> >  static DEVICE_ATTR(max_comp_streams, S_IRUGO | S_IWUSR,
> > @@ -927,6 +957,7 @@ static struct attribute *zram_disk_attrs[] = {
> >         &dev_attr_orig_data_size.attr,
> >         &dev_attr_compr_data_size.attr,
> >         &dev_attr_mem_used_total.attr,
> > +       &dev_attr_mem_used_max.attr,
> >         &dev_attr_mem_limit.attr,
> >         &dev_attr_max_comp_streams.attr,
> >         &dev_attr_comp_algorithm.attr,
> > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > index 086c51782e75..aca09b18fcbd 100644
> > --- a/drivers/block/zram/zram_drv.h
> > +++ b/drivers/block/zram/zram_drv.h
> > @@ -111,6 +111,8 @@ struct zram {
> >          */
> >         u64 disksize;   /* bytes */
> >         u64 limit_bytes;
> > +       u64 max_used_bytes;
> > +
> >         int max_comp_streams;
> >         struct zram_stats stats;
> >         char compressor[10];
> > --
> > 2.0.0
> >
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Kind regards,
Minchan Kim

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

Re: [PATCH 3/3] zram: add mem_used_max via sysfs

[Minchan Kim]

On Thu, Aug 14, 2014 at 11:32:36AM -0400, David Horner wrote:
> On Thu, Aug 14, 2014 at 11:09 AM, Dan Streetman <ddstreet@ieee.org> wrote:
> > On Wed, Aug 13, 2014 at 9:12 PM, Minchan Kim <minchan@kernel.org> wrote:
> >> -       if (zram->limit_bytes &&
> >> -               zs_get_total_size_bytes(meta->mem_pool) > zram->limit_bytes) {
> >> +       total_bytes = zs_get_total_size_bytes(meta->mem_pool);
> >> +       if (zram->limit_bytes && total_bytes > zram->limit_bytes) {
> >
> > do you need to take the init_lock to read limit_bytes here?  It could
> > be getting changed between these checks...
> 
> There is no real danger in freeing with an error.
> It is more timing than a race.
> 
> The max calculation is still ok because committed allocations are
> added atomically.

There is one problem in below code piece.

zram->max_used_bytes = max(zram->max_used_bytes, total_bytes);

so we should consider this case.

if (zram->max_used_bytes < total_bytes)
        zram->max_used_bytes = total_bytes;

And we could make the situation like this.


if (zram->max_used_bytes < total_bytes)
        IRQ happen;
        zram->max_used_bytes = total_bytes

During IRQ, other CPU could consume a lot of zsmalloc memory so that
zram->max_used_bytes would be increased under the foot so when IRQ is
finshed, zram->max_used_bytes could be reset with old total_bytes.

To prevent it, we should use the lock I posted RFC version or retry
logic with atomic opeartion(ie, cmpxchg) and my approach makes it simple
first and fix it if we see the trouble in future so my preference is
new spin lock at the moment.

Any comments?



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

-- 
Kind regards,
Minchan Kim

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