| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140813233131.GE9227@bbox>
Date: Thu, 14 Aug 2014 08:31:31 +0900
From: Minchan Kim <minchan@...nel.org>
To: Seth Jennings <sjennings@...iantweb.net>
Cc: Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
linux-mm@...ck.org, Jerome Marchand <jmarchan@...hat.com>,
linux-kernel@...r.kernel.org, juno.choi@....com,
seungho1.park@....com, Luigi Semenzato <semenzato@...gle.com>,
Nitin Gupta <ngupta@...are.org>
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@...nel.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@...nel.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@...ck.org. For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@...ck.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>
--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists