Re: [PATCH 2/2] zram: user per-cpu compression streams

[Minchan Kim <minchan@kernel.org>]

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@gmail.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@gmail.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
>