Re: [PATCHv2 3/6] zsmalloc: fine-grained inuse ratio based fullness grouping

[Minchan Kim <minchan@kernel.org>]

On Fri, Mar 03, 2023 at 10:06:43AM +0900, Sergey Senozhatsky wrote:
> On (23/03/02 16:20), Minchan Kim wrote:
> > On Thu, Mar 02, 2023 at 09:53:03AM +0900, Sergey Senozhatsky wrote:
> > > On (23/03/01 16:28), Minchan Kim wrote:
> > > > On Wed, Mar 01, 2023 at 05:55:44PM +0900, Sergey Senozhatsky wrote:
> > > > > On (23/02/28 14:53), Minchan Kim wrote:
> > > > > > BTW, I still prefer the enum instead of 10 define.
> > > > > > 
> > > > > > enum fullness_group {
> > > > > >     ZS_EMPTY,
> > > > > >     ZS_INUSE_RATIO_MIN,
> > > > > >     ZS_INUSE_RATIO_ALMOST_FULL = 7,
> > > > > >     ZS_INUSE_RATIO_MAX = 10,
> > > > > >     ZS_FULL,
> > > > > >     NR_ZS_FULLNESS,
> > > > > > }
> > > > > 
> > > > > For educational purposes, may I ask what do enums give us? We
> > > > > always use integers - int:4 in zspage fullness, int for arrays
> > > > > offsets and we cast to plain integers in get/set stats. So those
> > > > > enums exist only at declaration point, and plain int otherwise.
> > > > > What are the benefits over #defines?
> > > > 
> > > > Well, I just didn't like the 12 hard coded define *list* values
> > > > and never used other places except zs_stats_size_show since
> > > 
> > > If we have two enums, then we need more lines
> > > 
> > > enum fullness {
> > > 	ZS_INUSE_RATIO_0
> > > 	...
> > > 	ZS_INUSE_RATIO_100
> > > }
> > > 
> > > enum stats {
> > > 	INUSE_RATIO_0
> > > 	...
> > > 	INUSE_RATIO_100
> > > 
> > > 	// the rest of stats
> > > }
> > > 
> > > and then we use int:4 fullness value to access stats.
> > 
> > Yeah. I don't see any problem unless I miss your point.
> 
> OK. How about having one enum? E.g. "zs_flags" or something which
> will contain all our constants?
> 
> Otherwise I can create two big enums for fullness and stats.

Let's go with two enums at this moment since your great work is not
tied into the problem. If that becomes really maintaince hole,
we could tidy it up at that time.

> What's your preference on inuse_0 and inuse_100 naming? Do we
> keep unified naming or should it be INUSE_MIN/INUSE_MAX or
> EMPTY/FULL?

I don't have strong opinion about it. I will follow your choice. ;-)

> 
> > > For per inuse ratio zs_stats_size_show() we need to access stats
> > > individually:
> > > 
> > > 	inuse10, inuse20, inuse30, ... inuse99
> > 
> > Does it need specific index in the enum list?
> 
> If we report per inuse group then yes:
> 
> 	sprintf("... %lu %lu ..... %lu %lu ...\n",
> 		...
> 		get_stat(ZS_INUSE_RATIO_10),
> 		get_stat(ZS_INUSE_RATIO_20),
> 		get_stat(ZS_INUSE_RATIO_30),
> 		...
> 		get_stat(ZS_INUSE_RATIO_99),
> 		...);

I thought we could handle it with loop

prologue - seq_printf
for (ratio = min, ratio < max; ratio++ )
    seq_printf(s, "%lu", get_stat(ratio)
epilogue - seq_printf
seq_puts(s, "\n");