Messages in this thread Patch in this message | | | Date | Tue, 3 May 2016 14:23:24 +0900 | From | Minchan Kim <> | 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@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 >
| |