[lkml]   [2013]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCHv4 2/7] zsmalloc: promote to lib/
    On Tue, 29 Jan 2013 15:40:22 -0600
    Seth Jennings <> 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

    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 :(

     \ /
      Last update: 2013-01-30 00:22    [W:0.027 / U:29.420 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site