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@xxxxxxxxxxxxxxxxxx> 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
