Re: [RFC 3/3] zram: limit memory size for zram

[Minchan Kim <minchan@kernel.org>]

Hello Seth,

On Wed, Aug 13, 2014 at 10:30:20AM -0500, Seth Jennings wrote:
> On Wed, Aug 06, 2014 at 03:52:53PM +0900, Minchan Kim wrote:
> > On Tue, Aug 05, 2014 at 10:16:15PM +0900, Sergey Senozhatsky wrote:
> > > Hello,
> > > 
> > > On (08/05/14 18:48), Minchan Kim wrote:
> > > > Another idea: we could define void zs_limit_mem(unsinged long nr_pages)
> > > > in zsmalloc and put the limit in zs_pool via new API from zram so that
> > > > zs_malloc could be failed as soon as it exceeds the limit.
> > > > 
> > > > In the end, zram doesn't need to call zs_get_total_size_bytes on every
> > > > write. It's more clean and right layer, IMHO.
> > > 
> > > yes, I think this one is better.
> > > 
> > > 	-ss
> > 
> > From 279c406b5a8eabd03edca55490ec92b539b39c76 Mon Sep 17 00:00:00 2001
> > From: Minchan Kim <minchan@kernel.org>
> > Date: Tue, 5 Aug 2014 16:24:57 +0900
> > Subject: [PATCH] zram: limit memory size for zram
> > 
> > I have received a request several time from zram users.
> > They want to limit memory size for zram because zram can consume
> > lot of memory on system without limit so it makes memory management
> > control hard.
> > 
> > This patch adds new knob to limit memory of zram.
> > 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  Documentation/blockdev/zram.txt |  1 +
> >  drivers/block/zram/zram_drv.c   | 39 +++++++++++++++++++++++++++++++++++++--
> >  include/linux/zsmalloc.h        |  2 ++
> >  mm/zsmalloc.c                   | 24 ++++++++++++++++++++++++
> >  4 files changed, 64 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> > index d24534bee763..fcb0561dfe2e 100644
> > --- a/Documentation/blockdev/zram.txt
> > +++ b/Documentation/blockdev/zram.txt
> > @@ -96,6 +96,7 @@ size of the disk when not in use so a huge zram is wasteful.
> >  		compr_data_size
> >  		mem_used_total
> >  		mem_used_max
> > +		mem_limit
> >  
> >  7) Deactivate:
> >  	swapoff /dev/zram0
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index a4d637b4db7d..069e81ef0c17 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -137,6 +137,41 @@ 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 = 0;
> > +	struct zram *zram = dev_to_zram(dev);
> > +	struct zram_meta *meta = zram->meta;
> > +
> > +	down_read(&zram->init_lock);
> > +	if (init_done(zram))
> > +		val = zs_get_limit_size_bytes(meta->mem_pool);
> > +	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)
> > +{
> > +	int ret;
> > +	u64 limit;
> > +	struct zram *zram = dev_to_zram(dev);
> > +	struct zram_meta *meta = zram->meta;
> > +
> > +	ret = kstrtoull(buf, 0, &limit);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	down_write(&zram->init_lock);
> > +	if (init_done(zram))
> > +		zs_set_limit_size_bytes(meta->mem_pool, limit);
> > +	up_write(&zram->init_lock);
> > +	ret = len;
> > +	return ret;
> > +}
> > +
> >  static ssize_t max_comp_streams_store(struct device *dev,
> >  		struct device_attribute *attr, const char *buf, size_t len)
> >  {
> > @@ -506,8 +541,6 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  
> >  	handle = zs_malloc(meta->mem_pool, clen);
> >  	if (!handle) {
> > -		pr_info("Error allocating memory for compressed page: %u, size=%zu\n",
> > -			index, clen);
> >  		ret = -ENOMEM;
> >  		goto out;
> >  	}
> > @@ -854,6 +887,7 @@ 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, mem_used_max_show, NULL);
> > +static DEVICE_ATTR(mem_limit, S_IRUGO, 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,
> > @@ -883,6 +917,7 @@ static struct attribute *zram_disk_attrs[] = {
> >  	&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,
> >  	NULL,
> > diff --git a/include/linux/zsmalloc.h b/include/linux/zsmalloc.h
> > index fb087ca06a88..41122251a2d0 100644
> > --- a/include/linux/zsmalloc.h
> > +++ b/include/linux/zsmalloc.h
> > @@ -49,4 +49,6 @@ void zs_unmap_object(struct zs_pool *pool, unsigned long handle);
> >  u64 zs_get_total_size_bytes(struct zs_pool *pool);
> >  u64 zs_get_max_size_bytes(struct zs_pool *pool);
> >  
> > +u64 zs_get_limit_size_bytes(struct zs_pool *pool);
> > +void zs_set_limit_size_bytes(struct zs_pool *pool, u64 limit);
> 
> While having a function to change the limit is fine, the setting of the
> initial limit should be a parameter to zs_create_pool() since, if the
> user doesn't call zs_set_limit_size_bytes() after zs_create_pool(), the
> default size is 0.
> 
> This also breaks zswap which does its pool size limiting in the zswap
> layer using zs_get_total_size_bytes() to poll for the pool size.
> 
> It also has implications for the new zpool abstraction layer which
> doesn't have a handle for setting the pool limit.

Just I sent a my opinion. I'd like to avoid limit logic into zsmalloc.
Instead, I'd like to add it in zram part like zswap.
It doesn't break anything so you would be happy. :)

> 
> Could you do what zswap does already and enforce the pool limit in the
> zram code?

Yeb, it's more desirable.
Thanks for the review!


> 
> Seth
> 
> >  #endif
> > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> > index 3b5be076268a..8ca51118cf2b 100644
> > --- a/mm/zsmalloc.c
> > +++ b/mm/zsmalloc.c
> > @@ -220,6 +220,7 @@ struct zs_pool {
> >  	gfp_t flags;	/* allocation flags used when growing pool */
> >  	unsigned long pages_allocated;
> >  	unsigned long max_pages_allocated;
> > +	unsigned long pages_limited;
> >  };
> >  
> >  /*
> > @@ -940,6 +941,11 @@ unsigned long zs_malloc(struct zs_pool *pool, size_t size)
> >  
> >  	if (!first_page) {
> >  		spin_unlock(&class->lock);
> > +
> > +		if (pool->pages_limited && (pool->pages_limited <
> > +			pool->pages_allocated + class->pages_per_zspage))
> > +			return 0;
> > +
> >  		first_page = alloc_zspage(class, pool->flags);
> >  		if (unlikely(!first_page))
> >  			return 0;
> > @@ -1132,6 +1138,24 @@ u64 zs_get_max_size_bytes(struct zs_pool *pool)
> >  }
> >  EXPORT_SYMBOL_GPL(zs_get_max_size_bytes);
> >  
> > +void zs_set_limit_size_bytes(struct zs_pool *pool, u64 limit)
> > +{
> > +	pool->pages_limited = round_down(limit, PAGE_SIZE) >> PAGE_SHIFT;
> > +}
> > +EXPORT_SYMBOL_GPL(zs_set_limit_size_bytes);
> > +
> > +u64 zs_get_limit_size_bytes(struct zs_pool *pool)
> > +{
> > +	u64 npages;
> > +
> > +	spin_lock(&pool->stat_lock);
> > +	npages = pool->pages_limited;
> > +	spin_unlock(&pool->stat_lock);
> > +	return npages << PAGE_SHIFT;
> > +
> > +}
> > +EXPORT_SYMBOL_GPL(zs_get_limit_size_bytes);
> > +
> >  module_init(zs_init);
> >  module_exit(zs_exit);
> >  
> > -- 
> > 2.0.0
> > 
> > -- 
> > 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>
> 
> --
> 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