Re: [PATCH v5 08/12] zsmalloc: introduce zspage structure

[Minchan Kim]

On Mon, May 16, 2016 at 12:09:41PM +0900, Sergey Senozhatsky wrote:
> On (05/09/16 11:20), Minchan Kim wrote:
> > We have squeezed meta data of zspage into first page's descriptor.
> > So, to get meta data from subpage, we should get first page first
> > of all. But it makes trouble to implment page migration feature
> > of zsmalloc because any place where to get first page from subpage
> > can be raced with first page migration. IOW, first page it got
> > could be stale. For preventing it, I have tried several approahces
> > but it made code complicated so finally, I concluded to separate
> > metadata from first page. Of course, it consumes more memory. IOW,
> > 16bytes per zspage on 32bit at the moment. It means we lost 1%
> > at *worst case*(40B/4096B) which is not bad I think at the cost of
> > maintenance.
> > 
> > Cc: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> > Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
> [..]
> > @@ -153,8 +138,6 @@
> >  enum fullness_group {
> >  	ZS_ALMOST_FULL,
> >  	ZS_ALMOST_EMPTY,
> > -	_ZS_NR_FULLNESS_GROUPS,
> > -
> >  	ZS_EMPTY,
> >  	ZS_FULL
> >  };
> > @@ -203,7 +186,7 @@ static const int fullness_threshold_frac = 4;
> >  
> >  struct size_class {
> >  	spinlock_t lock;
> > -	struct page *fullness_list[_ZS_NR_FULLNESS_GROUPS];
> > +	struct list_head fullness_list[2];
> 
> seems that it also has some cleaup bits in it.
> 
> [..]
> > -static int create_handle_cache(struct zs_pool *pool)
> > +static int create_cache(struct zs_pool *pool)
> >  {
> >  	pool->handle_cachep = kmem_cache_create("zs_handle", ZS_HANDLE_SIZE,
> >  					0, 0, NULL);
> > -	return pool->handle_cachep ? 0 : 1;
> > +	if (!pool->handle_cachep)
> > +		return 1;
> > +
> > +	pool->zspage_cachep = kmem_cache_create("zspage", sizeof(struct zspage),
> > +					0, 0, NULL);
> > +	if (!pool->zspage_cachep) {
> > +		kmem_cache_destroy(pool->handle_cachep);
> 		^^^^^
> 
> do you need to NULL a pool->handle_cachep here?

Thanks, Will fix.

> 
> zs_create_pool()
> 	if (create_cache() == 1) {
> 			pool->zspage_cachep NULL
> 			pool->handle_cachep !NULL   already freed -> kmem_cache_destroy()
> 			return 1;
> 		goto err
> 	}
> err:
> 	zs_destroy_pool()
> 		destroy_cache() {
> 			kmem_cache_destroy(pool->handle_cachep);  !NULL and freed
> 			kmem_cache_destroy(pool->zspage_cachep);  NULL ok
> 		}
> 
> 
> can we also switch create_cache() to errnos? I just like a bit
> better
> 		return -ENOMEM;
> 	else
> 		return 0;
> 
> than
> 
> 		return 1;
> 	else
> 		return 0;
> 

Hmm, of course, I can do it easily.
But zs_create_pool returns NULL without error propagation from sub
functions so I don't see any gain from returning errno from
create_cache. I don't mean I hate it but just need a justificaion
to persuade grumpy me.

> 
> > @@ -997,44 +951,38 @@ static void init_zspage(struct size_class *class, struct page *first_page)
> >  		off %= PAGE_SIZE;
> >  	}
> >  
> > -	set_freeobj(first_page, (unsigned long)location_to_obj(first_page, 0));
> > +	set_freeobj(zspage,
> > +		(unsigned long)location_to_obj(zspage->first_page, 0));
> 
> 	static unsigned long location_to_obj()
> 
> it's already returning "(unsigned long)", so here and in several other places
> this cast can be dropped.

Yeb.

> 
> [..]
> > +static struct zspage *isolate_zspage(struct size_class *class, bool source)
> >  {
> > +	struct zspage *zspage;
> > +	enum fullness_group fg[2] = {ZS_ALMOST_EMPTY, ZS_ALMOST_FULL};
> > +	if (!source) {
> > +		fg[0] = ZS_ALMOST_FULL;
> > +		fg[1] = ZS_ALMOST_EMPTY;
> > +	}
> > +
> > +	for (i = 0; i < 2; i++) {
> 
> sorry, why not "for (i = ZS_ALMOST_EMPTY; i <= ZS_ALMOST_FULL ..." ?

For source zspage, the policy is to find a fragment object from ZS_ALMOST_EMPTY.
For target zspage, the policy is to find a fragment object from ZS_ALMOST_FULL.

Do I misunderstand your question?

> 
> > +		zspage = list_first_entry_or_null(&class->fullness_list[fg[i]],
> > +							struct zspage, list);
> > +		if (zspage) {
> > +			remove_zspage(class, zspage, fg[i]);
> > +			return zspage;
> >  		}
> >  	}
> >  
> > -	return page;
> > +	return zspage;
> >  }
> 
> 	-ss