lkml.org 
[lkml]   [2009]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/3] xvmalloc memory allocator
    Marcin Slusarz wrote:
    > Nitin Gupta wrote:
    >> +static void stat_inc(u64 *value)
    >> +{
    >> + (*value)++;
    >> +}
    >> +
    >> +static void stat_dec(u64 *value)
    >> +{
    >> + (*value)--;
    >> +}
    >
    > Why not open code it?
    >


    This looks nicer, doesn't it?


    >> +
    >> +static u32 test_flag(struct block_header *block, enum blockflags flag)
    >> +{
    >> + return block->prev & (1 << flag);
    >> +}
    >> +
    >> +static void set_flag(struct block_header *block, enum blockflags flag)
    >> +{
    >> + block->prev |= (1 << flag);
    >> +}
    >> +
    >> +static void clear_flag(struct block_header *block, enum blockflags flag)
    >> +{
    >> + block->prev &= ~(1 << flag);
    >> +}
    >
    > Why don't you use test_bit / __set_bit / __clear_bit directly?
    >

    block->prev is 16 bits. __set_bit() etc. need unsigned long pointer.

    >> +struct xv_pool *xv_create_pool(void)
    >> +{
    >> + int i;
    >> + u32 ovhd_size;
    >> + struct xv_pool *pool;
    >> +
    >> + ovhd_size = roundup(sizeof(*pool), PAGE_SIZE);
    >> + pool = kmalloc(ovhd_size, GFP_KERNEL);
    >> + if (!pool)
    >> + return NULL;
    >> +
    >> + memset(pool, 0, ovhd_size);
    >
    > Please use kzalloc.
    >

    Ok...

    >> +
    >> + for (i = 0; i < NUM_FREE_LISTS; i++) {
    >> + pool->freelist[i].pagenum = 0;
    >> + pool->freelist[i].offset = 0;
    >> + }
    >
    > It was already zeroed by memset.
    >
    Remains from earlier code; we used to set it to 'INVALID_PGNUM'.
    Ok, will remove this.


    Thanks for review.
    Nitin


    \
     
     \ /
      Last update: 2009-03-29 13:27    [W:0.023 / U:3.116 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site