Re: [PATCHv4 2/7] zsmalloc: promote to lib/

[Minchan Kim <minchan@kernel.org>]

On Wed, Jan 30, 2013 at 10:28:41AM -0600, Seth Jennings wrote:
> On 01/29/2013 04:51 PM, Andrew Morton wrote:
> > On Tue, 29 Jan 2013 15:40:22 -0600
> > Seth Jennings <sjenning@linux.vnet.ibm.com> wrote:
> > 
> >> This patch promotes the slab-based zsmalloc memory allocator
> >> from the staging tree to lib/
> > 
> > Hate to rain on the parade, but...  we haven't reviewed zsmalloc
> > yet.  At least, I haven't, and I haven't seen others do so.
> > 
> > So how's about we forget that zsmalloc was previously in staging and
> > send the zsmalloc code out for review?  With a very good changelog
> > explaining why it exists, what problems it solves, etc.
> > 
> > 
> > I peeked.
> > 
> > Don't answer any of the below questions - they are examples of
> > concepts which should be accessible to readers of the
> > hopefully-forthcoming very-good-changelog.
> > 
> > - kmap_atomic() returns a void* - there's no need to cast its return value.
> > 
> > - Remove private MAX(), use the (much better implemented) max().
> > 
> > - It says "This was one of the major issues with its predecessor
> >   (xvmalloc)", but drivers/staging/ramster/xvmalloc.c is still in the tree.
> > 
> > - USE_PGTABLE_MAPPING should be done via Kconfig.
> > 
> > - USE_PGTABLE_MAPPING is interesting and the changelog should go into
> >   some details.  What are the pros and cons here?  Why do the two
> >   options exist?  Can we eliminate one mode or the other?
> > 
> > - Various functions are obscure and would benefit from explanatory
> >   comments.  Good comments explain "why it exists", more than "what it
> >   does".
> > 
> >   These include get_size_class_index, get_fullness_group,
> >   insert_zspage, remove_zspage, fix_fullness_group.
> > 
> >   Also a description of this handle encoding thing - what do these
> >   "handles" refer to?  Why is stuff being encoded into them and how?
> > 
> > - I don't understand how the whole thing works :( If I allocate a
> >   16 kbyte object with zs_malloc(), what do I get?  16k of
> >   contiguous memory?  How can it do that if
> >   USE_PGTABLE_MAPPING=false?  Obviously it can't so it's doing
> >   something else.  But what?
> > 
> > - What does zs_create_pool() do and how do I use it?  It appears
> >   to create a pool of all possible object sizes.  But why do we need
> >   more than one such pool kernel-wide?
> > 
> > - I tried to work out the actual value of ZS_SIZE_CLASSES but it
> >   made my head spin.
> > 
> > - We really really don't want to merge zsmalloc!  It would be far
> >   better to use an existing allocator (perhaps after modifying it)
> >   than to add yet another new one.  The really-good-changelog should
> >   be compelling on this point, please.
> > 
> > See, I (and I assume others) are totally on first base here and we need
> > to get through this before we can get onto zswap.  Sorry. 
> > drivers/staging is where code goes to be ignored :(
> 
> I've noticed :-/
> 
> Thank you very much for your review!  I'll work with Nitin and Minchan
> to beef up the documentation so that the answers to your questions are
> more readily apparent in the code/comments.

Actually, Kconfig of USE_PGTABLE_MAPPING is my plan.
Will do it if anyone has no objection.

-- 
Kind regards,
Minchan Kim