[PATCH 1/4] zsmalloc: rename zspage_order with zspage_pages

[PATCH 1/4] zsmalloc: rename zspage_order with zspage_pages

[Minchan Kim]

zspage_order defines how many pages are needed to make a zspage.
So _order_ is rather awkward naming. It already deceive Jonathan
- http://lwn.net/Articles/477067/
" For each size, the code calculates an optimum number of pages (up to 16)"

Let's change from _order_ to _pages_ and some function names.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 drivers/staging/zsmalloc/zsmalloc-main.c |   14 +++++++-------
 drivers/staging/zsmalloc/zsmalloc_int.h  |    2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
index 504b6c2..8642800 100644
--- a/drivers/staging/zsmalloc/zsmalloc-main.c
+++ b/drivers/staging/zsmalloc/zsmalloc-main.c
@@ -180,7 +180,7 @@ out:
  * link together 3 PAGE_SIZE sized pages to form a zspage
  * since then we can perfectly fit in 8 such objects.
  */
-static int get_zspage_order(int class_size)
+static int get_pages_per_zspage(int class_size)
 {
 	int i, max_usedpc = 0;
 	/* zspage order which gives maximum used size per KB */
@@ -368,7 +368,7 @@ static struct page *alloc_zspage(struct size_class *class, gfp_t flags)
 	 * identify the last page.
 	 */
 	error = -ENOMEM;
-	for (i = 0; i < class->zspage_order; i++) {
+	for (i = 0; i < class->pages_per_zspage; i++) {
 		struct page *page, *prev_page;
 
 		page = alloc_page(flags);
@@ -388,7 +388,7 @@ static struct page *alloc_zspage(struct size_class *class, gfp_t flags)
 			page->first_page = first_page;
 		if (i >= 2)
 			list_add(&page->lru, &prev_page->lru);
-		if (i == class->zspage_order - 1)	/* last page */
+		if (i == class->pages_per_zspage - 1)	/* last page */
 			SetPagePrivate2(page);
 		prev_page = page;
 	}
@@ -397,7 +397,7 @@ static struct page *alloc_zspage(struct size_class *class, gfp_t flags)
 
 	first_page->freelist = obj_location_to_handle(first_page, 0);
 	/* Maximum number of objects we can store in this zspage */
-	first_page->objects = class->zspage_order * PAGE_SIZE / class->size;
+	first_page->objects = class->pages_per_zspage * PAGE_SIZE / class->size;
 
 	error = 0; /* Success */
 
@@ -512,7 +512,7 @@ struct zs_pool *zs_create_pool(const char *name, gfp_t flags)
 		class->size = size;
 		class->index = i;
 		spin_lock_init(&class->lock);
-		class->zspage_order = get_zspage_order(size);
+		class->pages_per_zspage = get_pages_per_zspage(size);
 
 	}
 
@@ -603,7 +603,7 @@ void *zs_malloc(struct zs_pool *pool, size_t size)
 
 		set_zspage_mapping(first_page, class->index, ZS_EMPTY);
 		spin_lock(&class->lock);
-		class->pages_allocated += class->zspage_order;
+		class->pages_allocated += class->pages_per_zspage;
 	}
 
 	obj = first_page->freelist;
@@ -658,7 +658,7 @@ void zs_free(struct zs_pool *pool, void *obj)
 	fullness = fix_fullness_group(pool, first_page);
 
 	if (fullness == ZS_EMPTY)
-		class->pages_allocated -= class->zspage_order;
+		class->pages_allocated -= class->pages_per_zspage;
 
 	spin_unlock(&class->lock);
 
diff --git a/drivers/staging/zsmalloc/zsmalloc_int.h b/drivers/staging/zsmalloc/zsmalloc_int.h
index 92eefc6..6fd32a9 100644
--- a/drivers/staging/zsmalloc/zsmalloc_int.h
+++ b/drivers/staging/zsmalloc/zsmalloc_int.h
@@ -124,7 +124,7 @@ struct size_class {
 	unsigned int index;
 
 	/* Number of PAGE_SIZE sized pages to combine to form a 'zspage' */
-	int zspage_order;
+	int pages_per_zspage;
 
 	spinlock_t lock;
 
-- 
1.7.9.5

[PATCH 2/4] zsmalloc: add/fix function comment

[Minchan Kim]

Add/fix the comment.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 drivers/staging/zsmalloc/zsmalloc-main.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
index 8642800..4496737 100644
--- a/drivers/staging/zsmalloc/zsmalloc-main.c
+++ b/drivers/staging/zsmalloc/zsmalloc-main.c
@@ -566,13 +566,9 @@ EXPORT_SYMBOL_GPL(zs_destroy_pool);
  * zs_malloc - Allocate block of given size from pool.
  * @pool: pool to allocate from
  * @size: size of block to allocate
- * @page: page no. that holds the object
- * @offset: location of object within page
- *
- * On success, <page, offset> identifies block allocated
- * and 0 is returned. On failure, <page, offset> is set to
- * 0 and -ENOMEM is returned.
  *
+ * On success, handle to the allocated object is returned,
+ * otherwise NULL.
  * Allocation requests with size > ZS_MAX_ALLOC_SIZE will fail.
  */
 void *zs_malloc(struct zs_pool *pool, size_t size)
@@ -667,6 +663,15 @@ void zs_free(struct zs_pool *pool, void *obj)
 }
 EXPORT_SYMBOL_GPL(zs_free);
 
+/**
+ * zs_map_object - get address of allocated object from handle.
+ * @pool: pool from which the object was allocated
+ * @handle: handle returned from zs_malloc
+ *
+ * Before using an object allocated from zs_malloc, it must be mapped using
+ * 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)
 {
 	struct page *page;
-- 
1.7.9.5

[PATCH 3/4] zsmalloc use zs_handle instead of void *

[Minchan Kim]

We should use zs_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.

Cc: Dan Magenheimer <dan.magenheimer@oracle.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 drivers/staging/zcache/zcache-main.c     |    8 ++++----
 drivers/staging/zram/zram_drv.c          |    8 ++++----
 drivers/staging/zram/zram_drv.h          |    2 +-
 drivers/staging/zsmalloc/zsmalloc-main.c |   28 ++++++++++++++--------------
 drivers/staging/zsmalloc/zsmalloc.h      |   15 +++++++++++----
 5 files changed, 34 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
index 2734dac..9b06948 100644
--- a/drivers/staging/zcache/zcache-main.c
+++ b/drivers/staging/zcache/zcache-main.c
@@ -700,12 +700,12 @@ 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;
+	zs_handle handle;
 
 	BUG_ON(!irqs_disabled());
 	BUG_ON(chunks >= NCHUNKS);
 	handle = zs_malloc(pool, size);
-	if (!handle)
+	if (zs_handle_invalid(handle))
 		goto out;
 	atomic_inc(&zv_curr_dist_counts[chunks]);
 	atomic_inc(&zv_cumul_dist_counts[chunks]);
@@ -721,7 +721,7 @@ out:
 	return handle;
 }
 
-static void zv_free(struct zs_pool *pool, void *handle)
+static void zv_free(struct zs_pool *pool, zs_handle 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, zs_handle handle)
 {
 	unsigned int clen = PAGE_SIZE;
 	char *to_va;
diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 685d612..7e42aa2 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;
+	zs_handle handle = zram->table[index].handle;
 
 	if (unlikely(!handle)) {
 		/*
@@ -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;
+	zs_handle handle;
 	struct zobj_header *zheader;
 	struct page *page, *page_store;
 	unsigned char *user_mem, *cmem, *src, *uncmem = NULL;
@@ -406,7 +406,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	}
 
 	handle = zs_malloc(zram->mem_pool, clen + sizeof(*zheader));
-	if (!handle) {
+	if (zs_handle_invalid(handle)) {
 		pr_info("Error allocating memory for compressed "
 			"page: %u, size=%zu\n", index, clen);
 		ret = -ENOMEM;
@@ -592,7 +592,7 @@ 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;
+		zs_handle handle = zram->table[index].handle;
 		if (!handle)
 			continue;
 
diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
index fbe8ac9..07d3192 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;
+	zs_handle 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..51074fa 100644
--- a/drivers/staging/zsmalloc/zsmalloc-main.c
+++ b/drivers/staging/zsmalloc/zsmalloc-main.c
@@ -247,7 +247,7 @@ 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(zs_handle handle, struct page **page,
 				unsigned long *obj_idx)
 {
 	unsigned long hval = (unsigned long)handle;
@@ -571,9 +571,9 @@ EXPORT_SYMBOL_GPL(zs_destroy_pool);
  * otherwise NULL.
  * Allocation requests with size > ZS_MAX_ALLOC_SIZE will fail.
  */
-void *zs_malloc(struct zs_pool *pool, size_t size)
+zs_handle zs_malloc(struct zs_pool *pool, size_t size)
 {
-	void *obj;
+	zs_handle handle;
 	struct link_free *link;
 	int class_idx;
 	struct size_class *class;
@@ -602,8 +602,8 @@ void *zs_malloc(struct zs_pool *pool, size_t size)
 		class->pages_allocated += class->pages_per_zspage;
 	}
 
-	obj = first_page->freelist;
-	obj_handle_to_location(obj, &m_page, &m_objidx);
+	handle = first_page->freelist;
+	obj_handle_to_location(handle, &m_page, &m_objidx);
 	m_offset = obj_idx_to_offset(m_page, m_objidx, class->size);
 
 	link = (struct link_free *)kmap_atomic(m_page) +
@@ -617,11 +617,11 @@ void *zs_malloc(struct zs_pool *pool, size_t size)
 	fix_fullness_group(pool, first_page);
 	spin_unlock(&class->lock);
 
-	return obj;
+	return handle;
 }
 EXPORT_SYMBOL_GPL(zs_malloc);
 
-void zs_free(struct zs_pool *pool, void *obj)
+void zs_free(struct zs_pool *pool, zs_handle handle)
 {
 	struct link_free *link;
 	struct page *first_page, *f_page;
@@ -631,10 +631,10 @@ void zs_free(struct zs_pool *pool, void *obj)
 	struct size_class *class;
 	enum fullness_group fullness;
 
-	if (unlikely(!obj))
+	if (unlikely(zs_handle_invalid(handle)))
 		return;
 
-	obj_handle_to_location(obj, &f_page, &f_objidx);
+	obj_handle_to_location(handle, &f_page, &f_objidx);
 	first_page = get_first_page(f_page);
 
 	get_zspage_mapping(first_page, &class_idx, &fullness);
@@ -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 = handle;
 
 	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, zs_handle handle)
 {
 	struct page *page;
 	unsigned long obj_idx, off;
@@ -682,7 +682,7 @@ void *zs_map_object(struct zs_pool *pool, void *handle)
 	struct size_class *class;
 	struct mapping_area *area;
 
-	BUG_ON(!handle);
+	BUG_ON(zs_handle_invalid(handle));
 
 	obj_handle_to_location(handle, &page, &obj_idx);
 	get_zspage_mapping(get_first_page(page), &class_idx, &fg);
@@ -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, zs_handle handle)
 {
 	struct page *page;
 	unsigned long obj_idx, off;
@@ -722,7 +722,7 @@ void zs_unmap_object(struct zs_pool *pool, void *handle)
 	struct size_class *class;
 	struct mapping_area *area;
 
-	BUG_ON(!handle);
+	BUG_ON(zs_handle_invalid(handle));
 
 	obj_handle_to_location(handle, &page, &obj_idx);
 	get_zspage_mapping(get_first_page(page), &class_idx, &fg);
diff --git a/drivers/staging/zsmalloc/zsmalloc.h b/drivers/staging/zsmalloc/zsmalloc.h
index 949384e..1ba6d0c 100644
--- a/drivers/staging/zsmalloc/zsmalloc.h
+++ b/drivers/staging/zsmalloc/zsmalloc.h
@@ -16,15 +16,22 @@
 #include <linux/types.h>
 
 struct zs_pool;
+typedef void * zs_handle;
+
+/*
+ * zs_malloc's caller should use zs_handle_invalid instead of if (!handle)
+ * to test successful allocation.
+ */
+#define zs_handle_invalid(zs_handle) !zs_handle
 
 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);
+zs_handle zs_malloc(struct zs_pool *pool, size_t size);
+void zs_free(struct zs_pool *pool, zs_handle handle);
 
-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, zs_handle handle);
+void zs_unmap_object(struct zs_pool *pool, zs_handle handle);
 
 u64 zs_get_total_size_bytes(struct zs_pool *pool);
 
-- 
1.7.9.5

[PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Minchan Kim]

It's a overkill to align pool size with PAGE_SIZE to avoid
false-sharing. This patch aligns it with just cache line size.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 drivers/staging/zsmalloc/zsmalloc-main.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
index 51074fa..3991b03 100644
--- a/drivers/staging/zsmalloc/zsmalloc-main.c
+++ b/drivers/staging/zsmalloc/zsmalloc-main.c
@@ -489,14 +489,14 @@ fail:
 
 struct zs_pool *zs_create_pool(const char *name, gfp_t flags)
 {
-	int i, error, ovhd_size;
+	int i, error;
 	struct zs_pool *pool;
 
 	if (!name)
 		return NULL;
 
-	ovhd_size = roundup(sizeof(*pool), PAGE_SIZE);
-	pool = kzalloc(ovhd_size, GFP_KERNEL);
+	pool = kzalloc(ALIGN(sizeof(*pool), cache_line_size()),
+				GFP_KERNEL);
 	if (!pool)
 		return NULL;
 
-- 
1.7.9.5

Re: [PATCH 1/4] zsmalloc: rename zspage_order with zspage_pages

[Nitin Gupta]

On 5/3/12 2:40 AM, Minchan Kim wrote:
> zspage_order defines how many pages are needed to make a zspage.
> So_order_  is rather awkward naming. It already deceive Jonathan
> -http://lwn.net/Articles/477067/
> " For each size, the code calculates an optimum number of pages (up to 16)"
>
> Let's change from_order_  to_pages_  and some function names.
>
> Signed-off-by: Minchan Kim<minchan@kernel.org>
> ---
>   drivers/staging/zsmalloc/zsmalloc-main.c |   14 +++++++-------
>   drivers/staging/zsmalloc/zsmalloc_int.h  |    2 +-
>   2 files changed, 8 insertions(+), 8 deletions(-)

Acked-by: Nitin Gupta <ngupta@vflare.org>

Thanks,
Nitin

Re: [PATCH 2/4] zsmalloc: add/fix function comment

[Nitin Gupta]

On 5/3/12 2:40 AM, Minchan Kim wrote:
> Add/fix the comment.
>
> Signed-off-by: Minchan Kim<minchan@kernel.org>
> ---
>   drivers/staging/zsmalloc/zsmalloc-main.c |   17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)

