lkml.org 
[lkml]   [2012]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/4] zsmalloc use zs_handle instead of void *
    On 5/10/12 10:02 AM, Konrad Rzeszutek Wilk wrote:
    > On Thu, May 10, 2012 at 11:03:19AM +0900, Minchan Kim wrote:
    >> On 05/10/2012 05:19 AM, Greg Kroah-Hartman wrote:
    >>
    >>> On Fri, May 04, 2012 at 11:24:54AM +0900, Minchan Kim wrote:
    >>>> On 05/04/2012 12:23 AM, Seth Jennings wrote:
    >>>>
    >>>>> On 05/03/2012 08:32 AM, Nitin Gupta wrote:
    >>>>>
    >>>>>> On 5/3/12 2:40 AM, Minchan Kim wrote:
    >>>>>>> We should use zs_handle instead of void * to avoid any
    >>>>>>> confusion. Without this, users may just treat zs_malloc return value as
    >>>>>>> a pointer and try to deference it.
    >>>>>>>
    >>>>>>> Cc: Dan Magenheimer<dan.magenheimer@oracle.com>
    >>>>>>> Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
    >>>>>>> Signed-off-by: Minchan Kim<minchan@kernel.org>
    >>>>>>> ---
    >>>>>>> drivers/staging/zcache/zcache-main.c | 8 ++++----
    >>>>>>> drivers/staging/zram/zram_drv.c | 8 ++++----
    >>>>>>> drivers/staging/zram/zram_drv.h | 2 +-
    >>>>>>> drivers/staging/zsmalloc/zsmalloc-main.c | 28
    >>>>>>> ++++++++++++++--------------
    >>>>>>> drivers/staging/zsmalloc/zsmalloc.h | 15 +++++++++++----
    >>>>>>> 5 files changed, 34 insertions(+), 27 deletions(-)
    >>>>>>
    >>>>>> This was a long pending change. Thanks!
    >>>>>
    >>>>>
    >>>>> The reason I hadn't done it before is that it introduces a checkpatch
    >>>>> warning:
    >>>>>
    >>>>> WARNING: do not add new typedefs
    >>>>> #303: FILE: drivers/staging/zsmalloc/zsmalloc.h:19:
    >>>>> +typedef void * zs_handle;
    >>>>>
    >>>>
    >>>>
    >>>> Yes. I did it but I think we are (a) of chapter 5: Typedefs in Documentation/CodingStyle.
    >>>>
    >>>> (a) totally opaque objects (where the typedef is actively used to _hide_
    >>>> what the object is).
    >>>>
    >>>> No?
    >>>
    >>> No.
    >>>
    >>> Don't add new typedefs to the kernel. Just use a structure if you need
    >>> to.
    >>
    >>
    >> I tried it but failed because there were already tightly coupling between [zcache|zram]
    >> and zsmalloc.
    >> They already knows handle's internal well so they used it as pointer, even zcache keeps
    >> handle's value as some key in tmem_put and tmem_get
    >> AFAIK, ramster also will use zsmalloc sooner or later and add more coupling codes. Sigh.
    >> Please fix it as soon as possible.
    >>
    >> Dan, Seth
    >> Any ideas?
    >
    > struct zs {
    > void *ptr;
    > };
    >
    > And pass that structure around?
    >

    A minor problem is that we store this handle value in a radix tree node.
    If we wrap it as a struct, then we will not be able to store it directly
    in the node -- the node will have to point to a 'struct zs'. This will
    unnecessarily waste sizeof(void *) for every object stored.

    We could 'memcpy' struct zs to a void * and then store that directly in
    the radix node but not sure if that would be less ugly than just
    returning the handle as a void * as is done currently.

    Thanks,
    Nitin


    \
     
     \ /
      Last update: 2012-05-10 17:21    [W:0.050 / U:0.772 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site