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

[Seth Jennings <sjenning@xxxxxxxxxxxxxxxxxx>]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/