Acked-by: Nitin Gupta <ngupta@vflare.org>

Thanks,
Nitin

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Nitin Gupta]

On 5/3/12 2:40 AM, Minchan Kim wrote:
> We should use zs_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.
>
> Cc: Dan Magenheimer<dan.magenheimer@oracle.com>
> Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
> Signed-off-by: Minchan Kim<minchan@kernel.org>
> ---
>   drivers/staging/zcache/zcache-main.c     |    8 ++++----
>   drivers/staging/zram/zram_drv.c          |    8 ++++----
>   drivers/staging/zram/zram_drv.h          |    2 +-
>   drivers/staging/zsmalloc/zsmalloc-main.c |   28 ++++++++++++++--------------
>   drivers/staging/zsmalloc/zsmalloc.h      |   15 +++++++++++----
>   5 files changed, 34 insertions(+), 27 deletions(-)

This was a long pending change. Thanks!

Acked-by: Nitin Gupta <ngupta@vflare.org>


  - Nitin

Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Nitin Gupta]

On 5/3/12 2:40 AM, Minchan Kim wrote:
> It's a overkill to align pool size with PAGE_SIZE to avoid
> false-sharing. This patch aligns it with just cache line size.
>
> Signed-off-by: Minchan Kim<minchan@kernel.org>
> ---
>   drivers/staging/zsmalloc/zsmalloc-main.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
> index 51074fa..3991b03 100644
> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
> @@ -489,14 +489,14 @@ fail:
>
>   struct zs_pool *zs_create_pool(const char *name, gfp_t flags)
>   {
> -	int i, error, ovhd_size;
> +	int i, error;
>   	struct zs_pool *pool;
>
>   	if (!name)
>   		return NULL;
>
> -	ovhd_size = roundup(sizeof(*pool), PAGE_SIZE);
> -	pool = kzalloc(ovhd_size, GFP_KERNEL);
> +	pool = kzalloc(ALIGN(sizeof(*pool), cache_line_size()),
> +				GFP_KERNEL);

a basic question:
  Is rounding off allocation size to cache_line_size enough to ensure 
that the object is cache-line-aligned? Isn't it possible that even 
though the object size is multiple of cache-line, it may still not be 
properly aligned and end up sharing cache line with some other 
read-mostly object?

Thanks,
Nitin


>   	if (!pool)
>   		return NULL;
>

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Seth Jennings]

On 05/03/2012 08:32 AM, Nitin Gupta wrote:

> On 5/3/12 2:40 AM, Minchan Kim wrote:
>> We should use zs_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.
>>
>> Cc: Dan Magenheimer<dan.magenheimer@oracle.com>
>> Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
>> Signed-off-by: Minchan Kim<minchan@kernel.org>
>> ---
>>   drivers/staging/zcache/zcache-main.c     |    8 ++++----
>>   drivers/staging/zram/zram_drv.c          |    8 ++++----
>>   drivers/staging/zram/zram_drv.h          |    2 +-
>>   drivers/staging/zsmalloc/zsmalloc-main.c |   28
>> ++++++++++++++--------------
>>   drivers/staging/zsmalloc/zsmalloc.h      |   15 +++++++++++----
>>   5 files changed, 34 insertions(+), 27 deletions(-)
> 
> This was a long pending change. Thanks!


The reason I hadn't done it before is that it introduces a checkpatch
warning:

WARNING: do not add new typedefs
#303: FILE: drivers/staging/zsmalloc/zsmalloc.h:19:
+typedef void * zs_handle;

In addition this particular patch has a checkpatch error:

ERROR: "foo * bar" should be "foo *bar"
#303: FILE: drivers/staging/zsmalloc/zsmalloc.h:19:
+typedef void * zs_handle;

--
Seth

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Minchan Kim]

On 05/04/2012 12:23 AM, Seth Jennings wrote:

> On 05/03/2012 08:32 AM, Nitin Gupta wrote:
> 
>> On 5/3/12 2:40 AM, Minchan Kim wrote:
>>> We should use zs_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.
>>>
>>> Cc: Dan Magenheimer<dan.magenheimer@oracle.com>
>>> Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
>>> Signed-off-by: Minchan Kim<minchan@kernel.org>
>>> ---
>>>   drivers/staging/zcache/zcache-main.c     |    8 ++++----
>>>   drivers/staging/zram/zram_drv.c          |    8 ++++----
>>>   drivers/staging/zram/zram_drv.h          |    2 +-
>>>   drivers/staging/zsmalloc/zsmalloc-main.c |   28
>>> ++++++++++++++--------------
>>>   drivers/staging/zsmalloc/zsmalloc.h      |   15 +++++++++++----
>>>   5 files changed, 34 insertions(+), 27 deletions(-)
>>
>> This was a long pending change. Thanks!
> 
> 
> The reason I hadn't done it before is that it introduces a checkpatch
> warning:
> 
> WARNING: do not add new typedefs
> #303: FILE: drivers/staging/zsmalloc/zsmalloc.h:19:
> +typedef void * zs_handle;
> 


Yes. I did it but I think we are (a) of chapter 5: Typedefs in Documentation/CodingStyle.

 (a) totally opaque objects (where the typedef is actively used to _hide_
     what the object is).

No?

> In addition this particular patch has a checkpatch error:
> 
> ERROR: "foo * bar" should be "foo *bar"
> #303: FILE: drivers/staging/zsmalloc/zsmalloc.h:19:
> +typedef void * zs_handle;


It was my mistake. Will fix.
Thanks!

-- 
Kind regards,
Minchan Kim

Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Minchan Kim]

On 05/03/2012 10:58 PM, Nitin Gupta wrote:

> On 5/3/12 2:40 AM, Minchan Kim wrote:
>> It's a overkill to align pool size with PAGE_SIZE to avoid
>> false-sharing. This patch aligns it with just cache line size.
>>
>> Signed-off-by: Minchan Kim<minchan@kernel.org>
>> ---
>>   drivers/staging/zsmalloc/zsmalloc-main.c |    6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
>> b/drivers/staging/zsmalloc/zsmalloc-main.c
>> index 51074fa..3991b03 100644
>> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
>> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
>> @@ -489,14 +489,14 @@ fail:
>>
>>   struct zs_pool *zs_create_pool(const char *name, gfp_t flags)
>>   {
>> -    int i, error, ovhd_size;
>> +    int i, error;
>>       struct zs_pool *pool;
>>
>>       if (!name)
>>           return NULL;
>>
>> -    ovhd_size = roundup(sizeof(*pool), PAGE_SIZE);
>> -    pool = kzalloc(ovhd_size, GFP_KERNEL);
>> +    pool = kzalloc(ALIGN(sizeof(*pool), cache_line_size()),
>> +                GFP_KERNEL);
> 
> a basic question:
>  Is rounding off allocation size to cache_line_size enough to ensure
> that the object is cache-line-aligned? Isn't it possible that even
> though the object size is multiple of cache-line, it may still not be
> properly aligned and end up sharing cache line with some other
> read-mostly object?


AFAIK, SLAB allocates object aligned cache-size so I think that problem cannot happen.
But needs double check.
Cced Pekka.

> 
> Thanks,
> Nitin
> 
> 
>>       if (!pool)
>>           return NULL;
>>
> 
> -- 
> 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 4/4] zsmalloc: zsmalloc: align cache line size

[Pekka Enberg]

On Fri, 4 May 2012, Minchan Kim wrote:
> >> It's a overkill to align pool size with PAGE_SIZE to avoid
> >> false-sharing. This patch aligns it with just cache line size.
> >>
> >> Signed-off-by: Minchan Kim<minchan@kernel.org>
> >> ---
> >>   drivers/staging/zsmalloc/zsmalloc-main.c |    6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
> >> b/drivers/staging/zsmalloc/zsmalloc-main.c
> >> index 51074fa..3991b03 100644
> >> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
> >> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
> >> @@ -489,14 +489,14 @@ fail:
> >>
> >>   struct zs_pool *zs_create_pool(const char *name, gfp_t flags)
> >>   {
> >> -    int i, error, ovhd_size;
> >> +    int i, error;
> >>       struct zs_pool *pool;
> >>
> >>       if (!name)
> >>           return NULL;
> >>
> >> -    ovhd_size = roundup(sizeof(*pool), PAGE_SIZE);
> >> -    pool = kzalloc(ovhd_size, GFP_KERNEL);
> >> +    pool = kzalloc(ALIGN(sizeof(*pool), cache_line_size()),
> >> +                GFP_KERNEL);
> > 
> > a basic question:
> >  Is rounding off allocation size to cache_line_size enough to ensure
> > that the object is cache-line-aligned? Isn't it possible that even
> > though the object size is multiple of cache-line, it may still not be
> > properly aligned and end up sharing cache line with some other
> > read-mostly object?
> 
> AFAIK, SLAB allocates object aligned cache-size so I think that problem cannot happen.
> But needs double check.
> Cced Pekka.

The kmalloc(size) function only gives you the following guarantees:

  (1) The allocated object is _at least_ 'size' bytes.
 
  (2) The returned pointer is aligned to ARCH_KMALLOC_MINALIGN.

Anything beyond that is implementation detail and probably will break if 
you switch between SLAB/SLUB/SLOB.

			Pekka

Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Nitin Gupta]

On 5/7/12 3:41 AM, Pekka Enberg wrote:
> On Fri, 4 May 2012, Minchan Kim wrote:
>>>> It's a overkill to align pool size with PAGE_SIZE to avoid
>>>> false-sharing. This patch aligns it with just cache line size.
>>>>
>>>> Signed-off-by: Minchan Kim<minchan@kernel.org>
>>>> ---
>>>>    drivers/staging/zsmalloc/zsmalloc-main.c |    6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
>>>> b/drivers/staging/zsmalloc/zsmalloc-main.c
>>>> index 51074fa..3991b03 100644
>>>> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
>>>> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
>>>> @@ -489,14 +489,14 @@ fail:
>>>>
>>>>    struct zs_pool *zs_create_pool(const char *name, gfp_t flags)
>>>>    {
>>>> -    int i, error, ovhd_size;
>>>> +    int i, error;
>>>>        struct zs_pool *pool;
>>>>
>>>>        if (!name)
>>>>            return NULL;
>>>>
>>>> -    ovhd_size = roundup(sizeof(*pool), PAGE_SIZE);
>>>> -    pool = kzalloc(ovhd_size, GFP_KERNEL);
>>>> +    pool = kzalloc(ALIGN(sizeof(*pool), cache_line_size()),
>>>> +                GFP_KERNEL);
>>>
>>> a basic question:
>>>   Is rounding off allocation size to cache_line_size enough to ensure
>>> that the object is cache-line-aligned? Isn't it possible that even
>>> though the object size is multiple of cache-line, it may still not be
>>> properly aligned and end up sharing cache line with some other
>>> read-mostly object?
>>
>> AFAIK, SLAB allocates object aligned cache-size so I think that problem cannot happen.
>> But needs double check.
>> Cced Pekka.
>
> The kmalloc(size) function only gives you the following guarantees:
>
>    (1) The allocated object is _at least_ 'size' bytes.
>
>    (2) The returned pointer is aligned to ARCH_KMALLOC_MINALIGN.
>
> Anything beyond that is implementation detail and probably will break if
> you switch between SLAB/SLUB/SLOB.
>
> 			Pekka

So, we can probably leave it as is (PAGE_SIZE aligned) or use 
kmem_cache_create(...,SLAB_HWCACHE_ALIGN,...) for allocating 'struct 
zs_pool's.

zcache can potentially create a lot of pools, so the latter will save 
some memory.

Thanks,
Nitin

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Seth Jennings]

On 05/03/2012 09:24 PM, Minchan Kim wrote:

> On 05/04/2012 12:23 AM, Seth Jennings wrote:
>> The reason I hadn't done it before is that it introduces a checkpatch
>> warning:
>>
>> WARNING: do not add new typedefs
>> #303: FILE: drivers/staging/zsmalloc/zsmalloc.h:19:
>> +typedef void * zs_handle;
>>
> 
> 
> Yes. I did it but I think we are (a) of chapter 5: Typedefs in Documentation/CodingStyle.
> 
>  (a) totally opaque objects (where the typedef is actively used to _hide_
>      what the object is).
> 
> No?


Interesting, seems like checkpatch and CodingStyle aren't completely in
sync here.  Maybe the warning should say "do not add new typedefs unless
allowed by CodingStyle 5(a)" or something.

Works for me though.

Thanks again Minchan!

--
Seth

Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Minchan Kim]

On 05/07/2012 09:40 PM, Nitin Gupta wrote:

