[PATCH] zsmalloc: use unsigned long instead of void *

[PATCH] zsmalloc: use unsigned long instead of void *

[Minchan Kim]

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.

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;
 }
 
-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;
 
 	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;
 	u16 size;	/* object size (excluding header) */
 	u8 count;	/* object ref count (not yet used) */
 	u8 flags;
diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
index 4496737..ae10a1a 100644
--- a/drivers/staging/zsmalloc/zsmalloc-main.c
+++ b/drivers/staging/zsmalloc/zsmalloc-main.c
@@ -247,10 +247,10 @@ static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
 }
 
 /* Decode <page, obj_idx> pair from the given object handle */
-static void obj_handle_to_location(void *handle, struct page **page,
+static void obj_handle_to_location(unsigned long handle, struct page **page,
 				unsigned long *obj_idx)
 {
-	unsigned long hval = (unsigned long)handle;
+	unsigned long hval = handle;
 
 	*page = pfn_to_page(hval >> OBJ_INDEX_BITS);
 	*obj_idx = hval & OBJ_INDEX_MASK;
@@ -568,12 +568,12 @@ EXPORT_SYMBOL_GPL(zs_destroy_pool);
  * @size: size of block to allocate
  *
  * On success, handle to the allocated object is returned,
- * otherwise NULL.
+ * otherwise 0.
  * Allocation requests with size > ZS_MAX_ALLOC_SIZE will fail.
  */
-void *zs_malloc(struct zs_pool *pool, size_t size)
+unsigned long zs_malloc(struct zs_pool *pool, size_t size)
 {
-	void *obj;
+	unsigned long obj;
 	struct link_free *link;
 	int class_idx;
 	struct size_class *class;
@@ -582,7 +582,7 @@ void *zs_malloc(struct zs_pool *pool, size_t size)
 	unsigned long m_objidx, m_offset;
 
 	if (unlikely(!size || size > ZS_MAX_ALLOC_SIZE))
-		return NULL;
+		return 0;
 
 	class_idx = get_size_class_index(size);
 	class = &pool->size_class[class_idx];
@@ -595,14 +595,14 @@ void *zs_malloc(struct zs_pool *pool, size_t size)
 		spin_unlock(&class->lock);
 		first_page = alloc_zspage(class, pool->flags);
 		if (unlikely(!first_page))
-			return NULL;
+			return 0;
 
 		set_zspage_mapping(first_page, class->index, ZS_EMPTY);
 		spin_lock(&class->lock);
 		class->pages_allocated += class->pages_per_zspage;
 	}
 
-	obj = first_page->freelist;
+	obj = (unsigned long)first_page->freelist;
 	obj_handle_to_location(obj, &m_page, &m_objidx);
 	m_offset = obj_idx_to_offset(m_page, m_objidx, class->size);
 
@@ -621,7 +621,7 @@ void *zs_malloc(struct zs_pool *pool, size_t size)
 }
 EXPORT_SYMBOL_GPL(zs_malloc);
 
-void zs_free(struct zs_pool *pool, void *obj)
+void zs_free(struct zs_pool *pool, unsigned long obj)
 {
 	struct link_free *link;
 	struct page *first_page, *f_page;
@@ -648,7 +648,7 @@ void zs_free(struct zs_pool *pool, void *obj)
 							+ f_offset);
 	link->next = first_page->freelist;
 	kunmap_atomic(link);
-	first_page->freelist = obj;
+	first_page->freelist = (void *)obj;
 
 	first_page->inuse--;
 	fullness = fix_fullness_group(pool, first_page);
@@ -672,7 +672,7 @@ EXPORT_SYMBOL_GPL(zs_free);
  * this function. When done with the object, it must be unmapped using
  * zs_unmap_object
 */
-void *zs_map_object(struct zs_pool *pool, void *handle)
+void *zs_map_object(struct zs_pool *pool, unsigned long handle)
 {
 	struct page *page;
 	unsigned long obj_idx, off;
@@ -712,7 +712,7 @@ void *zs_map_object(struct zs_pool *pool, void *handle)
 }
 EXPORT_SYMBOL_GPL(zs_map_object);
 
-void zs_unmap_object(struct zs_pool *pool, void *handle)
+void zs_unmap_object(struct zs_pool *pool, unsigned long handle)
 {
 	struct page *page;
 	unsigned long obj_idx, off;
diff --git a/drivers/staging/zsmalloc/zsmalloc.h b/drivers/staging/zsmalloc/zsmalloc.h
index 949384e..485cbb1 100644
--- a/drivers/staging/zsmalloc/zsmalloc.h
+++ b/drivers/staging/zsmalloc/zsmalloc.h
@@ -20,11 +20,11 @@ struct zs_pool;
 struct zs_pool *zs_create_pool(const char *name, gfp_t flags);
 void zs_destroy_pool(struct zs_pool *pool);
 
-void *zs_malloc(struct zs_pool *pool, size_t size);
-void zs_free(struct zs_pool *pool, void *obj);
+unsigned long zs_malloc(struct zs_pool *pool, size_t size);
+void zs_free(struct zs_pool *pool, unsigned long obj);
 
-void *zs_map_object(struct zs_pool *pool, void *handle);
-void zs_unmap_object(struct zs_pool *pool, void *handle);
+void *zs_map_object(struct zs_pool *pool, unsigned long handle);
+void zs_unmap_object(struct zs_pool *pool, unsigned long handle);
 
 u64 zs_get_total_size_bytes(struct zs_pool *pool);
 
-- 
1.7.9.5

Re: [PATCH] zsmalloc: use unsigned long instead of void *

[Seth Jennings]

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?

> 
> 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.

>  }
> 
> -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.

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

RE: [PATCH] zsmalloc: use unsigned long instead of void *

[Dan Magenheimer]

> From: Seth Jennings [mailto:sjenning@linux.vnet.ibm.com]
> Subject: Re: [PATCH] zsmalloc: use unsigned long instead of void *
> 
> 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.

Someone once said: "Opaque is a computer science term and has no
meaning in system software and computer engineering."  ;-)

Re: [PATCH] zsmalloc: use unsigned long instead of void *

[Seth Jennings]

On 05/21/2012 09:19 AM, 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.


Wasn't really clear here.  All that to say, I think we do need this patch.

Thanks,
Seth

Re: [PATCH] zsmalloc: use unsigned long instead of void *

[Konrad Rzeszutek Wilk]

On Tue, May 22, 2012 at 08:42:10AM -0500, Seth Jennings wrote:
> On 05/21/2012 09:19 AM, 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.
> 
> 
> Wasn't really clear here.  All that to say, I think we do need this patch.

That sounds like an Acked-by ?

> 
> Thanks,
> Seth

Re: [PATCH] zsmalloc: use unsigned long instead of void *

[Seth Jennings]

On 05/22/2012 01:31 PM, Konrad Rzeszutek Wilk wrote:

> On Tue, May 22, 2012 at 08:42:10AM -0500, Seth Jennings wrote:
>> On 05/21/2012 09:19 AM, 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.
>>
>>
>> Wasn't really clear here.  All that to say, I think we do need this patch.
> 
> That sounds like an Acked-by ?


Almost. I still need to know what the base is so I can apply the
patchset and at least build it before I add my Ack.

Thanks,
Seth

Re: [PATCH] zsmalloc: use unsigned long instead of void *

[Minchan Kim]

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

Re: [PATCH] zsmalloc: use unsigned long instead of void *

[Minchan Kim]

On 05/23/2012 09:02 AM, Minchan Kim wrote:

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


I didn't based v2 patches on v3.4 because mainline tree doesn't have lots of staging patchset.
So it's not a good idea to apply staging patchset in mainline. I believe linux-next is good candidate
for it so I sent v2 patches against next-20120522.

Thanks.

-- 
Kind regards,
Minchan Kim

Re: [PATCH] zsmalloc: use unsigned long instead of void *

[Greg Kroah-Hartman]

On Wed, May 23, 2012 at 09:02:30AM +0900, Minchan Kim wrote:
> 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?

linux-next is fine.

But note, I'm ignoring all patches for the next 2 weeks, especially
staging patches, as this is the merge window time, and I can't apply
anything to my trees, sorry.

After 3.5-rc1 is out, then I will look at new stuff like this again.

thanks,

greg k-h