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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx"> email@xxxxxxxxx </a>