> On 5/7/12 3:41 AM, Pekka Enberg wrote:
>> On Fri, 4 May 2012, Minchan Kim wrote:
>>>>> It's a overkill to align pool size with PAGE_SIZE to avoid
>>>>> false-sharing. This patch aligns it with just cache line size.
>>>>>
>>>>> Signed-off-by: Minchan Kim<minchan@kernel.org>
>>>>> ---
>>>>>    drivers/staging/zsmalloc/zsmalloc-main.c |    6 +++---
>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
>>>>> b/drivers/staging/zsmalloc/zsmalloc-main.c
>>>>> index 51074fa..3991b03 100644
>>>>> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
>>>>> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
>>>>> @@ -489,14 +489,14 @@ fail:
>>>>>
>>>>>    struct zs_pool *zs_create_pool(const char *name, gfp_t flags)
>>>>>    {
>>>>> -    int i, error, ovhd_size;
>>>>> +    int i, error;
>>>>>        struct zs_pool *pool;
>>>>>
>>>>>        if (!name)
>>>>>            return NULL;
>>>>>
>>>>> -    ovhd_size = roundup(sizeof(*pool), PAGE_SIZE);
>>>>> -    pool = kzalloc(ovhd_size, GFP_KERNEL);
>>>>> +    pool = kzalloc(ALIGN(sizeof(*pool), cache_line_size()),
>>>>> +                GFP_KERNEL);
>>>>
>>>> a basic question:
>>>>   Is rounding off allocation size to cache_line_size enough to ensure
>>>> that the object is cache-line-aligned? Isn't it possible that even
>>>> though the object size is multiple of cache-line, it may still not be
>>>> properly aligned and end up sharing cache line with some other
>>>> read-mostly object?
>>>
>>> AFAIK, SLAB allocates object aligned cache-size so I think that
>>> problem cannot happen.
>>> But needs double check.
>>> Cced Pekka.
>>
>> The kmalloc(size) function only gives you the following guarantees:
>>
>>    (1) The allocated object is _at least_ 'size' bytes.
>>
>>    (2) The returned pointer is aligned to ARCH_KMALLOC_MINALIGN.
>>
>> Anything beyond that is implementation detail and probably will break if
>> you switch between SLAB/SLUB/SLOB.
>>
>>             Pekka


Pekka, Thanks.

> 
> So, we can probably leave it as is (PAGE_SIZE aligned) or use
> kmem_cache_create(...,SLAB_HWCACHE_ALIGN,...) for allocating 'struct
> zs_pool's.


3) remove aligning code totally because there isn't any report about degradation by false-sharing. 
4)
origin = pool = kzalloc(sizeof(*pool) + cache_line_size, GFP_KERNEL);
pool = round_up(pool, cache_line_size);

Which preference?
I choose 3.


> 
> zcache can potentially create a lot of pools, so the latter will save
> some memory.


Dumb question.
Why should we create pool per user? 
What's the problem if there is only one pool in system?

> 
> Thanks,
> Nitin
> 
> -- 
> 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 4/4] zsmalloc: zsmalloc: align cache line size

[Dan Magenheimer]

> From: Minchan Kim [mailto:minchan@kernel.org]
> > zcache can potentially create a lot of pools, so the latter will save
> > some memory.
> 
> 
> Dumb question.
> Why should we create pool per user?
> What's the problem if there is only one pool in system?

zcache doesn't use zsmalloc for cleancache pages today, but
that's Seth's plan for the future.  Then if there is a
separate pool for each cleancache pool, when a filesystem
is umount'ed, it isn't necessary to walk through and delete
all pages one-by-one, which could take quite awhile.

ramster needs one pool for each client (i.e. machine in the
cluster) for frontswap pages for the same reason, and
later, for cleancache pages, one per mounted filesystem
per client

Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Minchan Kim]

On 05/08/2012 11:00 PM, Dan Magenheimer wrote:

>> From: Minchan Kim [mailto:minchan@kernel.org]
>>> zcache can potentially create a lot of pools, so the latter will save
>>> some memory.
>>
>>
>> Dumb question.
>> Why should we create pool per user?
>> What's the problem if there is only one pool in system?
> 
> zcache doesn't use zsmalloc for cleancache pages today, but
> that's Seth's plan for the future.  Then if there is a
> separate pool for each cleancache pool, when a filesystem
> is umount'ed, it isn't necessary to walk through and delete
> all pages one-by-one, which could take quite awhile.

> 

> ramster needs one pool for each client (i.e. machine in the
> cluster) for frontswap pages for the same reason, and
> later, for cleancache pages, one per mounted filesystem
> per client


Fair enough.
But some subsystems can't want a own pool for not waste unnecessary memory.

Then, how about this interfaces like slab?

1. zs_handle zs_malloc(size_t size, gfp_t flags) - share a pool by many subsystem(like kmalloc)
2. zs_handle zs_malloc_pool(struct zs_pool *pool, size_t size) - use own pool(like kmem_cache_alloc)

Any thoughts?

> 
> --
> 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=ilto:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

RE: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Dan Magenheimer]

> From: Minchan Kim [mailto:minchan@kernel.org]
> Subject: Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size
> 
> On 05/08/2012 11:00 PM, Dan Magenheimer wrote:
> 
> >> From: Minchan Kim [mailto:minchan@kernel.org]
> >>> zcache can potentially create a lot of pools, so the latter will save
> >>> some memory.
> >>
> >>
> >> Dumb question.
> >> Why should we create pool per user?
> >> What's the problem if there is only one pool in system?
> >
> > zcache doesn't use zsmalloc for cleancache pages today, but
> > that's Seth's plan for the future.  Then if there is a
> > separate pool for each cleancache pool, when a filesystem
> > is umount'ed, it isn't necessary to walk through and delete
> > all pages one-by-one, which could take quite awhile.
> 
> > ramster needs one pool for each client (i.e. machine in the
> > cluster) for frontswap pages for the same reason, and
> > later, for cleancache pages, one per mounted filesystem
> > per client
> 
> Fair enough.
> 
> Then, how about this interfaces like slab?
> 
> 1. zs_handle zs_malloc(size_t size, gfp_t flags) - share a pool by many subsystem(like kmalloc)
> 2. zs_handle zs_malloc_pool(struct zs_pool *pool, size_t size) - use own pool(like kmem_cache_alloc)
> 
> Any thoughts?

Seems fine to me.

> But some subsystems can't want a own pool for not waste unnecessary memory.

Are you using zsmalloc for something else in the kernel?  I'm
wondering what other subsystem would have random size allocations
always less than a page.

Thanks,
Dan

Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Minchan Kim]

On 05/09/2012 12:08 PM, Dan Magenheimer wrote:

>> From: Minchan Kim [mailto:minchan@kernel.org]
>> Subject: Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size
>>
>> On 05/08/2012 11:00 PM, Dan Magenheimer wrote:
>>
>>>> From: Minchan Kim [mailto:minchan@kernel.org]
>>>>> zcache can potentially create a lot of pools, so the latter will save
>>>>> some memory.
>>>>
>>>>
>>>> Dumb question.
>>>> Why should we create pool per user?
>>>> What's the problem if there is only one pool in system?
>>>
>>> zcache doesn't use zsmalloc for cleancache pages today, but
>>> that's Seth's plan for the future.  Then if there is a
>>> separate pool for each cleancache pool, when a filesystem
>>> is umount'ed, it isn't necessary to walk through and delete
>>> all pages one-by-one, which could take quite awhile.
>>
>>> ramster needs one pool for each client (i.e. machine in the
>>> cluster) for frontswap pages for the same reason, and
>>> later, for cleancache pages, one per mounted filesystem
>>> per client
>>
>> Fair enough.
>>
>> Then, how about this interfaces like slab?
>>
>> 1. zs_handle zs_malloc(size_t size, gfp_t flags) - share a pool by many subsystem(like kmalloc)
>> 2. zs_handle zs_malloc_pool(struct zs_pool *pool, size_t size) - use own pool(like kmem_cache_alloc)
>>
>> Any thoughts?
> 
> Seems fine to me.
> 
>> But some subsystems can't want a own pool for not waste unnecessary memory.
> 
> Are you using zsmalloc for something else in the kernel?  I'm
> wondering what other subsystem would have random size allocations
> always less than a page.


Nope. It's a just wondering during review the code about interface.
I thought normal user can use zs_malloc without creating pool so that it's rather easy to use
and simillar to normal allocator which can help availability of zsmalloc.

And it will make space efficiency good.
For example, some subsystem makes own pool and use a object in a class which consists of 4-pages once during her life.
Other object unused in the class are just wasteful but if we can share a pool with others, we may reduce such inefficiency.

I think zsmalloc's goal should be biased on space efficiency.

> 
> Thanks,
> Dan
> 
> --
> 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=ilto:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Greg Kroah-Hartman]

On Fri, May 04, 2012 at 11:24:54AM +0900, Minchan Kim wrote:
> On 05/04/2012 12:23 AM, Seth Jennings wrote:
> 
> > On 05/03/2012 08:32 AM, Nitin Gupta wrote:
> > 
> >> On 5/3/12 2:40 AM, Minchan Kim wrote:
> >>> We should use zs_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.
> >>>
> >>> Cc: Dan Magenheimer<dan.magenheimer@oracle.com>
> >>> Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
> >>> Signed-off-by: Minchan Kim<minchan@kernel.org>
> >>> ---
> >>>   drivers/staging/zcache/zcache-main.c     |    8 ++++----
> >>>   drivers/staging/zram/zram_drv.c          |    8 ++++----
> >>>   drivers/staging/zram/zram_drv.h          |    2 +-
> >>>   drivers/staging/zsmalloc/zsmalloc-main.c |   28
> >>> ++++++++++++++--------------
> >>>   drivers/staging/zsmalloc/zsmalloc.h      |   15 +++++++++++----
> >>>   5 files changed, 34 insertions(+), 27 deletions(-)
> >>
> >> This was a long pending change. Thanks!
> > 
> > 
> > The reason I hadn't done it before is that it introduces a checkpatch
> > warning:
> > 
> > WARNING: do not add new typedefs
> > #303: FILE: drivers/staging/zsmalloc/zsmalloc.h:19:
> > +typedef void * zs_handle;
> > 
> 
> 
> Yes. I did it but I think we are (a) of chapter 5: Typedefs in Documentation/CodingStyle.
> 
>  (a) totally opaque objects (where the typedef is actively used to _hide_
>      what the object is).
> 
> No?

No.

Don't add new typedefs to the kernel.  Just use a structure if you need
to.

Vague "handles" are almost never what you want to do in Linux, sorry, I
can't take this patch.

greg k-h

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Minchan Kim]

On 05/10/2012 05:19 AM, Greg Kroah-Hartman wrote:

> On Fri, May 04, 2012 at 11:24:54AM +0900, Minchan Kim wrote:
>> On 05/04/2012 12:23 AM, Seth Jennings wrote:
>>
>>> On 05/03/2012 08:32 AM, Nitin Gupta wrote:
>>>
>>>> On 5/3/12 2:40 AM, Minchan Kim wrote:
>>>>> We should use zs_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.
>>>>>
>>>>> Cc: Dan Magenheimer<dan.magenheimer@oracle.com>
>>>>> Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
>>>>> Signed-off-by: Minchan Kim<minchan@kernel.org>
>>>>> ---
>>>>>   drivers/staging/zcache/zcache-main.c     |    8 ++++----
>>>>>   drivers/staging/zram/zram_drv.c          |    8 ++++----
>>>>>   drivers/staging/zram/zram_drv.h          |    2 +-
>>>>>   drivers/staging/zsmalloc/zsmalloc-main.c |   28
>>>>> ++++++++++++++--------------
>>>>>   drivers/staging/zsmalloc/zsmalloc.h      |   15 +++++++++++----
>>>>>   5 files changed, 34 insertions(+), 27 deletions(-)
>>>>
>>>> This was a long pending change. Thanks!
>>>
>>>
>>> The reason I hadn't done it before is that it introduces a checkpatch
>>> warning:
>>>
>>> WARNING: do not add new typedefs
>>> #303: FILE: drivers/staging/zsmalloc/zsmalloc.h:19:
>>> +typedef void * zs_handle;
>>>
>>
>>
>> Yes. I did it but I think we are (a) of chapter 5: Typedefs in Documentation/CodingStyle.
>>
>>  (a) totally opaque objects (where the typedef is actively used to _hide_
>>      what the object is).
>>
>> No?
> 
> No.
> 
> Don't add new typedefs to the kernel.  Just use a structure if you need
> to.


I tried it but failed because there were already tightly coupling between [zcache|zram]
and zsmalloc. 
They already knows handle's internal well so they used it as pointer, even zcache keeps
handle's value as some key in tmem_put and tmem_get
AFAIK, ramster also will use zsmalloc sooner or later and add more coupling codes. Sigh.
Please fix it as soon as possible.

Dan, Seth
Any ideas?

> 
> Vague "handles" are almost never what you want to do in Linux, sorry, I
> can't take this patch.
> 
> greg k-h
> 
> --
> 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 3/4] zsmalloc use zs_handle instead of void *

[Konrad Rzeszutek Wilk]

On Thu, May 10, 2012 at 11:03:19AM +0900, Minchan Kim wrote:
> On 05/10/2012 05:19 AM, Greg Kroah-Hartman wrote:
> 
> > On Fri, May 04, 2012 at 11:24:54AM +0900, Minchan Kim wrote:
> >> On 05/04/2012 12:23 AM, Seth Jennings wrote:
> >>
> >>> On 05/03/2012 08:32 AM, Nitin Gupta wrote:
> >>>
> >>>> On 5/3/12 2:40 AM, Minchan Kim wrote:
> >>>>> We should use zs_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.
> >>>>>
> >>>>> Cc: Dan Magenheimer<dan.magenheimer@oracle.com>
> >>>>> Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
> >>>>> Signed-off-by: Minchan Kim<minchan@kernel.org>
> >>>>> ---
> >>>>>   drivers/staging/zcache/zcache-main.c     |    8 ++++----
> >>>>>   drivers/staging/zram/zram_drv.c          |    8 ++++----
> >>>>>   drivers/staging/zram/zram_drv.h          |    2 +-
> >>>>>   drivers/staging/zsmalloc/zsmalloc-main.c |   28
> >>>>> ++++++++++++++--------------
> >>>>>   drivers/staging/zsmalloc/zsmalloc.h      |   15 +++++++++++----
> >>>>>   5 files changed, 34 insertions(+), 27 deletions(-)
> >>>>
> >>>> This was a long pending change. Thanks!
> >>>
> >>>
> >>> The reason I hadn't done it before is that it introduces a checkpatch
> >>> warning:
> >>>
> >>> WARNING: do not add new typedefs
> >>> #303: FILE: drivers/staging/zsmalloc/zsmalloc.h:19:
> >>> +typedef void * zs_handle;
> >>>
> >>
> >>
> >> Yes. I did it but I think we are (a) of chapter 5: Typedefs in Documentation/CodingStyle.
> >>
> >>  (a) totally opaque objects (where the typedef is actively used to _hide_
> >>      what the object is).
> >>
> >> No?
> > 
> > No.
> > 
> > Don't add new typedefs to the kernel.  Just use a structure if you need
> > to.
> 
> 
> I tried it but failed because there were already tightly coupling between [zcache|zram]
> and zsmalloc. 
> They already knows handle's internal well so they used it as pointer, even zcache keeps
> handle's value as some key in tmem_put and tmem_get
> AFAIK, ramster also will use zsmalloc sooner or later and add more coupling codes. Sigh.
> Please fix it as soon as possible.
> 
> Dan, Seth
> Any ideas?

