lkml.org 
[lkml]   [2012]   [May]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] zsmalloc: use unsigned long instead of void *
    On 05/21/2012 11:19 PM, Seth Jennings wrote:

    > On 05/20/2012 09:23 PM, Minchan Kim wrote:
    >
    >> We should use unsigned long as 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.
    >
    >
    > I wouldn't have agreed with you about the need for this change as people
    > should understand a void * to be the address of some data with unknown
    > structure.
    >
    > However, I recently discussed with Dan regarding his RAMster project
    > where he assumed that the void * would be an address, and as such,
    > 4-byte aligned. So he has masked two bits into the two LSBs of the
    > handle for RAMster, which doesn't work with zsmalloc since the handle is
    > not an address.
    >
    > So really we do need to convey as explicitly as possible to the user
    > that the handle is an _opaque_ value about which no assumption can be made.
    >
    > Also, I wanted to test this but is doesn't apply cleanly on
    > zsmalloc-main.c on v3.4 or what I have as your latest patch series.
    > What is the base for this patch?


    It's based on next-20120518.
    I have always used linux-next tree for staging.
    Greg, What's the convenient tree for you?

    Maybe I will resend next spin based on v3.4 today
    I hope it doesn't hurt you.

    >
    >>
    >> This patch passed compile test(zram, zcache and ramster) and zram is
    >> tested on qemu.
    >>
    >> Cc: Seth Jennings <sjenning@linux.vnet.ibm.com>
    >> Cc: Dan Magenheimer <dan.magenheimer@oracle.com>
    >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
    >> Cc: Nitin Gupta <ngupta@vflare.org>
    >> Signed-off-by: Minchan Kim <minchan@kernel.org>
    >> ---
    >>
    >> Nitin, Konrad and I discussed and concluded that we should use 'unsigned long'
    >> instead of 'void *'.
    >> Look at the lengthy thread if you have a question.
    >> http://marc.info/?l=linux-mm&m=133716653118566&w=4
    >> Watch out! it has number of noises.
    >>
    >> drivers/staging/zcache/zcache-main.c | 12 ++++++------
    >> drivers/staging/zram/zram_drv.c | 16 ++++++++--------
    >> drivers/staging/zram/zram_drv.h | 2 +-
    >> drivers/staging/zsmalloc/zsmalloc-main.c | 24 ++++++++++++------------
    >> drivers/staging/zsmalloc/zsmalloc.h | 8 ++++----
    >> 5 files changed, 31 insertions(+), 31 deletions(-)
    >>
    >> diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
    >> index 2734dac..4c218a7 100644
    >> --- a/drivers/staging/zcache/zcache-main.c
    >> +++ b/drivers/staging/zcache/zcache-main.c
    >> @@ -700,7 +700,7 @@ static struct zv_hdr *zv_create(struct zs_pool *pool, uint32_t pool_id,
    >> struct zv_hdr *zv;
    >> u32 size = clen + sizeof(struct zv_hdr);
    >> int chunks = (size + (CHUNK_SIZE - 1)) >> CHUNK_SHIFT;
    >> - void *handle = NULL;
    >> + unsigned long handle = 0;
    >>
    >> BUG_ON(!irqs_disabled());
    >> BUG_ON(chunks >= NCHUNKS);
    >> @@ -718,10 +718,10 @@ static struct zv_hdr *zv_create(struct zs_pool *pool, uint32_t pool_id,
    >> memcpy((char *)zv + sizeof(struct zv_hdr), cdata, clen);
    >> zs_unmap_object(pool, handle);
    >> out:
    >> - return handle;
    >> + return (struct zv_hdr *)handle;
    >
    >
    > This is kind of weird, and somewhat defeats the point, casting it back
    > to a pointer. I know you'd have to change it all the way up the stack.
    > Just saying.


    Okay.

    >
    >> }
    >>
    >> -static void zv_free(struct zs_pool *pool, void *handle)
    >> +static void zv_free(struct zs_pool *pool, unsigned long handle)
    >> {
    >> unsigned long flags;
    >> struct zv_hdr *zv;
    >> @@ -743,7 +743,7 @@ static void zv_free(struct zs_pool *pool, void *handle)
    >> local_irq_restore(flags);
    >> }
    >>
    >> -static void zv_decompress(struct page *page, void *handle)
    >> +static void zv_decompress(struct page *page, unsigned long handle)
    >> {
    >> unsigned int clen = PAGE_SIZE;
    >> char *to_va;
    >> @@ -1247,7 +1247,7 @@ static int zcache_pampd_get_data(char *data, size_t *bufsize, bool raw,
    >> int ret = 0;
    >>
    >> BUG_ON(is_ephemeral(pool));
    >> - zv_decompress((struct page *)(data), pampd);
    >> + zv_decompress((struct page *)(data), (unsigned long)pampd);
    >> return ret;
    >> }
    >>
    >> @@ -1282,7 +1282,7 @@ static void zcache_pampd_free(void *pampd, struct tmem_pool *pool,
    >> atomic_dec(&zcache_curr_eph_pampd_count);
    >> BUG_ON(atomic_read(&zcache_curr_eph_pampd_count) < 0);
    >> } else {
    >> - zv_free(cli->zspool, pampd);
    >> + zv_free(cli->zspool, (unsigned long)pampd);
    >> atomic_dec(&zcache_curr_pers_pampd_count);
    >> BUG_ON(atomic_read(&zcache_curr_pers_pampd_count) < 0);
    >> }
    >> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
    >> index 685d612..abd69d1 100644
    >> --- a/drivers/staging/zram/zram_drv.c
    >> +++ b/drivers/staging/zram/zram_drv.c
    >> @@ -135,7 +135,7 @@ static void zram_set_disksize(struct zram *zram, size_t totalram_bytes)
    >>
    >> static void zram_free_page(struct zram *zram, size_t index)
    >> {
    >> - void *handle = zram->table[index].handle;
    >> + unsigned long handle = zram->table[index].handle;
    >
    >
    > Should we incorporate the union { handle, page } idea we were working on
    > earlier before doing this? Might cut down on some the casting below.


    Yes. It should be another patch and I don't care it's applied based on
    this patch or reverse.
    I will try it.

    >
    >>
    >> if (unlikely(!handle)) {
    >> /*
    >> @@ -150,7 +150,7 @@ static void zram_free_page(struct zram *zram, size_t index)
    >> }
    >>
    >> if (unlikely(zram_test_flag(zram, index, ZRAM_UNCOMPRESSED))) {
    >> - __free_page(handle);
    >> + __free_page((struct page *)handle);
    >> zram_clear_flag(zram, index, ZRAM_UNCOMPRESSED);
    >> zram_stat_dec(&zram->stats.pages_expand);
    >> goto out;
    >> @@ -166,7 +166,7 @@ out:
    >> zram->table[index].size);
    >> zram_stat_dec(&zram->stats.pages_stored);
    >>
    >> - zram->table[index].handle = NULL;
    >> + zram->table[index].handle = 0;
    >> zram->table[index].size = 0;
    >> }
    >>
    >> @@ -189,7 +189,7 @@ static void handle_uncompressed_page(struct zram *zram, struct bio_vec *bvec,
    >> unsigned char *user_mem, *cmem;
    >>
    >> user_mem = kmap_atomic(page);
    >> - cmem = kmap_atomic(zram->table[index].handle);
    >> + cmem = kmap_atomic((struct page *)zram->table[index].handle);
    >>
    >> memcpy(user_mem + bvec->bv_offset, cmem + offset, bvec->bv_len);
    >> kunmap_atomic(cmem);
    >> @@ -317,7 +317,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
    >> int ret;
    >> u32 store_offset;
    >> size_t clen;
    >> - void *handle;
    >> + unsigned long handle;
    >> struct zobj_header *zheader;
    >> struct page *page, *page_store;
    >> unsigned char *user_mem, *cmem, *src, *uncmem = NULL;
    >> @@ -399,7 +399,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
    >> store_offset = 0;
    >> zram_set_flag(zram, index, ZRAM_UNCOMPRESSED);
    >> zram_stat_inc(&zram->stats.pages_expand);
    >> - handle = page_store;
    >> + handle = (unsigned long)page_store;
    >> src = kmap_atomic(page);
    >> cmem = kmap_atomic(page_store);
    >> goto memstore;
    >> @@ -592,12 +592,12 @@ void __zram_reset_device(struct zram *zram)
    >>
    >> /* Free all pages that are still in this zram device */
    >> for (index = 0; index < zram->disksize >> PAGE_SHIFT; index++) {
    >> - void *handle = zram->table[index].handle;
    >> + unsigned long handle = zram->table[index].handle;
    >> if (!handle)
    >> continue;
    >>
    >> if (unlikely(zram_test_flag(zram, index, ZRAM_UNCOMPRESSED)))
    >> - __free_page(handle);
    >> + __free_page((struct page *)handle);
    >> else
    >> zs_free(zram->mem_pool, handle);
    >> }
    >> diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
    >> index fbe8ac9..7a7e256 100644
    >> --- a/drivers/staging/zram/zram_drv.h
    >> +++ b/drivers/staging/zram/zram_drv.h
    >> @@ -81,7 +81,7 @@ enum zram_pageflags {
    >>
    >> /* Allocated for each disk page */
    >> struct table {
    >> - void *handle;
    >> + unsigned long handle;
    >
    >
    > Putting the union here, as mentioned above.
    >
    >> u16 size; /* object size (excluding header) */
    >> u8 count; /* object ref count (not yet used) */
    >> u8 flags;
    >
    > <snip>
    >
    > Thanks,
    > Seth
    >
    > --
    > To unsubscribe, send a message with 'unsubscribe linux-mm' in
    > the body to majordomo@kvack.org. For more info on Linux MM,
    > see: http://www.linux-mm.org/ .
    > Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
    > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
    >



    --
    Kind regards,
    Minchan Kim


    \
     
     \ /
      Last update: 2012-05-23 02:41    [W:0.192 / U:0.192 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site