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

[Minchan Kim]

On Tue, May 03, 2016 at 02:23:24PM +0900, Minchan Kim wrote:
> 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@xxxxxxxxx>
> > 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@xxxxxxxxx>
> > ---
> >  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.

One more thing,

User:
echo 4 > /sys/xxx/max_comp_stream"
cat /sys/xxx/max_comp_streams
8

which is rather weird?

We should keep user's value and return it to user although it's techically
lying. IMO, it would be best way to prevent confusing for user until we
removes max_comp_streams finally.