struct zs {
	void *ptr;
};

And pass that structure around?

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Greg Kroah-Hartman]

On Thu, May 10, 2012 at 10:02:15AM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, May 10, 2012 at 11:03:19AM +0900, Minchan Kim wrote:
> > On 05/10/2012 05:19 AM, Greg Kroah-Hartman wrote:
> > 
> > > On Fri, May 04, 2012 at 11:24:54AM +0900, Minchan Kim wrote:
> > >> On 05/04/2012 12:23 AM, Seth Jennings wrote:
> > >>
> > >>> On 05/03/2012 08:32 AM, Nitin Gupta wrote:
> > >>>
> > >>>> On 5/3/12 2:40 AM, Minchan Kim wrote:
> > >>>>> We should use zs_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.
> > >>>>>
> > >>>>> Cc: Dan Magenheimer<dan.magenheimer@oracle.com>
> > >>>>> Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
> > >>>>> Signed-off-by: Minchan Kim<minchan@kernel.org>
> > >>>>> ---
> > >>>>>   drivers/staging/zcache/zcache-main.c     |    8 ++++----
> > >>>>>   drivers/staging/zram/zram_drv.c          |    8 ++++----
> > >>>>>   drivers/staging/zram/zram_drv.h          |    2 +-
> > >>>>>   drivers/staging/zsmalloc/zsmalloc-main.c |   28
> > >>>>> ++++++++++++++--------------
> > >>>>>   drivers/staging/zsmalloc/zsmalloc.h      |   15 +++++++++++----
> > >>>>>   5 files changed, 34 insertions(+), 27 deletions(-)
> > >>>>
> > >>>> This was a long pending change. Thanks!
> > >>>
> > >>>
> > >>> The reason I hadn't done it before is that it introduces a checkpatch
> > >>> warning:
> > >>>
> > >>> WARNING: do not add new typedefs
> > >>> #303: FILE: drivers/staging/zsmalloc/zsmalloc.h:19:
> > >>> +typedef void * zs_handle;
> > >>>
> > >>
> > >>
> > >> Yes. I did it but I think we are (a) of chapter 5: Typedefs in Documentation/CodingStyle.
> > >>
> > >>  (a) totally opaque objects (where the typedef is actively used to _hide_
> > >>      what the object is).
> > >>
> > >> No?
> > > 
> > > No.
> > > 
> > > Don't add new typedefs to the kernel.  Just use a structure if you need
> > > to.
> > 
> > 
> > I tried it but failed because there were already tightly coupling between [zcache|zram]
> > and zsmalloc. 
> > They already knows handle's internal well so they used it as pointer, even zcache keeps
> > handle's value as some key in tmem_put and tmem_get
> > AFAIK, ramster also will use zsmalloc sooner or later and add more coupling codes. Sigh.
> > Please fix it as soon as possible.
> > 
> > Dan, Seth
> > Any ideas?
> 
> struct zs {
> 	void *ptr;
> };
> 
> And pass that structure around?

With a better name, that would be fine.

greg k-h

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Nitin Gupta]

On 5/10/12 10:02 AM, Konrad Rzeszutek Wilk wrote:
> On Thu, May 10, 2012 at 11:03:19AM +0900, Minchan Kim wrote:
>> On 05/10/2012 05:19 AM, Greg Kroah-Hartman wrote:
>>
>>> On Fri, May 04, 2012 at 11:24:54AM +0900, Minchan Kim wrote:
>>>> On 05/04/2012 12:23 AM, Seth Jennings wrote:
>>>>
>>>>> On 05/03/2012 08:32 AM, Nitin Gupta wrote:
>>>>>
>>>>>> On 5/3/12 2:40 AM, Minchan Kim wrote:
>>>>>>> We should use zs_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.
>>>>>>>
>>>>>>> Cc: Dan Magenheimer<dan.magenheimer@oracle.com>
>>>>>>> Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
>>>>>>> Signed-off-by: Minchan Kim<minchan@kernel.org>
>>>>>>> ---
>>>>>>>    drivers/staging/zcache/zcache-main.c     |    8 ++++----
>>>>>>>    drivers/staging/zram/zram_drv.c          |    8 ++++----
>>>>>>>    drivers/staging/zram/zram_drv.h          |    2 +-
>>>>>>>    drivers/staging/zsmalloc/zsmalloc-main.c |   28
>>>>>>> ++++++++++++++--------------
>>>>>>>    drivers/staging/zsmalloc/zsmalloc.h      |   15 +++++++++++----
>>>>>>>    5 files changed, 34 insertions(+), 27 deletions(-)
>>>>>>
>>>>>> This was a long pending change. Thanks!
>>>>>
>>>>>
>>>>> The reason I hadn't done it before is that it introduces a checkpatch
>>>>> warning:
>>>>>
>>>>> WARNING: do not add new typedefs
>>>>> #303: FILE: drivers/staging/zsmalloc/zsmalloc.h:19:
>>>>> +typedef void * zs_handle;
>>>>>
>>>>
>>>>
>>>> Yes. I did it but I think we are (a) of chapter 5: Typedefs in Documentation/CodingStyle.
>>>>
>>>>   (a) totally opaque objects (where the typedef is actively used to _hide_
>>>>       what the object is).
>>>>
>>>> No?
>>>
>>> No.
>>>
>>> Don't add new typedefs to the kernel.  Just use a structure if you need
>>> to.
>>
>>
>> I tried it but failed because there were already tightly coupling between [zcache|zram]
>> and zsmalloc.
>> They already knows handle's internal well so they used it as pointer, even zcache keeps
>> handle's value as some key in tmem_put and tmem_get
>> AFAIK, ramster also will use zsmalloc sooner or later and add more coupling codes. Sigh.
>> Please fix it as soon as possible.
>>
>> Dan, Seth
>> Any ideas?
>
> struct zs {
> 	void *ptr;
> };
>
> And pass that structure around?
>

A minor problem is that we store this handle value in a radix tree node. 
If we wrap it as a struct, then we will not be able to store it directly 
in the node -- the node will have to point to a 'struct zs'. This will 
unnecessarily waste sizeof(void *) for every object stored.

We could 'memcpy' struct zs to a void * and then store that directly in 
the radix node but not sure if that would be less ugly than just 
returning the handle as a void * as is done currently.

Thanks,
Nitin

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Minchan Kim]

On Thu, May 10, 2012 at 10:47:31AM -0400, Nitin Gupta wrote:
> On 5/10/12 10:02 AM, Konrad Rzeszutek Wilk wrote:
> >On Thu, May 10, 2012 at 11:03:19AM +0900, Minchan Kim wrote:
> >>On 05/10/2012 05:19 AM, Greg Kroah-Hartman wrote:
> >>
> >>>On Fri, May 04, 2012 at 11:24:54AM +0900, Minchan Kim wrote:
> >>>>On 05/04/2012 12:23 AM, Seth Jennings wrote:
> >>>>
> >>>>>On 05/03/2012 08:32 AM, Nitin Gupta wrote:
> >>>>>
> >>>>>>On 5/3/12 2:40 AM, Minchan Kim wrote:
> >>>>>>>We should use zs_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.
> >>>>>>>
> >>>>>>>Cc: Dan Magenheimer<dan.magenheimer@oracle.com>
> >>>>>>>Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
> >>>>>>>Signed-off-by: Minchan Kim<minchan@kernel.org>
> >>>>>>>---
> >>>>>>>   drivers/staging/zcache/zcache-main.c     |    8 ++++----
> >>>>>>>   drivers/staging/zram/zram_drv.c          |    8 ++++----
> >>>>>>>   drivers/staging/zram/zram_drv.h          |    2 +-
> >>>>>>>   drivers/staging/zsmalloc/zsmalloc-main.c |   28
> >>>>>>>++++++++++++++--------------
> >>>>>>>   drivers/staging/zsmalloc/zsmalloc.h      |   15 +++++++++++----
> >>>>>>>   5 files changed, 34 insertions(+), 27 deletions(-)
> >>>>>>
> >>>>>>This was a long pending change. Thanks!
> >>>>>
> >>>>>
> >>>>>The reason I hadn't done it before is that it introduces a checkpatch
> >>>>>warning:
> >>>>>
> >>>>>WARNING: do not add new typedefs
> >>>>>#303: FILE: drivers/staging/zsmalloc/zsmalloc.h:19:
> >>>>>+typedef void * zs_handle;
> >>>>>
> >>>>
> >>>>
> >>>>Yes. I did it but I think we are (a) of chapter 5: Typedefs in Documentation/CodingStyle.
> >>>>
> >>>>  (a) totally opaque objects (where the typedef is actively used to _hide_
> >>>>      what the object is).
> >>>>
> >>>>No?
> >>>
> >>>No.
> >>>
> >>>Don't add new typedefs to the kernel.  Just use a structure if you need
> >>>to.
> >>
> >>
> >>I tried it but failed because there were already tightly coupling between [zcache|zram]
> >>and zsmalloc.
> >>They already knows handle's internal well so they used it as pointer, even zcache keeps
> >>handle's value as some key in tmem_put and tmem_get
> >>AFAIK, ramster also will use zsmalloc sooner or later and add more coupling codes. Sigh.
> >>Please fix it as soon as possible.
> >>
> >>Dan, Seth
> >>Any ideas?
> >
> >struct zs {

I like struct zs_handle.

> >	void *ptr;
> >};
> >
> >And pass that structure around?
> >
> 
> A minor problem is that we store this handle value in a radix tree
> node. If we wrap it as a struct, then we will not be able to store
> it directly in the node -- the node will have to point to a 'struct

That was my point and I think it's not minor problem.

> zs'. This will unnecessarily waste sizeof(void *) for every object
> stored.
> 
> We could 'memcpy' struct zs to a void * and then store that directly

I don't like it because it still coupled with zsmalloc which means
zcache already know zs's internal so we should avoid it.

> in the radix node but not sure if that would be less ugly than just
> returning the handle as a void * as is done currently.
> 
> Thanks,
> Nitin

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Seth Jennings]

On 05/10/2012 09:47 AM, Nitin Gupta wrote:

> On 5/10/12 10:02 AM, Konrad Rzeszutek Wilk wrote:
>> struct zs {
>>     void *ptr;
>> };
>>
>> And pass that structure around?
>>
> 
> A minor problem is that we store this handle value in a radix tree node.
> If we wrap it as a struct, then we will not be able to store it directly
> in the node -- the node will have to point to a 'struct zs'. This will
> unnecessarily waste sizeof(void *) for every object stored.


I don't think so. You can use the fact that for a struct zs var, &var
and &var->ptr are the same.

For the structure above:

void * zs_to_void(struct zs *p) { return p->ptr; }
struct zs * void_to_zs(void *p) { return (struct zs *)p; }

Right?

--
Seth

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Minchan Kim]

On Thu, May 10, 2012 at 10:11:27AM -0500, Seth Jennings wrote:
> On 05/10/2012 09:47 AM, Nitin Gupta wrote:
> 
> > On 5/10/12 10:02 AM, Konrad Rzeszutek Wilk wrote:
> >> struct zs {
> >>     void *ptr;
> >> };
> >>
> >> And pass that structure around?
> >>
> > 
> > A minor problem is that we store this handle value in a radix tree node.
> > If we wrap it as a struct, then we will not be able to store it directly
> > in the node -- the node will have to point to a 'struct zs'. This will
> > unnecessarily waste sizeof(void *) for every object stored.
> 
> 
> I don't think so. You can use the fact that for a struct zs var, &var
> and &var->ptr are the same.
> 
> For the structure above:
> 
> void * zs_to_void(struct zs *p) { return p->ptr; }
> struct zs * void_to_zs(void *p) { return (struct zs *)p; }
> 
> Right?

I though this, too but didn't tried it.
We DO REALLY want it?
Why should zsmalloc support like such strange interface?
I want to solve the problem in zcache, not with zsmalloc.

> 
> --
> Seth
> 

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Greg Kroah-Hartman]

On Thu, May 10, 2012 at 10:11:27AM -0500, Seth Jennings wrote:
> On 05/10/2012 09:47 AM, Nitin Gupta wrote:
> 
> > On 5/10/12 10:02 AM, Konrad Rzeszutek Wilk wrote:
> >> struct zs {
> >>     void *ptr;
> >> };
> >>
> >> And pass that structure around?
> >>
> > 
> > A minor problem is that we store this handle value in a radix tree node.
> > If we wrap it as a struct, then we will not be able to store it directly
> > in the node -- the node will have to point to a 'struct zs'. This will
> > unnecessarily waste sizeof(void *) for every object stored.
> 
> 
> I don't think so. You can use the fact that for a struct zs var, &var
> and &var->ptr are the same.
> 
> For the structure above:
> 
> void * zs_to_void(struct zs *p) { return p->ptr; }
> struct zs * void_to_zs(void *p) { return (struct zs *)p; }

Do like what the rest of the kernel does and pass around *ptr and use
container_of to get 'struct zs'.  Yes, they resolve to the same pointer
right now, but you shouldn't "expect" to to be the same.

greg k-h

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Nitin Gupta]

