[PATCH v1] zram: add size class equals check into recompression

[PATCH v1] zram: add size class equals check into recompression

[Alexey Romanov]

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(+)

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);
-- 
2.25.1

Re: [PATCH v1] zram: add size class equals check into recompression

[Andrew Morton]

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 >=?

Re: [PATCH v1] zram: add size class equals check into recompression

[Sergey Senozhatsky]

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.

Re: [PATCH v1] zram: add size class equals check into recompression

[Sergey Senozhatsky]

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.

Re: [PATCH v1] zram: add size class equals check into recompression

[Sergey Senozhatsky]

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.

Re: [PATCH v1] zram: add size class equals check into recompression

[Aleksey Romanov]

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.

-- 
Thank you,
Alexey

Re: [PATCH v1] zram: add size class equals check into recompression

[Sergey Senozhatsky]

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.

Re: [PATCH v1] zram: add size class equals check into recompression

[Sergey Senozhatsky]

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;
}

Re: [PATCH v1] zram: add size class equals check into recompression

[Aleksey Romanov]

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?

-- 
Thank you,
Alexey

Re: [PATCH v1] zram: add size class equals check into recompression

[Sergey Senozhatsky]

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.

Re: [PATCH v1] zram: add size class equals check into recompression

[Aleksey Romanov]

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.

-- 
Thank you,
Alexey