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

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

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.