On 5/10/12 11:19 AM, Greg Kroah-Hartman wrote:
> On Thu, May 10, 2012 at 10:11:27AM -0500, Seth Jennings wrote:
>> On 05/10/2012 09:47 AM, Nitin Gupta wrote:
>>
>>> On 5/10/12 10:02 AM, Konrad Rzeszutek Wilk wrote:
>>>> struct zs {
>>>>      void *ptr;
>>>> };
>>>>
>>>> And pass that structure around?
>>>>
>>>
>>> A minor problem is that we store this handle value in a radix tree node.
>>> If we wrap it as a struct, then we will not be able to store it directly
>>> in the node -- the node will have to point to a 'struct zs'. This will
>>> unnecessarily waste sizeof(void *) for every object stored.
>>
>>
>> I don't think so. You can use the fact that for a struct zs var,&var
>> and&var->ptr are the same.
>>
>> For the structure above:
>>
>> void * zs_to_void(struct zs *p) { return p->ptr; }
>> struct zs * void_to_zs(void *p) { return (struct zs *)p; }
>
> Do like what the rest of the kernel does and pass around *ptr and use
> container_of to get 'struct zs'.  Yes, they resolve to the same pointer
> right now, but you shouldn't "expect" to to be the same.
>
>

I think we can just use unsigned long as zs handle type since all we 
have to do is tell the user that the returned value is not a pointer. 
This will be less pretty than a typedef but still better than a single 
entry struct + container_of stuff.

Thanks,
Nitin

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Greg Kroah-Hartman]

On Thu, May 10, 2012 at 12:29:41PM -0400, Nitin Gupta wrote:
> On 5/10/12 11:19 AM, Greg Kroah-Hartman wrote:
> >On Thu, May 10, 2012 at 10:11:27AM -0500, Seth Jennings wrote:
> >>On 05/10/2012 09:47 AM, Nitin Gupta wrote:
> >>
> >>>On 5/10/12 10:02 AM, Konrad Rzeszutek Wilk wrote:
> >>>>struct zs {
> >>>>     void *ptr;
> >>>>};
> >>>>
> >>>>And pass that structure around?
> >>>>
> >>>
> >>>A minor problem is that we store this handle value in a radix tree node.
> >>>If we wrap it as a struct, then we will not be able to store it directly
> >>>in the node -- the node will have to point to a 'struct zs'. This will
> >>>unnecessarily waste sizeof(void *) for every object stored.
> >>
> >>
> >>I don't think so. You can use the fact that for a struct zs var,&var
> >>and&var->ptr are the same.
> >>
> >>For the structure above:
> >>
> >>void * zs_to_void(struct zs *p) { return p->ptr; }
> >>struct zs * void_to_zs(void *p) { return (struct zs *)p; }
> >
> >Do like what the rest of the kernel does and pass around *ptr and use
> >container_of to get 'struct zs'.  Yes, they resolve to the same pointer
> >right now, but you shouldn't "expect" to to be the same.
> >
> >
> 
> I think we can just use unsigned long as zs handle type since all we
> have to do is tell the user that the returned value is not a
> pointer. This will be less pretty than a typedef but still better
> than a single entry struct + container_of stuff.

But then you are casting the thing all around just as much as you were
with the void *, right?

Making this a "real" structure ensures type safety and lets the compiler
find the problems you accidentally create at times :)

thanks,

greg k-h

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Nitin Gupta]

On 5/10/12 12:44 PM, Greg Kroah-Hartman wrote:
> On Thu, May 10, 2012 at 12:29:41PM -0400, Nitin Gupta wrote:
>> On 5/10/12 11:19 AM, Greg Kroah-Hartman wrote:
>>> On Thu, May 10, 2012 at 10:11:27AM -0500, Seth Jennings wrote:
>>>> On 05/10/2012 09:47 AM, Nitin Gupta wrote:
>>>>
>>>>> On 5/10/12 10:02 AM, Konrad Rzeszutek Wilk wrote:
>>>>>> struct zs {
>>>>>>      void *ptr;
>>>>>> };
>>>>>>
>>>>>> And pass that structure around?
>>>>>>
>>>>>
>>>>> A minor problem is that we store this handle value in a radix tree node.
>>>>> If we wrap it as a struct, then we will not be able to store it directly
>>>>> in the node -- the node will have to point to a 'struct zs'. This will
>>>>> unnecessarily waste sizeof(void *) for every object stored.
>>>>
>>>>
>>>> I don't think so. You can use the fact that for a struct zs var,&var
>>>> and&var->ptr are the same.
>>>>
>>>> For the structure above:
>>>>
>>>> void * zs_to_void(struct zs *p) { return p->ptr; }
>>>> struct zs * void_to_zs(void *p) { return (struct zs *)p; }
>>>
>>> Do like what the rest of the kernel does and pass around *ptr and use
>>> container_of to get 'struct zs'.  Yes, they resolve to the same pointer
>>> right now, but you shouldn't "expect" to to be the same.
>>>
>>>
>>
>> I think we can just use unsigned long as zs handle type since all we
>> have to do is tell the user that the returned value is not a
>> pointer. This will be less pretty than a typedef but still better
>> than a single entry struct + container_of stuff.
>
> But then you are casting the thing all around just as much as you were
> with the void *, right?
>
> Making this a "real" structure ensures type safety and lets the compiler
> find the problems you accidentally create at times :)
>

If we return a 'struct zs' from zs_malloc then I cannot see how we are 
solving the original problem of storing the handle directly in a radix 
node. If we pass a struct zs we will require pointing radix node to this 
struct, wasting sizeof(void *) for every object.   If we pass unsigned 
long, then this problem is solved and it also makes it clear that the 
passed value is not a pointer.

Its true that making it a real struct would prevent accidental casts to 
void * but due to the above problem, I think we have to stick with 
unsigned long.

Thanks,
Nitin

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Konrad Rzeszutek Wilk]

On Thu, May 10, 2012 at 01:24:36PM -0400, Nitin Gupta wrote:
> On 5/10/12 12:44 PM, Greg Kroah-Hartman wrote:
> >On Thu, May 10, 2012 at 12:29:41PM -0400, Nitin Gupta wrote:
> >>On 5/10/12 11:19 AM, Greg Kroah-Hartman wrote:
> >>>On Thu, May 10, 2012 at 10:11:27AM -0500, Seth Jennings wrote:
> >>>>On 05/10/2012 09:47 AM, Nitin Gupta wrote:
> >>>>
> >>>>>On 5/10/12 10:02 AM, Konrad Rzeszutek Wilk wrote:
> >>>>>>struct zs {
> >>>>>>     void *ptr;
> >>>>>>};
> >>>>>>
> >>>>>>And pass that structure around?
> >>>>>>
> >>>>>
> >>>>>A minor problem is that we store this handle value in a radix tree node.
> >>>>>If we wrap it as a struct, then we will not be able to store it directly
> >>>>>in the node -- the node will have to point to a 'struct zs'. This will
> >>>>>unnecessarily waste sizeof(void *) for every object stored.
> >>>>
> >>>>
> >>>>I don't think so. You can use the fact that for a struct zs var,&var
> >>>>and&var->ptr are the same.
> >>>>
> >>>>For the structure above:
> >>>>
> >>>>void * zs_to_void(struct zs *p) { return p->ptr; }
> >>>>struct zs * void_to_zs(void *p) { return (struct zs *)p; }
> >>>
> >>>Do like what the rest of the kernel does and pass around *ptr and use
> >>>container_of to get 'struct zs'.  Yes, they resolve to the same pointer
> >>>right now, but you shouldn't "expect" to to be the same.
> >>>
> >>>
> >>
> >>I think we can just use unsigned long as zs handle type since all we
> >>have to do is tell the user that the returned value is not a
> >>pointer. This will be less pretty than a typedef but still better
> >>than a single entry struct + container_of stuff.
> >
> >But then you are casting the thing all around just as much as you were
> >with the void *, right?
> >
> >Making this a "real" structure ensures type safety and lets the compiler
> >find the problems you accidentally create at times :)
> >
> 
> If we return a 'struct zs' from zs_malloc then I cannot see how we
> are solving the original problem of storing the handle directly in a
> radix node. If we pass a struct zs we will require pointing radix
> node to this struct, wasting sizeof(void *) for every object.   If
> we pass unsigned long, then this problem is solved and it also makes
> it clear that the passed value is not a pointer.

It is the same size: sizeof(struct zs) == sizeof(void *).
When you return the 'struct zs' it will be as if you are returning 
a void * pointer.

> 
> Its true that making it a real struct would prevent accidental casts
> to void * but due to the above problem, I think we have to stick
> with unsigned long.
> 
> Thanks,
> Nitin

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Minchan Kim]

On 05/11/2012 02:33 AM, Konrad Rzeszutek Wilk wrote:

> On Thu, May 10, 2012 at 01:24:36PM -0400, Nitin Gupta wrote:
>> On 5/10/12 12:44 PM, Greg Kroah-Hartman wrote:
>>> On Thu, May 10, 2012 at 12:29:41PM -0400, Nitin Gupta wrote:
>>>> On 5/10/12 11:19 AM, Greg Kroah-Hartman wrote:
>>>>> On Thu, May 10, 2012 at 10:11:27AM -0500, Seth Jennings wrote:
>>>>>> On 05/10/2012 09:47 AM, Nitin Gupta wrote:
>>>>>>
>>>>>>> On 5/10/12 10:02 AM, Konrad Rzeszutek Wilk wrote:
>>>>>>>> struct zs {
>>>>>>>>     void *ptr;
>>>>>>>> };
>>>>>>>>
>>>>>>>> And pass that structure around?
>>>>>>>>
>>>>>>>
>>>>>>> A minor problem is that we store this handle value in a radix tree node.
>>>>>>> If we wrap it as a struct, then we will not be able to store it directly
>>>>>>> in the node -- the node will have to point to a 'struct zs'. This will
>>>>>>> unnecessarily waste sizeof(void *) for every object stored.
>>>>>>
>>>>>>
>>>>>> I don't think so. You can use the fact that for a struct zs var,&var
>>>>>> and&var->ptr are the same.
>>>>>>
>>>>>> For the structure above:
>>>>>>
>>>>>> void * zs_to_void(struct zs *p) { return p->ptr; }
>>>>>> struct zs * void_to_zs(void *p) { return (struct zs *)p; }
>>>>>
>>>>> Do like what the rest of the kernel does and pass around *ptr and use
>>>>> container_of to get 'struct zs'.  Yes, they resolve to the same pointer
>>>>> right now, but you shouldn't "expect" to to be the same.
>>>>>
>>>>>
>>>>
>>>> I think we can just use unsigned long as zs handle type since all we
>>>> have to do is tell the user that the returned value is not a
>>>> pointer. This will be less pretty than a typedef but still better
>>>> than a single entry struct + container_of stuff.
>>>
>>> But then you are casting the thing all around just as much as you were
>>> with the void *, right?
>>>
>>> Making this a "real" structure ensures type safety and lets the compiler
>>> find the problems you accidentally create at times :)
>>>
>>
>> If we return a 'struct zs' from zs_malloc then I cannot see how we
>> are solving the original problem of storing the handle directly in a
>> radix node. If we pass a struct zs we will require pointing radix
>> node to this struct, wasting sizeof(void *) for every object.   If
>> we pass unsigned long, then this problem is solved and it also makes
>> it clear that the passed value is not a pointer.
> 
> It is the same size: sizeof(struct zs) == sizeof(void *).
> When you return the 'struct zs' it will be as if you are returning 
> a void * pointer.
> 


Please look.

struct zs_handle {
	void *handle
};

1)

static struct zv_hdr *zv_create(..)
{
	struct zs_handle handle;
	..
	handle = zs_malloc(pool, size);
	..
	return handle;
}

handle is on stack so it can't be used by index for slot of radix tree.

2)

static struct zv_hdr *zv_create(..)
{
	struct zs_handle handle;
	..
	handle = zs_malloc(pool, size);
	..
	return handle.handle;
}

Okay. Now it works but zcache coupled with zsmalloc tightly.
User of zsmalloc should never know internal of zs_handle.

3)

- zsmalloc.h
void *zs_handle_to_ptr(struct zs_handle handle)
{
	return handle.hanle;
}

static struct zv_hdr *zv_create(..)
{
	struct zs_handle handle;
	..
	handle = zs_malloc(pool, size);
	..
	return zs_handle_to_ptr(handle);
}

Why should zsmalloc support such interface?
It's a zcache problem so it's desriable to solve it in zcache internal.
And in future, if we can add/remove zs_handle's fields, we can't make
sure such API.


>> Its true that making it a real struct would prevent accidental casts
>> to void * but due to the above problem, I think we have to stick
>> with unsigned long.
>>
>> Thanks,
>> Nitin
> 
> --
> 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 3/4] zsmalloc use zs_handle instead of void *

[Dan Magenheimer]

> From: Minchan Kim [mailto:minchan@kernel.org]
> 
> Okay. Now it works but zcache coupled with zsmalloc tightly.
> User of zsmalloc should never know internal of zs_handle.
> 
> 3)
> 
> - zsmalloc.h
> void *zs_handle_to_ptr(struct zs_handle handle)
> {
> 	return handle.hanle;
> }
> 
> static struct zv_hdr *zv_create(..)
> {
> 	struct zs_handle handle;
> 	..
> 	handle = zs_malloc(pool, size);
> 	..
> 	return zs_handle_to_ptr(handle);
> }
> 
> Why should zsmalloc support such interface?
> It's a zcache problem so it's desriable to solve it in zcache internal.
> And in future, if we can add/remove zs_handle's fields, we can't make
> sure such API.

Hi Minchan --

I'm confused so maybe I am misunderstanding or you can
explain further.  It seems like you are trying to redesign
zsmalloc so that it can be a pure abstraction in a library.
While I understand and value abstractions in software
designs, the primary use now of zsmalloc is in zcache.  If
there are other users that require a different interface
or a more precise abstract API, zsmalloc could then
evolve to meet the needs of multiple users.  But I think
zcache is going to need more access to the internals
of its allocator, not less.  Zsmalloc is currently missing
some important functionality that (I believe) will be
necessary to turn zcache into an enterprise-ready,
always-on kernel feature.  If it evolves to add that
functionality, then it may no longer be able to provide
generic abstract access... in which case generic zsmalloc
may then have zero users in the kernel.

