| 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: <20160503052324.GB17316@bbox>
Date: Tue, 3 May 2016 14:23:24 +0900
From: Minchan Kim <minchan@...nel.org>
To: Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] zram: user per-cpu compression streams
On Mon, May 02, 2016 at 05:06:00PM +0900, Sergey Senozhatsky wrote:
> On (05/02/16 16:25), Sergey Senozhatsky wrote:
> [..]
> > > Trivial:
> > > We could remove max_strm now and change description.
> >
> > oh, yes.
>
> how about something like this? remove max_comp_streams entirely, but
> leave the attr. if we keep zram->max_comp_streams and return its value
> (set by user space) from show() handler, we are techically lying;
> because the actual number of streams is now num_online_cpus().
Yes, we should have limit the value to num_online_cpus from the
beginning.
>
>
> ===8<===8<===
>
> From: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
> Subject: [PATCH] zram: remove max_comp_streams internals
>
> Remove the internal part of max_comp_streams interface, since we
> switched to per-cpu streams. We will keep RW max_comp_streams attr
> around, because:
>
> a) we may (silently) switch back to idle compression streams list
> and don't want to disturb user space
> b) max_comp_streams attr must wait for the next 'lay off cycle';
> we give user space 2 years to adjust before we remove/downgrade
> the attr, and there are already several attrs scheduled for
> removal in 4.11, so it's too late for max_comp_streams.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
> ---
> drivers/block/zram/zcomp.c | 7 +------
> drivers/block/zram/zcomp.h | 2 +-
> drivers/block/zram/zram_drv.c | 47 +++++++++++--------------------------------
> drivers/block/zram/zram_drv.h | 1 -
> 4 files changed, 14 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index d4159e4..d4de9cb 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -95,11 +95,6 @@ bool zcomp_available_algorithm(const char *comp)
> return find_backend(comp) != NULL;
> }
>
> -bool zcomp_set_max_streams(struct zcomp *comp, int num_strm)
> -{
> - return true;
> -}
> -
> struct zcomp_strm *zcomp_strm_find(struct zcomp *comp)
> {
> return *get_cpu_ptr(comp->stream);
> @@ -211,7 +206,7 @@ void zcomp_destroy(struct zcomp *comp)
> * case of allocation error, or any other error potentially
> * returned by functions zcomp_strm_{multi,single}_create.
> */
> -struct zcomp *zcomp_create(const char *compress, int max_strm)
> +struct zcomp *zcomp_create(const char *compress)
> {
> struct zcomp *comp;
> struct zcomp_backend *backend;
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index aba8c21..ffd88cb 100644
> --- a/drivers/block/zram/zcomp.h
> +++ b/drivers/block/zram/zcomp.h
> @@ -45,7 +45,7 @@ struct zcomp {
> ssize_t zcomp_available_show(const char *comp, char *buf);
> bool zcomp_available_algorithm(const char *comp);
>
> -struct zcomp *zcomp_create(const char *comp, int max_strm);
> +struct zcomp *zcomp_create(const char *comp);
> void zcomp_destroy(struct zcomp *comp);
>
> struct zcomp_strm *zcomp_strm_find(struct zcomp *comp);
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index cad1751..817e511 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -304,46 +304,25 @@ static ssize_t mem_used_max_store(struct device *dev,
> return len;
> }
>
> +/*
> + * We switched to per-cpu streams and this attr is not needed anymore.
> + * However, we will keep it around for some time, because:
> + * a) we may revert per-cpu streams in the future
> + * b) it's visible to user space and we need to follow our 2 years
> + * retirement rule; but we already have a number of 'soon to be
> + * altered' attrs, so max_comp_streams need to wait for the next
> + * layoff cycle.
> + */
Thanks for nice comment.
> static ssize_t max_comp_streams_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - int val;
> - struct zram *zram = dev_to_zram(dev);
> -
> - down_read(&zram->init_lock);
> - val = zram->max_comp_streams;
> - up_read(&zram->init_lock);
> -
> - return scnprintf(buf, PAGE_SIZE, "%d\n", val);
> + return scnprintf(buf, PAGE_SIZE, "%d\n", num_online_cpus());
> }
>
> static ssize_t max_comp_streams_store(struct device *dev,
> struct device_attribute *attr, const char *buf, size_t len)
> {
> - int num;
> - struct zram *zram = dev_to_zram(dev);
> - int ret;
> -
> - ret = kstrtoint(buf, 0, &num);
> - if (ret < 0)
> - return ret;
> - if (num < 1)
> - return -EINVAL;
> -
> - down_write(&zram->init_lock);
> - if (init_done(zram)) {
> - if (!zcomp_set_max_streams(zram->comp, num)) {
> - pr_info("Cannot change max compression streams\n");
> - ret = -EINVAL;
> - goto out;
> - }
> - }
> -
> - zram->max_comp_streams = num;
> - ret = len;
> -out:
> - up_write(&zram->init_lock);
> - return ret;
At least, we need sanity check code, still?
Otherwise, user can echo "garbage" > /sys/xxx/max_comp_stream" and then
cat /sys/xxx/max_comp_stream returns num_online_cpus.
> + return len;
> }
>
> static ssize_t comp_algorithm_show(struct device *dev,
> @@ -1035,7 +1014,6 @@ static void zram_reset_device(struct zram *zram)
> /* Reset stats */
> memset(&zram->stats, 0, sizeof(zram->stats));
> zram->disksize = 0;
> - zram->max_comp_streams = 1;
>
> set_capacity(zram->disk, 0);
> part_stat_set_all(&zram->disk->part0, 0);
> @@ -1064,7 +1042,7 @@ static ssize_t disksize_store(struct device *dev,
> if (!meta)
> return -ENOMEM;
>
> - comp = zcomp_create(zram->compressor, zram->max_comp_streams);
> + comp = zcomp_create(zram->compressor);
> if (IS_ERR(comp)) {
> pr_err("Cannot initialise %s compressing backend\n",
> zram->compressor);
> @@ -1299,7 +1277,6 @@ static int zram_add(void)
> }
> strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
> zram->meta = NULL;
> - zram->max_comp_streams = 1;
>
> pr_info("Added device: %s\n", zram->disk->disk_name);
> return device_id;
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 8e92339..06b1636 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -102,7 +102,6 @@ struct zram {
> * the number of pages zram can consume for storing compressed data
> */
> unsigned long limit_pages;
> - int max_comp_streams;
>
> struct zram_stats stats;
> atomic_t refcount; /* refcount for zram_meta */
> --
> 2.8.2
>
Powered by blists - more mailing lists