| Message ID | 20221024120942.13885-1-avromanov@sberdevices.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] zram: add size class equals check into recompression | expand |
On Mon, 24 Oct 2022 15:09:42 +0300 Alexey Romanov <avromanov@sberdevices.ru> wrote: > It makes no sense for us to recompress the object if it will > be in the same size class. We anyway don't get any memory gain. > But, at the same time, we get a CPU time overhead when inserting > this object into zspage and decompressing it afterwards. > Dumb question: is it ever possible for compression to result in an increase in size? > + class_size_next == class_size_prev || IOW, could this be >=?
On (22/10/24 13:59), Andrew Morton wrote: > > It makes no sense for us to recompress the object if it will > > be in the same size class. We anyway don't get any memory gain. > > But, at the same time, we get a CPU time overhead when inserting > > this object into zspage and decompressing it afterwards. > > > > Dumb question: is it ever possible for compression to result in an > increase in size? That's a good question. Re-compressed object can be bigger than the original compressed one, but this should already be taken care of. We do if (comp_len_next >= huge_class_size || comp_len_next >= comp_len_prev || This checks whether recompressed object is above huge-size watermark and whether recompressed size is larger than the original size.
On (22/10/24 15:09), Alexey Romanov wrote: > It makes no sense for us to recompress the object if it will > be in the same size class. We anyway don't get any memory gain. > But, at the same time, we get a CPU time overhead when inserting > this object into zspage and decompressing it afterwards. Sounds reasonable. In my synthetic recompression test I saw only 5 objects that landed in the same class after recompression; but this, as always, depends on data patterns and compression algorithms being used. [..] > + class_size_prev = zs_get_class_size(zram->mem_pool, comp_len_prev); > + class_size_next = zs_get_class_size(zram->mem_pool, comp_len_next); > /* > * Either a compression error or we failed to compressed the object > * in a way that will save us memory. Mark the object so that we > @@ -1663,6 +1667,7 @@ static int zram_recompress(struct zram *zram, u32 index, struct page *page, > */ > if (comp_len_next >= huge_class_size || > comp_len_next >= comp_len_prev || > + class_size_next == class_size_prev || Let's use >= here, what Andrew has suggested. > ret) { > zram_set_flag(zram, index, ZRAM_RECOMP_SKIP); > zram_clear_flag(zram, index, ZRAM_IDLE); > diff --git a/include/linux/zsmalloc.h b/include/linux/zsmalloc.h > index 2a430e713ce5..75dcbafd5f36 100644 > --- a/include/linux/zsmalloc.h > +++ b/include/linux/zsmalloc.h > @@ -56,4 +56,6 @@ unsigned long zs_get_total_pages(struct zs_pool *pool); > unsigned long zs_compact(struct zs_pool *pool); [..] > +/** > + * zs_get_class_size() - Returns the size (in bytes) of the > + * zsmalloc &size_class into which the object with specified > + * size will be inserted or already inserted. > + * > + * @pool: zsmalloc pool to use > + * > + * Context: Any context. > + * > + * Return: the size (in bytes) of the zsmalloc &size_class into which > + * the object with specified size will be inserted. > + */ Can't think of a btter way of doing it. On one hand we probably don't want to expose the object size to class size mapping outside of zsmalloc, but on the other hand we sort of already do so: zs_huge_class_size(). > +unsigned int zs_get_class_size(struct zs_pool *pool, unsigned int size) > +{ > + struct size_class *class = pool->size_class[get_size_class_index(size)]; > + > + return class->size; > +} > +EXPORT_SYMBOL_GPL(zs_get_class_size); I'll kindly ask for v2. This conflicts with configurable zspage order patch set which I posted last night. get_size_class_index() now takes the pool parameter.
On (22/10/25 10:53), Sergey Senozhatsky wrote: > > +unsigned int zs_get_class_size(struct zs_pool *pool, unsigned int size) > > +{ > > + struct size_class *class = pool->size_class[get_size_class_index(size)]; > > + > > + return class->size; > > +} > > +EXPORT_SYMBOL_GPL(zs_get_class_size); > > I'll kindly ask for v2. This conflicts with configurable zspage order > patch set which I posted last night. get_size_class_index() now takes > the pool parameter. Oh, apparently Andrew already has a fixup patch for this.
On Tue, Oct 25, 2022 at 11:04:40AM +0900, Sergey Senozhatsky wrote: > On (22/10/25 10:53), Sergey Senozhatsky wrote: > > > +unsigned int zs_get_class_size(struct zs_pool *pool, unsigned int size) > > > +{ > > > + struct size_class *class = pool->size_class[get_size_class_index(size)]; > > > + > > > + return class->size; > > > +} > > > +EXPORT_SYMBOL_GPL(zs_get_class_size); > > > > I'll kindly ask for v2. This conflicts with configurable zspage order > > patch set which I posted last night. get_size_class_index() now takes > > the pool parameter. > > Oh, apparently Andrew already has a fixup patch for this. Hi! Thanks for the quick response. Do I need to submit v2 with a fix for >=? Also, I forgot to correct the comment on the zs_get_class_size() function: > * Return: the size (in bytes) of the zsmalloc &size_class into which > * the object with specified size will be inserted. ... or already inserted.
On (22/10/25 09:49), Aleksey Romanov wrote: > > Oh, apparently Andrew already has a fixup patch for this. > > Hi! Thanks for the quick response. > > Do I need to submit v2 with a fix for >=? Also, I forgot to > correct the comment on the zs_get_class_size() function: I will cherry-pick you patch and send it to Andrew with my rebased series that this patch conflicts with. > > * Return: the size (in bytes) of the zsmalloc &size_class into which > > * the object with specified size will be inserted. > > ... or already inserted. Will correct that.
On (22/10/25 09:49), Aleksey Romanov wrote: > On Tue, Oct 25, 2022 at 11:04:40AM +0900, Sergey Senozhatsky wrote: > > On (22/10/25 10:53), Sergey Senozhatsky wrote: > > > > +unsigned int zs_get_class_size(struct zs_pool *pool, unsigned int size) > > > > +{ > > > > + struct size_class *class = pool->size_class[get_size_class_index(size)]; > > > > + > > > > + return class->size; > > > > +} > > > > +EXPORT_SYMBOL_GPL(zs_get_class_size); I wonder if we want to return class->index instead of class->size? Something like this (a sketch) Return: the index of the zsmalloc &size_class that hold objects of the provided size. unsigned int zs_lookup_class_index(struct zs_pool *pool, unsigned int size) { struct size_class *class = pool->size_class[get_size_class_index(size)]; return class->index; }
On Tue, Oct 25, 2022 at 07:08:54PM +0900, Sergey Senozhatsky wrote: > On (22/10/25 09:49), Aleksey Romanov wrote: > > On Tue, Oct 25, 2022 at 11:04:40AM +0900, Sergey Senozhatsky wrote: > > > On (22/10/25 10:53), Sergey Senozhatsky wrote: > > > > > +unsigned int zs_get_class_size(struct zs_pool *pool, unsigned int size) > > > > > +{ > > > > > + struct size_class *class = pool->size_class[get_size_class_index(size)]; > > > > > + > > > > > + return class->size; > > > > > +} > > > > > +EXPORT_SYMBOL_GPL(zs_get_class_size); > > I wonder if we want to return class->index instead of class->size? > > Something like this (a sketch) > > Return: the index of the zsmalloc &size_class that hold objects of the > provided size. > > unsigned int zs_lookup_class_index(struct zs_pool *pool, unsigned int size) > { > struct size_class *class = pool->size_class[get_size_class_index(size)]; > > return class->index; > } Sure. I think it would be more logical, and perhaps such a function would be more applicable in other cases, in the future. Will you fix it in your cherry-pick?
On (22/10/25 11:55), Aleksey Romanov wrote: > > Return: the index of the zsmalloc &size_class that hold objects of the > > provided size. > > > > unsigned int zs_lookup_class_index(struct zs_pool *pool, unsigned int size) > > { > > struct size_class *class = pool->size_class[get_size_class_index(size)]; > > > > return class->index; > > } > > Sure. I think it would be more logical, and perhaps such a function > would be more applicable in other cases, in the future. > > Will you fix it in your cherry-pick? For that I probably will ask you to send out v2, if possible.
On Tue, Oct 25, 2022 at 09:38:04PM +0900, Sergey Senozhatsky wrote: > On (22/10/25 11:55), Aleksey Romanov wrote: > > > Return: the index of the zsmalloc &size_class that hold objects of the > > > provided size. > > > > > > unsigned int zs_lookup_class_index(struct zs_pool *pool, unsigned int size) > > > { > > > struct size_class *class = pool->size_class[get_size_class_index(size)]; > > > > > > return class->index; > > > } > > > > Sure. I think it would be more logical, and perhaps such a function > > would be more applicable in other cases, in the future. > > > > Will you fix it in your cherry-pick? > > For that I probably will ask you to send out v2, if possible. Okay, I will send v2 soon.
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 364323713393..bf610cf6a09c 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -1632,6 +1632,8 @@ static int zram_recompress(struct zram *zram, u32 index, struct page *page, unsigned long handle_next; unsigned int comp_len_next; unsigned int comp_len_prev; + unsigned int class_size_next; + unsigned int class_size_prev; struct zcomp_strm *zstrm; void *src, *dst; int ret; @@ -1656,6 +1658,8 @@ static int zram_recompress(struct zram *zram, u32 index, struct page *page, ret = zcomp_compress(zstrm, src, &comp_len_next); kunmap_atomic(src); + class_size_prev = zs_get_class_size(zram->mem_pool, comp_len_prev); + class_size_next = zs_get_class_size(zram->mem_pool, comp_len_next); /* * Either a compression error or we failed to compressed the object * in a way that will save us memory. Mark the object so that we @@ -1663,6 +1667,7 @@ static int zram_recompress(struct zram *zram, u32 index, struct page *page, */ if (comp_len_next >= huge_class_size || comp_len_next >= comp_len_prev || + class_size_next == class_size_prev || ret) { zram_set_flag(zram, index, ZRAM_RECOMP_SKIP); zram_clear_flag(zram, index, ZRAM_IDLE); diff --git a/include/linux/zsmalloc.h b/include/linux/zsmalloc.h index 2a430e713ce5..75dcbafd5f36 100644 --- a/include/linux/zsmalloc.h +++ b/include/linux/zsmalloc.h @@ -56,4 +56,6 @@ unsigned long zs_get_total_pages(struct zs_pool *pool); unsigned long zs_compact(struct zs_pool *pool); void zs_pool_stats(struct zs_pool *pool, struct zs_pool_stats *stats); + +unsigned int zs_get_class_size(struct zs_pool *pool, unsigned int size); #endif diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c index d03941cace2c..148451385445 100644 --- a/mm/zsmalloc.c +++ b/mm/zsmalloc.c @@ -1205,6 +1205,26 @@ static bool zspage_full(struct size_class *class, struct zspage *zspage) return get_zspage_inuse(zspage) == class->objs_per_zspage; } +/** + * zs_get_class_size() - Returns the size (in bytes) of the + * zsmalloc &size_class into which the object with specified + * size will be inserted or already inserted. + * + * @pool: zsmalloc pool to use + * + * Context: Any context. + * + * Return: the size (in bytes) of the zsmalloc &size_class into which + * the object with specified size will be inserted. + */ +unsigned int zs_get_class_size(struct zs_pool *pool, unsigned int size) +{ + struct size_class *class = pool->size_class[get_size_class_index(size)]; + + return class->size; +} +EXPORT_SYMBOL_GPL(zs_get_class_size); + unsigned long zs_get_total_pages(struct zs_pool *pool) { return atomic_long_read(&pool->pages_allocated);
It makes no sense for us to recompress the object if it will be in the same size class. We anyway don't get any memory gain. But, at the same time, we get a CPU time overhead when inserting this object into zspage and decompressing it afterwards. Signed-off-by: Alexey Romanov <avromanov@sberdevices.ru> --- drivers/block/zram/zram_drv.c | 5 +++++ include/linux/zsmalloc.h | 2 ++ mm/zsmalloc.c | 20 ++++++++++++++++++++ 3 files changed, 27 insertions(+)