So I'd suggest we hold off on trying to make zsmalloc
"pretty" until we better understand how it will be used
by zcache (and ramster) and, if there are any, any future
users.

That's just my opinion...
Dan

RE: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Dan Magenheimer]

> From: Minchan Kim [mailto:minchan@kernel.org]
> Subject: Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size
> 
> On 05/08/2012 11:00 PM, Dan Magenheimer wrote:
> 
> >> From: Minchan Kim [mailto:minchan@kernel.org]
> >>> zcache can potentially create a lot of pools, so the latter will save
> >>> some memory.
> >>
> >>
> >> Dumb question.
> >> Why should we create pool per user?
> >> What's the problem if there is only one pool in system?
> >
> > zcache doesn't use zsmalloc for cleancache pages today, but
> > that's Seth's plan for the future.  Then if there is a
> > separate pool for each cleancache pool, when a filesystem
> > is umount'ed, it isn't necessary to walk through and delete
> > all pages one-by-one, which could take quite awhile.
> 
> >
> 
> > ramster needs one pool for each client (i.e. machine in the
> > cluster) for frontswap pages for the same reason, and
> > later, for cleancache pages, one per mounted filesystem
> > per client
> 
> 
> Fair enough.
> But some subsystems can't want a own pool for not waste unnecessary memory.
> 
> Then, how about this interfaces like slab?
> 
> 1. zs_handle zs_malloc(size_t size, gfp_t flags) - share a pool by many subsystem(like kmalloc)
> 2. zs_handle zs_malloc_pool(struct zs_pool *pool, size_t size) - use own pool(like kmem_cache_alloc)
> 
> Any thoughts?

I don't have any objections to adding this kind of
capability to zsmalloc.  But since we are just speculating
that this capability would be used by some future
kernel subsystem, isn't it normal kernel protocol for
this new capability NOT to be added until that future
kernel subsystem creates a need for it.

As I said in reply to the other thread, there is missing
functionality in zsmalloc that is making it difficult for
it to be used by zcache.  It would be good if Seth
and Nitin (and any other kernel developers) would work
on those issues before adding capabilities for non-existent
future users of zsmalloc.

Again, that's just my opinion.
Dan

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Minchan Kim]

Hi Dan,

On 05/11/2012 08:50 AM, Dan Magenheimer wrote:

>> From: Minchan Kim [mailto:minchan@kernel.org]
>>
>> Okay. Now it works but zcache coupled with zsmalloc tightly.
>> User of zsmalloc should never know internal of zs_handle.
>>
>> 3)
>>
>> - zsmalloc.h
>> void *zs_handle_to_ptr(struct zs_handle handle)
>> {
>> 	return handle.hanle;
>> }
>>
>> static struct zv_hdr *zv_create(..)
>> {
>> 	struct zs_handle handle;
>> 	..
>> 	handle = zs_malloc(pool, size);
>> 	..
>> 	return zs_handle_to_ptr(handle);
>> }
>>
>> Why should zsmalloc support such interface?
>> It's a zcache problem so it's desriable to solve it in zcache internal.
>> And in future, if we can add/remove zs_handle's fields, we can't make
>> sure such API.
> 
> Hi Minchan --
> 
> I'm confused so maybe I am misunderstanding or you can
> explain further.  It seems like you are trying to redesign
> zsmalloc so that it can be a pure abstraction in a library.
> While I understand and value abstractions in software
> designs, the primary use now of zsmalloc is in zcache.  If

> there are other users that require a different interface
> or a more precise abstract API, zsmalloc could then
> evolve to meet the needs of multiple users.  But I think


At least, zram is also primary user and it also has such mess although it's not severe than zcache. zram->table[index].handle sometime has real (void*) handle, sometime (struct page*).
And I assume ramster you sent yesterday will be.

I think there are already many mess and I bet it will prevent going to mainline.
Especially, handle problem is severe because it a arguement of most functions exported in zsmalloc
So, we should clean up before late, IMHO.

> zcache is going to need more access to the internals
> of its allocator, not less.  Zsmalloc is currently missing
> some important functionality that (I believe) will be
> necessary to turn zcache into an enterprise-ready,


If you have such TODO list, could you post it?
It helps direction point of my stuff.

> always-on kernel feature.  If it evolves to add that
> functionality, then it may no longer be able to provide
> generic abstract access... in which case generic zsmalloc
> may then have zero users in the kernel.


Hmm, Do you want to make zsmalloc by zcache owned private allocator?

> 
> So I'd suggest we hold off on trying to make zsmalloc
> "pretty" until we better understand how it will be used
> by zcache (and ramster) and, if there are any, any future
> users.


zcache isn't urgent? I'm okay about zcache but at least, zram is when it merged into mainline, I think.
Many embedded system have a advantage with it so I hope we finish zsmalloc mess as soon as possble.

> 
> That's just my opinion...


Dan, Thanks for sharing your opinion.

> Dan
> 
> --
> 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=ilto:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Minchan Kim]

On 05/11/2012 09:03 AM, Dan Magenheimer wrote:

>> From: Minchan Kim [mailto:minchan@kernel.org]
>> Subject: Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size
>>
>> On 05/08/2012 11:00 PM, Dan Magenheimer wrote:
>>
>>>> From: Minchan Kim [mailto:minchan@kernel.org]
>>>>> zcache can potentially create a lot of pools, so the latter will save
>>>>> some memory.
>>>>
>>>>
>>>> Dumb question.
>>>> Why should we create pool per user?
>>>> What's the problem if there is only one pool in system?
>>>
>>> zcache doesn't use zsmalloc for cleancache pages today, but
>>> that's Seth's plan for the future.  Then if there is a
>>> separate pool for each cleancache pool, when a filesystem
>>> is umount'ed, it isn't necessary to walk through and delete
>>> all pages one-by-one, which could take quite awhile.
>>
>>>
>>
>>> ramster needs one pool for each client (i.e. machine in the
>>> cluster) for frontswap pages for the same reason, and
>>> later, for cleancache pages, one per mounted filesystem
>>> per client
>>
>>
>> Fair enough.
>> But some subsystems can't want a own pool for not waste unnecessary memory.
>>
>> Then, how about this interfaces like slab?
>>
>> 1. zs_handle zs_malloc(size_t size, gfp_t flags) - share a pool by many subsystem(like kmalloc)
>> 2. zs_handle zs_malloc_pool(struct zs_pool *pool, size_t size) - use own pool(like kmem_cache_alloc)
>>
>> Any thoughts?
> 
> I don't have any objections to adding this kind of
> capability to zsmalloc.  But since we are just speculating
> that this capability would be used by some future
> kernel subsystem, isn't it normal kernel protocol for
> this new capability NOT to be added until that future
> kernel subsystem creates a need for it.


Now zram makes pool per block device and a embedded system may use zram
for several block device, ex) swap device, several compressed tmpfs
In such case, share pool is better than private pool because embedded system
don't mount/umount frequently on such directories since booting.

> 
> As I said in reply to the other thread, there is missing
> functionality in zsmalloc that is making it difficult for
> it to be used by zcache.  It would be good if Seth
> and Nitin (and any other kernel developers) would work


So, if you guys post TODO list, it helps fix the direction.

> on those issues before adding capabilities for non-existent
> future users of zsmalloc.


I think it's not urgent than zs_handle mess.

> 
> Again, that's just my opinion.

> Dan
> 
> --
> 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=ilto:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

RE: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Dan Magenheimer]

> From: Minchan Kim [mailto:minchan@kernel.org]
> Subject: Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *
> 
> Hi Dan,
> 
> At least, zram is also primary user and it also has such mess although it's not severe than zcache.
> zram->table[index].handle sometime has real (void*) handle, sometime (struct page*).
> And I assume ramster you sent yesterday will be.
> 
> I think there are already many mess and I bet it will prevent going to mainline.
> Especially, handle problem is severe because it a arguement of most functions exported in zsmalloc
> So, we should clean up before late, IMHO.
> 
> > zcache is going to need more access to the internals
> > of its allocator, not less.  Zsmalloc is currently missing
> > some important functionality that (I believe) will be
> > necessary to turn zcache into an enterprise-ready,
> 
> If you have such TODO list, could you post it?
> It helps direction point of my stuff.

Will you be proposing to promote zram and zsmalloc out of staging
for the upcoming window?  If so, I will try to make some time
for this.  Otherwise, I apologize, but I will need to
wait a week or two (after the upcoming window) when I will
have more time.
 
> > always-on kernel feature.  If it evolves to add that
> > functionality, then it may no longer be able to provide
> > generic abstract access... in which case generic zsmalloc
> > may then have zero users in the kernel.
> 
> Hmm, Do you want to make zsmalloc by zcache owned private allocator?

I would prefer to use only zsmalloc, but it currently cannot provide
all the functionality of "zbud" which is a private allocator in
zcache and ramster.  I'll explain more later.

Dan

Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Konrad Rzeszutek Wilk]

> >> 1. zs_handle zs_malloc(size_t size, gfp_t flags) - share a pool by many subsystem(like kmalloc)
> >> 2. zs_handle zs_malloc_pool(struct zs_pool *pool, size_t size) - use own pool(like kmem_cache_alloc)
> >>
> >> Any thoughts?
> > 
> > I don't have any objections to adding this kind of
> > capability to zsmalloc.  But since we are just speculating
> > that this capability would be used by some future
> > kernel subsystem, isn't it normal kernel protocol for
> > this new capability NOT to be added until that future
> > kernel subsystem creates a need for it.
> 
> 
> Now zram makes pool per block device and a embedded system may use zram
> for several block device, ex) swap device, several compressed tmpfs
> In such case, share pool is better than private pool because embedded system
> don't mount/umount frequently on such directories since booting.
> 
> > 
> > As I said in reply to the other thread, there is missing
> > functionality in zsmalloc that is making it difficult for
> > it to be used by zcache.  It would be good if Seth
> > and Nitin (and any other kernel developers) would work
> 
> 
> So, if you guys post TODO list, it helps fix the direction.
> 
> > on those issues before adding capabilities for non-existent
> > future users of zsmalloc.
> 
> 
> I think it's not urgent than zs_handle mess.

I am having a hard time parsing that. Are you saying that
this is not as important as the zs_handle fixup? I think
that is what you meant, but what to make sure.

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Konrad Rzeszutek Wilk]

> Please look.
> 
> struct zs_handle {
> 	void *handle
> };
> 
> 1)
> 
> static struct zv_hdr *zv_create(..)
> {
> 	struct zs_handle handle;
> 	..
> 	handle = zs_malloc(pool, size);
> 	..
> 	return handle;

Compiler will complain that you are returning incorrect type.

> }
> 
> handle is on stack so it can't be used by index for slot of radix tree.

The fix is of course to return a pointer (which your function
declared), and instead do this:

{
	struct zs_handle *handle;

	handle = zs_malloc(pool, size);
	return handle;
}

> 
> 2)
> 
> static struct zv_hdr *zv_create(..)
> {
> 	struct zs_handle handle;
> 	..
> 	handle = zs_malloc(pool, size);
> 	..
> 	return handle.handle;
> }
> 
> Okay. Now it works but zcache coupled with zsmalloc tightly.
> User of zsmalloc should never know internal of zs_handle.

OK. Then it can just forward declare it:

struct zs_handle;

and zsmalloc will treat it as an opaque pointer.

> 
> 3)
> 
> - zsmalloc.h
> void *zs_handle_to_ptr(struct zs_handle handle)
> {
> 	return handle.hanle;
> }
> 
> static struct zv_hdr *zv_create(..)
> {
> 	struct zs_handle handle;
> 	..
> 	handle = zs_malloc(pool, size);
> 	..
> 	return zs_handle_to_ptr(handle);

> }

> 
> Why should zsmalloc support such interface?

Why not? It is better than a 'void *' or a typedef.

It is modeled after a pte_t.


> It's a zcache problem so it's desriable to solve it in zcache internal.

Not really. We shouldn't really pass any 'void *' pointers around.

> And in future, if we can add/remove zs_handle's fields, we can't make
> sure such API.

Meaning ... what exactly do you mean? That the size of the structure
will change and we won't return the right value? Why not?
If you use the 'zs_handle_to_ptr' won't that work? Especially if you
add new values to the end of the struct it won't cause issues.

> 
> 
> >> Its true that making it a real struct would prevent accidental casts
> >> to void * but due to the above problem, I think we have to stick
> >> with unsigned long.

So the problem you are seeing is that you don't want 'struct zs_handle'
be present in the drivers/staging/zsmalloc/zsmalloc.h header file?
It looks like the proper place.

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Konrad Rzeszutek Wilk]

> > there are other users that require a different interface
> > or a more precise abstract API, zsmalloc could then
> > evolve to meet the needs of multiple users.  But I think
> 
> 
> At least, zram is also primary user and it also has such mess although it's not severe than zcache. zram->table[index].handle sometime has real (void*) handle, sometime (struct page*).

Yikes. Yeah that needs to be fixed.

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Seth Jennings]

On 05/11/2012 02:29 PM, Konrad Rzeszutek Wilk wrote:

>> At least, zram is also primary user and it also has such mess
>> although it's not severe than zcache. zram->table[index].handle
>> sometime has real (void*) handle, sometime (struct page*).
> 
> Yikes. Yeah that needs to be fixed.
> 


How about this (untested)?  Changes to zram_bvec_write() are a little
hard to make out in this format.  There are a couple of checkpatch fixes
(two split line strings) and an unused variable store_offset removal mixed
in too. If this patch is good, I'll break them up for official submission
after I test.

diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
index fbe8ac9..10dcd99 100644
--- a/drivers/staging/zram/zram_drv.h
+++ b/drivers/staging/zram/zram_drv.h
@@ -81,7 +81,10 @@ enum zram_pageflags {
 
 /* Allocated for each disk page */
 struct table {
-	void *handle;
+	union {
+		void *handle; /* compressible */
+		struct page *page; /* incompressible */
+	};
 	u16 size;	/* object size (excluding header) */
 	u8 count;	/* object ref count (not yet used) */
 	u8 flags;
diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 685d612..d49deca 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -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(zram->table[index].page);
 		zram_clear_flag(zram, index, ZRAM_UNCOMPRESSED);
 		zram_stat_dec(&zram->stats.pages_expand);
 		goto out;
@@ -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(zram->table[index].page);
 
 	memcpy(user_mem + bvec->bv_offset, cmem + offset, bvec->bv_len);
 	kunmap_atomic(cmem);
@@ -315,7 +315,6 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 			   int offset)
 {
 	int ret;
-	u32 store_offset;
 	size_t clen;
 	void *handle;
 	struct zobj_header *zheader;
@@ -390,31 +389,38 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		clen = PAGE_SIZE;
 		page_store = alloc_page(GFP_NOIO | __GFP_HIGHMEM);
 		if (unlikely(!page_store)) {
-			pr_info("Error allocating memory for "
-				"incompressible page: %u\n", index);
+			pr_info("Error allocating memory for incompressible page: %u\n", index);
 			ret = -ENOMEM;
 			goto out;
 		}
 
-		store_offset = 0;
-		zram_set_flag(zram, index, ZRAM_UNCOMPRESSED);
-		zram_stat_inc(&zram->stats.pages_expand);
-		handle = page_store;
 		src = kmap_atomic(page);
 		cmem = kmap_atomic(page_store);
-		goto memstore;
-	}
+		memcpy(cmem, src, clen);
+		kunmap_atomic(cmem);
+		kunmap_atomic(src);
 
-	handle = zs_malloc(zram->mem_pool, clen + sizeof(*zheader));
-	if (!handle) {
-		pr_info("Error allocating memory for compressed "
-			"page: %u, size=%zu\n", index, clen);
-		ret = -ENOMEM;
-		goto out;
+		zram->table[index].page = page_store;
+		zram->table[index].size = PAGE_SIZE;
+
+		zram_set_flag(zram, index, ZRAM_UNCOMPRESSED);
+		zram_stat_inc(&zram->stats.pages_expand);
+	} else {
+		handle = zs_malloc(zram->mem_pool, clen + sizeof(*zheader));
+		if (!handle) {
+			pr_info("Error allocating memory for compressed page: %u, size=%zu\n", index, clen);
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		zram->table[index].handle = handle;
+		zram->table[index].size = clen;
+
+		cmem = zs_map_object(zram->mem_pool, handle);
+		memcpy(cmem, src, clen);
+		zs_unmap_object(zram->mem_pool, handle);
 	}
-	cmem = zs_map_object(zram->mem_pool, handle);
 
-memstore:
 #if 0
 	/* Back-reference needed for memory defragmentation */
 	if (!zram_test_flag(zram, index, ZRAM_UNCOMPRESSED)) {
@@ -424,18 +430,6 @@ memstore:
 	}
 #endif
 
-	memcpy(cmem, src, clen);
-
-	if (unlikely(zram_test_flag(zram, index, ZRAM_UNCOMPRESSED))) {
-		kunmap_atomic(cmem);
-		kunmap_atomic(src);
-	} else {
-		zs_unmap_object(zram->mem_pool, handle);
-	}
-
-	zram->table[index].handle = handle;
-	zram->table[index].size = clen;
-
 	/* Update stats */
 	zram_stat64_add(zram, &zram->stats.compr_size, clen);
 	zram_stat_inc(&zram->stats.pages_stored);
@@ -580,6 +574,8 @@ error:
 void __zram_reset_device(struct zram *zram)
 {
 	size_t index;
+	void *handle;
+	struct page *page;
 
 	zram->init_done = 0;
 
@@ -592,14 +588,17 @@ 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;
-		if (!handle)
-			continue;
-
-		if (unlikely(zram_test_flag(zram, index, ZRAM_UNCOMPRESSED)))
-			__free_page(handle);
-		else
+		if (unlikely(zram_test_flag(zram, index, ZRAM_UNCOMPRESSED))) {
+			page = zram->table[index].page;
+			if (!page)
+				continue;
+			__free_page(page);
+		} else {
+			handle = zram->table[index].handle;
+			if (!handle)
+				continue;
 			zs_free(zram->mem_pool, handle);
+		}
 	}
 
 	vfree(zram->table);

Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Minchan Kim]

On 05/12/2012 04:06 AM, Konrad Rzeszutek Wilk wrote:

>>>> 1. zs_handle zs_malloc(size_t size, gfp_t flags) - share a pool by many subsystem(like kmalloc)
>>>> 2. zs_handle zs_malloc_pool(struct zs_pool *pool, size_t size) - use own pool(like kmem_cache_alloc)
>>>>
>>>> Any thoughts?
>>>
>>> I don't have any objections to adding this kind of
>>> capability to zsmalloc.  But since we are just speculating
>>> that this capability would be used by some future
>>> kernel subsystem, isn't it normal kernel protocol for
>>> this new capability NOT to be added until that future
>>> kernel subsystem creates a need for it.
>>
>>
>> Now zram makes pool per block device and a embedded system may use zram
>> for several block device, ex) swap device, several compressed tmpfs
>> In such case, share pool is better than private pool because embedded system
>> don't mount/umount frequently on such directories since booting.
>>
>>>
>>> As I said in reply to the other thread, there is missing
>>> functionality in zsmalloc that is making it difficult for
>>> it to be used by zcache.  It would be good if Seth
>>> and Nitin (and any other kernel developers) would work
>>
>>
>> So, if you guys post TODO list, it helps fix the direction.
>>
>>> on those issues before adding capabilities for non-existent
>>> future users of zsmalloc.
>>
>>
>> I think it's not urgent than zs_handle mess.
> 
> I am having a hard time parsing that. Are you saying that
> this is not as important as the zs_handle fixup? I think
> that is what you meant, but what to make sure.


Yes. I think zs_hande fixup is top priority for me than any other stuff I pointed out.

> 
> --
> 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 3/4] zsmalloc use zs_handle instead of void *

[Minchan Kim]

On 05/12/2012 04:28 AM, Konrad Rzeszutek Wilk wrote:

>> Please look.
>>
>> struct zs_handle {
>> 	void *handle
>> };
>>
>> 1)
>>
>> static struct zv_hdr *zv_create(..)
>> {
>> 	struct zs_handle handle;
>> 	..
>> 	handle = zs_malloc(pool, size);
>> 	..
>> 	return handle;
> 
> Compiler will complain that you are returning incorrect type.


My bad. &handle.

> 
>> }
>>
>> handle is on stack so it can't be used by index for slot of radix tree.
> 
> The fix is of course to return a pointer (which your function
> declared), and instead do this:
> 
> {
> 	struct zs_handle *handle;
> 
> 	handle = zs_malloc(pool, size);


It's not a good idea.
For it, zs_malloc needs memory space to keep zs_handle internally.
Why should zsallocator do it? Just for zcache?
It's not good abstraction.


> 	return handle;
> }
> 
>>
>> 2)
>>
>> static struct zv_hdr *zv_create(..)
>> {
>> 	struct zs_handle handle;
>> 	..
>> 	handle = zs_malloc(pool, size);
>> 	..
>> 	return handle.handle;
>> }
>>
>> Okay. Now it works but zcache coupled with zsmalloc tightly.
>> User of zsmalloc should never know internal of zs_handle.
> 
> OK. Then it can just forward declare it:
> 
> struct zs_handle;
> 
> and zsmalloc will treat it as an opaque pointer.
> 
>>
>> 3)
>>
>> - zsmalloc.h
>> void *zs_handle_to_ptr(struct zs_handle handle)
>> {
>> 	return handle.hanle;
>> }
>>
>> static struct zv_hdr *zv_create(..)
>> {
>> 	struct zs_handle handle;
>> 	..
>> 	handle = zs_malloc(pool, size);
>> 	..
>> 	return zs_handle_to_ptr(handle);
> 
>> }
> 
>>
>> Why should zsmalloc support such interface?
> 
> Why not? It is better than a 'void *' or a typedef.
> 
> It is modeled after a pte_t.


It's not same with pte_t.
We normally don't use pte_val to (void*) for unique index of slot.
The problem is that zcache assume handle of zsmalloc is a sizeof(void*)'s
unique value but zcache never assume it's a sizeof(void*).

> 
> 
>> It's a zcache problem so it's desriable to solve it in zcache internal.
> 
> Not really. We shouldn't really pass any 'void *' pointers around.
> 
>> And in future, if we can add/remove zs_handle's fields, we can't make
>> sure such API.
> 
> Meaning ... what exactly do you mean? That the size of the structure
> will change and we won't return the right value? Why not?
> If you use the 'zs_handle_to_ptr' won't that work? Especially if you
> add new values to the end of the struct it won't cause issues.


I mean we might change zs_handle to following as, in future.
(It's insane but who know it?)

struct zs_handle {
	int upper;
	int middle;
	int lower;
};

How could you handle this for zs_handle_to_ptr?

> 
>>
>>
>>>> Its true that making it a real struct would prevent accidental casts
>>>> to void * but due to the above problem, I think we have to stick
>>>> with unsigned long.
> 
> So the problem you are seeing is that you don't want 'struct zs_handle'
> be present in the drivers/staging/zsmalloc/zsmalloc.h header file?
> It looks like the proper place.


No. What I want is to remove coupling zsallocator's handle with zram/zcache.
They shouldn't know internal of handle and assume it's a pointer.

If Nitin confirm zs_handle's format can never change in future, I prefer "unsigned long" Nitin suggested than (void *).
It can prevent confusion that normal allocator's return value is pointer for address so the problem is easy.
But I am not sure he can make sure it.

> 
> --
> 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 3/4] zsmalloc use zs_handle instead of void *

[Minchan Kim]

On 05/12/2012 06:49 AM, Seth Jennings wrote:

> On 05/11/2012 02:29 PM, Konrad Rzeszutek Wilk wrote:
> 
>>> At least, zram is also primary user and it also has such mess
>>> although it's not severe than zcache. zram->table[index].handle
>>> sometime has real (void*) handle, sometime (struct page*).
>>
>> Yikes. Yeah that needs to be fixed.
>>
> 
> 
> How about this (untested)?  Changes to zram_bvec_write() are a little
> hard to make out in this format.  There are a couple of checkpatch fixes
> (two split line strings) and an unused variable store_offset removal mixed
> in too. If this patch is good, I'll break them up for official submission
> after I test.
> 
> diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
> index fbe8ac9..10dcd99 100644
> --- a/drivers/staging/zram/zram_drv.h
> +++ b/drivers/staging/zram/zram_drv.h
> @@ -81,7 +81,10 @@ enum zram_pageflags {
>  
>  /* Allocated for each disk page */
>  struct table {
> -	void *handle;
> +	union {
> +		void *handle; /* compressible */
> +		struct page *page; /* incompressible */


You read my mind. That's exactly same idea with my patch which queued up to my tree.
But there is still problem.

zram has like this code

void *handle = zram->table[index].handle;
if (!handle) {

}

zram->table[index].handle = NULL;

It assume handle's size is greater than or equal to sizeof(struct page*)) for union working.
But we can't make sure handle's size.
If Nitin confirm this, too, the problem would be easy to fix.

-- 
Kind regards,
Minchan Kim

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Nitin Gupta]

On Sun, May 13, 2012 at 10:18 PM, Minchan Kim <minchan@kernel.org> wrote:
> On 05/12/2012 04:28 AM, Konrad Rzeszutek Wilk wrote:
>
>>> Please look.
>>>
>>> struct zs_handle {
>>>      void *handle
>>> };
>>>
>>> 1)
>>>
>>> static struct zv_hdr *zv_create(..)
>>> {
>>>      struct zs_handle handle;
>>>      ..
>>>      handle = zs_malloc(pool, size);
>>>      ..
>>>      return handle;
>>
>> Compiler will complain that you are returning incorrect type.
>
>
> My bad. &handle.
>
>>
>>> }
>>>
>>> handle is on stack so it can't be used by index for slot of radix tree.
>>
>> The fix is of course to return a pointer (which your function
>> declared), and instead do this:
>>
>> {
>>       struct zs_handle *handle;
>>
>>       handle = zs_malloc(pool, size);
>
>
> It's not a good idea.
> For it, zs_malloc needs memory space to keep zs_handle internally.
> Why should zsallocator do it? Just for zcache?
> It's not good abstraction.
>
>
>>       return handle;
>> }
>>
>>>
>>> 2)
>>>
>>> static struct zv_hdr *zv_create(..)
>>> {
>>>      struct zs_handle handle;
>>>      ..
>>>      handle = zs_malloc(pool, size);
>>>      ..
>>>      return handle.handle;
>>> }
>>>
>>> Okay. Now it works but zcache coupled with zsmalloc tightly.
>>> User of zsmalloc should never know internal of zs_handle.
>>
>> OK. Then it can just forward declare it:
>>
>> struct zs_handle;
>>
>> and zsmalloc will treat it as an opaque pointer.
>>
>>>
>>> 3)
>>>
>>> - zsmalloc.h
>>> void *zs_handle_to_ptr(struct zs_handle handle)
>>> {
>>>      return handle.hanle;
>>> }
>>>
>>> static struct zv_hdr *zv_create(..)
>>> {
>>>      struct zs_handle handle;
>>>      ..
>>>      handle = zs_malloc(pool, size);
>>>      ..
>>>      return zs_handle_to_ptr(handle);
>>
>>> }
>>
>>>
>>> Why should zsmalloc support such interface?
>>
>> Why not? It is better than a 'void *' or a typedef.
>>
>> It is modeled after a pte_t.
>
>
> It's not same with pte_t.
> We normally don't use pte_val to (void*) for unique index of slot.
> The problem is that zcache assume handle of zsmalloc is a sizeof(void*)'s
> unique value but zcache never assume it's a sizeof(void*).
>
>>
>>
>>> It's a zcache problem so it's desriable to solve it in zcache internal.
>>
>> Not really. We shouldn't really pass any 'void *' pointers around.
>>
>>> And in future, if we can add/remove zs_handle's fields, we can't make
>>> sure such API.
>>
>> Meaning ... what exactly do you mean? That the size of the structure
>> will change and we won't return the right value? Why not?
>> If you use the 'zs_handle_to_ptr' won't that work? Especially if you
>> add new values to the end of the struct it won't cause issues.
>
>
> I mean we might change zs_handle to following as, in future.
> (It's insane but who know it?)
>
> struct zs_handle {
>        int upper;
>        int middle;
>        int lower;
> };
>
> How could you handle this for zs_handle_to_ptr?
>
>>
>>>
>>>
>>>>> Its true that making it a real struct would prevent accidental casts
>>>>> to void * but due to the above problem, I think we have to stick
>>>>> with unsigned long.
>>
>> So the problem you are seeing is that you don't want 'struct zs_handle'
>> be present in the drivers/staging/zsmalloc/zsmalloc.h header file?
>> It looks like the proper place.
>
>
> No. What I want is to remove coupling zsallocator's handle with zram/zcache.
> They shouldn't know internal of handle and assume it's a pointer.
>
> If Nitin confirm zs_handle's format can never change in future, I prefer "unsigned long" Nitin suggested than (void *).
> It can prevent confusion that normal allocator's return value is pointer for address so the problem is easy.
> But I am not sure he can make sure it.
>

zs_handle will always be an unsigned long so its better to just use
the same as return type.

Another alternative is to return 'struct zs_handle *' which can be
used as a 'void *' by zcache and as unsigned long by zsmalloc.
However, I see no good reason for preferring this over simple unsigned
long as the return type.

Thanks,
Nitin

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Minchan Kim]

