Re: [PATCHv2 2/2] zram: drop max_zpage_size and use zs_huge_class_size()

[Minchan Kim]

On Tue, Mar 13, 2018 at 11:18:13PM +0900, Sergey Senozhatsky wrote:
> On (03/13/18 22:58), Minchan Kim wrote:
> > > > If it is static, we can do this in zram_init? I believe it's more readable in that
> > > > it's never changed betweens zram instances.
> > > 
> > > We need to have at least one pool, because pool decides where the
> > > watermark is. At zram_init() stage we don't have a pool yet. We
> > > zs_create_pool() in zram_meta_alloc() so that's why I put
> > > zs_huge_class_size() there. I'm not in love with it, but that's
> > > the only place where we can have it.
> > 
> > Fair enough. Then what happens if client calls zs_huge_class_size
> > without creating zs_create_pool?
> 
> Will receive 0.
> One of the version was returning SIZE_MAX in such case.
> 
> size_t zs_huge_class_size(void)
>  {
> +	if (unlikely(!huge_class_size))
> +		return SIZE_MAX;
>  	return huge_class_size;
>  }

I really don't want to have such API which returns different size on
different context.

The thing is we need to create pool first to get right value.
It means zs_huge_class_size should depend on zs_create_pool.
So, I think passing zs_pool to zs_huge_class_size is right way to
prevent such misuse and confusing. Yub, franky speaknig, zsmalloc
is not popular like slab allocator  or page allocator so this like
discussion would be waste. However, we don't need big effort to do
and this is review phase so I want to make more robust/readable. ;-)

> 
> > I think we should make zs_huge_class_size has a zs_pool as argument.
> 
> Can do, but the param will be unused. May be we can do something

Yub, param wouldn't be unused but it's the way of creating dependency
intentionally. It could make code more robust/readable.

Please, let's pass zs_pool and returns always right huge size.

> like below instead:
> 
>  size_t zs_huge_class_size(void)
>  {
> +	if (unlikely(!huge_class_size))
> +		return 3 * PAGE_SIZE / 4;
>  	return huge_class_size;
>  }
> 
> Should do no harm (unless I'm missing something).


> 
> 	-ss