Messages in this thread | | | Date | Thu, 14 Aug 2014 08:31:31 +0900 | From | Minchan Kim <> | Subject | Re: [RFC 3/3] zram: limit memory size for zram |
| |
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
| |