On 05/15/2012 10:57 AM, Nitin Gupta wrote:

<SNIP>

>>>
>>>>
>>>>
>>>>>> Its true that making it a real struct would prevent accidental casts
>>>>>> to void * but due to the above problem, I think we have to stick
>>>>>> with unsigned long.
>>>
>>> So the problem you are seeing is that you don't want 'struct zs_handle'
>>> be present in the drivers/staging/zsmalloc/zsmalloc.h header file?
>>> It looks like the proper place.
>>
>>
>> No. What I want is to remove coupling zsallocator's handle with zram/zcache.
>> They shouldn't know internal of handle and assume it's a pointer.
>>
>> If Nitin confirm zs_handle's format can never change in future, I prefer "unsigned long" Nitin suggested than (void *).
>> It can prevent confusion that normal allocator's return value is pointer for address so the problem is easy.
>> But I am not sure he can make sure it.
>>
> 
> zs_handle will always be an unsigned long so its better to just use
> the same as return type.


Okay. it makes problem very easy.

> 
> Another alternative is to return 'struct zs_handle *' which can be
> used as a 'void *' by zcache and as unsigned long by zsmalloc.
> However, I see no good reason for preferring this over simple unsigned
> long as the return type.


Agreed.

-- 
Kind regards,
Minchan Kim

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Konrad Rzeszutek Wilk]

>>
>> The fix is of course to return a pointer (which your function
>> declared), and instead do this:
>>
>> {
>>       struct zs_handle *handle;
>>
>>       handle = zs_malloc(pool, size);
>
>
> It's not a good idea.
> For it, zs_malloc needs memory space to keep zs_handle internally.
> Why should zsallocator do it? Just for zcache?

How different is from now? The zs_malloc keeps the handle internally
as well - it just that is is a void * pointer. Internally, the
ownership and the responsibility to free it lays with zsmalloc.

> It's not good abstraction.

If we want good abstraction, then I don't think 'unsigned long' is
either? I mean it will do for the conversion from 'void *'. Perhaps I
am being a bit optimistic here - and I am trying to jam in this
'struct zs_handle' in all cases but in reality it needs a more
iterative process. So first do 'void *' -> 'unsigned long', and then
later on if we can come up with something more nicely that abstracts
- then use that?
.. snip ..
>>> Why should zsmalloc support such interface?
>>
>> Why not? It is better than a 'void *' or a typedef.
>>
>> It is modeled after a pte_t.
>
>
> It's not same with pte_t.
> We normally don't use pte_val to (void*) for unique index of slot.

Right, but I thought we want to get rid of all of the '(void *)'
usages and instead
pass some opaque pointer.

> The problem is that zcache assume handle of zsmalloc is a sizeof(void*)'s
> unique value but zcache never assume it's a sizeof(void*).

Huh? I am parsing your sentence as: "zcache assumes .. sizeof(void *),
but zcache never assumes its .. sizeof(void *)"?

Zcache has to assume it is a pointer. And providing a 'struct
zs_handle *' would fit the bill?
>>
>>
>>> It's a zcache problem so it's desriable to solve it in zcache internal.
>>
>> Not really. We shouldn't really pass any 'void *' pointers around.
>>
>>> And in future, if we can add/remove zs_handle's fields, we can't make
>>> sure such API.
>>
>> Meaning ... what exactly do you mean? That the size of the structure
>> will change and we won't return the right value? Why not?
>> If you use the 'zs_handle_to_ptr' won't that work? Especially if you
>> add new values to the end of the struct it won't cause issues.
>
>
> I mean we might change zs_handle to following as, in future.
> (It's insane but who know it?)

OK, so BUILD_BUG(sizeof(struct zs_handle *) != sizeof(void *))
with a big fat comment saying that one needs to go over the other users
of zcache/zram/zsmalloc to double check?

But why would it matter? The zs_handle would be returned as a pointer
- so the size is the same to the caller.

>
> struct zs_handle {
>        int upper;
>        int middle;
>        int lower;
> };
>
> How could you handle this for zs_handle_to_ptr?

Gosh, um, I couldn't :-) Well, maybe with something that does
 return "upper | middle | lower", but yeah that is not the goal.


>>>>> Its true that making it a real struct would prevent accidental casts
>>>>> to void * but due to the above problem, I think we have to stick
>>>>> with unsigned long.
>>
>> So the problem you are seeing is that you don't want 'struct zs_handle'
>> be present in the drivers/staging/zsmalloc/zsmalloc.h header file?
>> It looks like the proper place.
>
>
> No. What I want is to remove coupling zsallocator's handle with zram/zcache.
> They shouldn't know internal of handle and assume it's a pointer.

I concur. And hence I was thinking that the 'struct zs_handle *'
pointer would work.

>
> If Nitin confirm zs_handle's format can never change in future, I prefer "unsigned long" Nitin suggested than (void *).
> It can prevent confusion that normal allocator's return value is pointer for address so the problem is easy.
> But I am not sure he can make sure it.

Well, everything changes over time  so putting a stick in the ground
and saying 'this must
be this way' is not really the best way.

Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Konrad Rzeszutek Wilk]

>>> I think it's not urgent than zs_handle mess.
>>
>> I am having a hard time parsing that. Are you saying that
>> this is not as important as the zs_handle fixup? I think
>> that is what you meant, but what to make sure.
>
>
> Yes. I think zs_hande fixup is top priority for me than any other stuff I pointed out.

What else is should we put on the TODO?

Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Minchan Kim]

On 05/16/2012 12:04 AM, Konrad Rzeszutek Wilk wrote:

>>>
>>> The fix is of course to return a pointer (which your function
>>> declared), and instead do this:
>>>
>>> {
>>>       struct zs_handle *handle;
>>>
>>>       handle = zs_malloc(pool, size);
>>
>>
>> It's not a good idea.
>> For it, zs_malloc needs memory space to keep zs_handle internally.
>> Why should zsallocator do it? Just for zcache?
> 
> How different is from now? The zs_malloc keeps the handle internally
> as well - it just that is is a void * pointer. Internally, the
> ownership and the responsibility to free it lays with zsmalloc.


I don't get it. now zsmalloc doesn't keep the handle internally.
It just makes handle and return to caller.
About void* as return value, I think it's not good.
Return just struct zs_handle(NOT struct zs_handle *) or unsigned long would be good because
zsmalloc doesn't need keeping the handle internally at the cost of consume
memory space to store it.


> 
>> It's not good abstraction.
> 
> If we want good abstraction, then I don't think 'unsigned long' is
> either? I mean it will do for the conversion from 'void *'. Perhaps I
> am being a bit optimistic here - and I am trying to jam in this
> 'struct zs_handle' in all cases but in reality it needs a more
> iterative process. So first do 'void *' -> 'unsigned long', and then
> later on if we can come up with something more nicely that abstracts
> - then use that?
> .. snip ..
>>>> Why should zsmalloc support such interface?
>>>
>>> Why not? It is better than a 'void *' or a typedef.
>>>
>>> It is modeled after a pte_t.
>>
>>
>> It's not same with pte_t.
>> We normally don't use pte_val to (void*) for unique index of slot.
> 
> Right, but I thought we want to get rid of all of the '(void *)'
> usages and instead
> pass some opaque pointer.


opaque is good but pointer isn't good as handle, IMHO.
Value, not pointer would be better.
zsmalloc's goal is memory space efficiency so let's not consume unnecessary space for keeping
the handle in zsmalloc's internal

> 
>> The problem is that zcache assume handle of zsmalloc is a sizeof(void*)'s
>> unique value but zcache never assume it's a sizeof(void*).
> 
> Huh? I am parsing your sentence as: "zcache assumes .. sizeof(void *),
> but zcache never assumes its .. sizeof(void *)"?
> 
> Zcache has to assume it is a pointer. And providing a 'struct
> zs_handle *' would fit the bill?


Sorry for typo.
I mean zcache shouldn't assume handle of zsmalloc is void*.
And I prefer value rather than pointer. I already mentioned why I like it in above.

>>>
>>>
>>>> It's a zcache problem so it's desriable to solve it in zcache internal.
>>>
>>> Not really. We shouldn't really pass any 'void *' pointers around.
>>>
>>>> And in future, if we can add/remove zs_handle's fields, we can't make
>>>> sure such API.
>>>
>>> Meaning ... what exactly do you mean? That the size of the structure
>>> will change and we won't return the right value? Why not?
>>> If you use the 'zs_handle_to_ptr' won't that work? Especially if you
>>> add new values to the end of the struct it won't cause issues.
>>
>>
>> I mean we might change zs_handle to following as, in future.
>> (It's insane but who know it?)
> 
> OK, so BUILD_BUG(sizeof(struct zs_handle *) != sizeof(void *))
> with a big fat comment saying that one needs to go over the other users
> of zcache/zram/zsmalloc to double check?


If we will use unsigned long as handle, we don't need so BUILD_BUG_ON.

> 
> But why would it matter? The zs_handle would be returned as a pointer
> - so the size is the same to the caller.
> 
>>
>> struct zs_handle {
>>        int upper;
>>        int middle;
>>        int lower;
>> };
>>
>> How could you handle this for zs_handle_to_ptr?
> 
> Gosh, um, I couldn't :-) Well, maybe with something that does
>  return "upper | middle | lower", but yeah that is not the goal.
> 
> 
>>>>>> Its true that making it a real struct would prevent accidental casts
>>>>>> to void * but due to the above problem, I think we have to stick
>>>>>> with unsigned long.
>>>
>>> So the problem you are seeing is that you don't want 'struct zs_handle'
>>> be present in the drivers/staging/zsmalloc/zsmalloc.h header file?
>>> It looks like the proper place.
>>
>>
>> No. What I want is to remove coupling zsallocator's handle with zram/zcache.
>> They shouldn't know internal of handle and assume it's a pointer.
> 
> I concur. And hence I was thinking that the 'struct zs_handle *'
> pointer would work.


Do you really hate "unsigned long" as handle?

> 
>>
>> If Nitin confirm zs_handle's format can never change in future, I prefer "unsigned long" Nitin suggested than (void *).
>> It can prevent confusion that normal allocator's return value is pointer for address so the problem is easy.
>> But I am not sure he can make sure it.
> 
> Well, everything changes over time  so putting a stick in the ground
> and saying 'this must
> be this way' is not really the best way.


Hmm, agree on your above statement but I can't imagine better idea.

> 
> --
> 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=ilto:"dont@kvack.org"> email@kvack.org </a>
> 
> 
> 



-- 
Kind regards,
Minchan Kim

Re: [PATCH 4/4] zsmalloc: zsmalloc: align cache line size

[Minchan Kim]

On 05/16/2012 12:18 AM, Konrad Rzeszutek Wilk wrote:

>>>> I think it's not urgent than zs_handle mess.
>>>
>>> I am having a hard time parsing that. Are you saying that
>>> this is not as important as the zs_handle fixup? I think
>>> that is what you meant, but what to make sure.
>>
>>
>> Yes. I think zs_hande fixup is top priority for me than any other stuff I pointed out.
> 
> What else is should we put on the TODO?


Next I have a plan.

1. zs_handle zs_malloc(size_t size, gfp_t flags) - share a pool by many
subsystem(like kmalloc)
2. zs_handle zs_malloc_pool(struct zs_pool *pool, size_t size) - use own
pool(like kmem_cache_alloc)

And there is severe another item but I think it's not good time to
mention it because I don't have a big picture still yet.
Only thing I can speak is that it's would be related to zram or zcache.
I will compare which is better for me.

I will tell it if I am ready.

> 
> --
> 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 3/4] zsmalloc use zs_handle instead of void *

[Konrad Rzeszutek Wilk]

>>> It's not good abstraction.
>>
>> If we want good abstraction, then I don't think 'unsigned long' is
>> either? I mean it will do for the conversion from 'void *'. Perhaps I
>> am being a bit optimistic here - and I am trying to jam in this
>> 'struct zs_handle' in all cases but in reality it needs a more
>> iterative process. So first do 'void *' -> 'unsigned long', and then
>> later on if we can come up with something more nicely that abstracts
>> - then use that?

..snip..
>>> No. What I want is to remove coupling zsallocator's handle with zram/zcache.
>>> They shouldn't know internal of handle and assume it's a pointer.
>>
>> I concur. And hence I was thinking that the 'struct zs_handle *'
>> pointer would work.
>
>
> Do you really hate "unsigned long" as handle?
..snip,,
>> Well, everything changes over time  so putting a stick in the ground
>> and saying 'this must
>> be this way' is not really the best way.
>
>
> Hmm, agree on your above statement but I can't imagine better idea.
>

OK. Lets go with unsigned long. I can prep a patch next week when I am
back from vacation unless somebody beats me to it.