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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130131053554.GD23548@blaptop>
Date:	Thu, 31 Jan 2013 14:35:54 +0900
From:	Minchan Kim <minchan@...nel.org>
To:	Seth Jennings <sjenning@...ux.vnet.ibm.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Nitin Gupta <ngupta@...are.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Dan Magenheimer <dan.magenheimer@...cle.com>,
	Robert Jennings <rcj@...ux.vnet.ibm.com>,
	Jenifer Hopper <jhopper@...ibm.com>,
	Mel Gorman <mgorman@...e.de>,
	Johannes Weiner <jweiner@...hat.com>,
	Rik van Riel <riel@...hat.com>,
	Larry Woodman <lwoodman@...hat.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Dave Hansen <dave@...ux.vnet.ibm.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org
Subject: Re: [PATCHv4 2/7] zsmalloc: promote to lib/

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@...ux.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/