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@xxxxxxxxxxxxxxxxxx> >> Cc: Dan Magenheimer <dan.magenheimer@xxxxxxxxxx> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> >> Cc: Nitin Gupta <ngupta@xxxxxxxxxx> >> Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx> >> --- >> >> 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@xxxxxxxxx. 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@xxxxxxxxx";> email@xxxxxxxxx </a> > -- Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. 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@xxxxxxxxx";> email@xxxxxxxxx </a>