[PATCH v2 1/8] mm/zsmalloc: modify zs compact trace interface

[PATCH v2 1/8] mm/zsmalloc: modify zs compact trace interface

[Ganesh Mahendran]

This patch changes trace_zsmalloc_compact_start[end] to
trace_zs_compact_start[end] to keep function naming consistent
with others in zsmalloc

Also this patch remove pages_total_compacted information which
may not really needed.

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
---
v2: change commit message
---
 include/trace/events/zsmalloc.h | 16 ++++++----------
 mm/zsmalloc.c                   |  7 +++----
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/include/trace/events/zsmalloc.h b/include/trace/events/zsmalloc.h
index 3b6f14e..c7a39f4 100644
--- a/include/trace/events/zsmalloc.h
+++ b/include/trace/events/zsmalloc.h
@@ -7,7 +7,7 @@
 #include <linux/types.h>
 #include <linux/tracepoint.h>
 
-TRACE_EVENT(zsmalloc_compact_start,
+TRACE_EVENT(zs_compact_start,
 
 	TP_PROTO(const char *pool_name),
 
@@ -25,29 +25,25 @@ TRACE_EVENT(zsmalloc_compact_start,
 		  __entry->pool_name)
 );
 
-TRACE_EVENT(zsmalloc_compact_end,
+TRACE_EVENT(zs_compact_end,
 
-	TP_PROTO(const char *pool_name, unsigned long pages_compacted,
-			unsigned long pages_total_compacted),
+	TP_PROTO(const char *pool_name, unsigned long pages_compacted),
 
-	TP_ARGS(pool_name, pages_compacted, pages_total_compacted),
+	TP_ARGS(pool_name, pages_compacted),
 
 	TP_STRUCT__entry(
 		__field(const char *, pool_name)
 		__field(unsigned long, pages_compacted)
-		__field(unsigned long, pages_total_compacted)
 	),
 
 	TP_fast_assign(
 		__entry->pool_name = pool_name;
 		__entry->pages_compacted = pages_compacted;
-		__entry->pages_total_compacted = pages_total_compacted;
 	),
 
-	TP_printk("pool %s: %ld pages compacted(total %ld)",
+	TP_printk("pool %s: %ld pages compacted",
 		  __entry->pool_name,
-		  __entry->pages_compacted,
-		  __entry->pages_total_compacted)
+		  __entry->pages_compacted)
 );
 
 #endif /* _TRACE_ZSMALLOC_H */
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index e425de4..c7f79d5 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -2323,7 +2323,7 @@ unsigned long zs_compact(struct zs_pool *pool)
 	struct size_class *class;
 	unsigned long pages_compacted_before = pool->stats.pages_compacted;
 
-	trace_zsmalloc_compact_start(pool->name);
+	trace_zs_compact_start(pool->name);
 
 	for (i = zs_size_classes - 1; i >= 0; i--) {
 		class = pool->size_class[i];
@@ -2334,9 +2334,8 @@ unsigned long zs_compact(struct zs_pool *pool)
 		__zs_compact(pool, class);
 	}
 
-	trace_zsmalloc_compact_end(pool->name,
-		pool->stats.pages_compacted - pages_compacted_before,
-		pool->stats.pages_compacted);
+	trace_zs_compact_end(pool->name,
+		pool->stats.pages_compacted - pages_compacted_before);
 
 	return pool->stats.pages_compacted;
 }
-- 
1.9.1

[PATCH v2 2/8] mm/zsmalloc: use obj_index to keep consistent with others

[Ganesh Mahendran]

This is a cleanup patch. Change "index" to "obj_index" to keep
consistent with others in zsmalloc.

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
----
v2: none
---
 mm/zsmalloc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index c7f79d5..8915a1d 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1779,7 +1779,7 @@ struct zs_compact_control {
 	struct page *d_page;
 	 /* Starting object index within @s_page which used for live object
 	  * in the subpage. */
-	int index;
+	int obj_idx;
 };
 
 static int migrate_zspage(struct zs_pool *pool, struct size_class *class,
@@ -1789,16 +1789,16 @@ static int migrate_zspage(struct zs_pool *pool, struct size_class *class,
 	unsigned long handle;
 	struct page *s_page = cc->s_page;
 	struct page *d_page = cc->d_page;
-	unsigned long index = cc->index;
+	int obj_idx = cc->obj_idx;
 	int ret = 0;
 
 	while (1) {
-		handle = find_alloced_obj(class, s_page, index);
+		handle = find_alloced_obj(class, s_page, obj_idx);
 		if (!handle) {
 			s_page = get_next_page(s_page);
 			if (!s_page)
 				break;
-			index = 0;
+			obj_idx = 0;
 			continue;
 		}
 
@@ -1812,7 +1812,7 @@ static int migrate_zspage(struct zs_pool *pool, struct size_class *class,
 		used_obj = handle_to_obj(handle);
 		free_obj = obj_malloc(class, get_zspage(d_page), handle);
 		zs_object_copy(class, free_obj, used_obj);
-		index++;
+		obj_idx++;
 		/*
 		 * record_obj updates handle's value to free_obj and it will
 		 * invalidate lock bit(ie, HANDLE_PIN_BIT) of handle, which
@@ -1827,7 +1827,7 @@ static int migrate_zspage(struct zs_pool *pool, struct size_class *class,
 
 	/* Remember last position in this iteration */
 	cc->s_page = s_page;
-	cc->index = index;
+	cc->obj_idx = obj_idx;
 
 	return ret;
 }
@@ -2282,7 +2282,7 @@ static void __zs_compact(struct zs_pool *pool, struct size_class *class)
 		if (!zs_can_compact(class))
 			break;
 
-		cc.index = 0;
+		cc.obj_idx = 0;
 		cc.s_page = get_first_page(src_zspage);
 
 		while ((dst_zspage = isolate_zspage(class, false))) {
-- 
1.9.1

Re: [PATCH v2 2/8] mm/zsmalloc: use obj_index to keep consistent with others

[Sergey Senozhatsky]

On (07/04/16 14:49), Ganesh Mahendran wrote:
> This is a cleanup patch. Change "index" to "obj_index" to keep
> consistent with others in zsmalloc.
> 
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

Re: [PATCH v2 2/8] mm/zsmalloc: use obj_index to keep consistent with others

[Minchan Kim]

On Mon, Jul 04, 2016 at 02:49:53PM +0800, Ganesh Mahendran wrote:
> This is a cleanup patch. Change "index" to "obj_index" to keep
> consistent with others in zsmalloc.
> 
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>

[PATCH v2 3/8] mm/zsmalloc: take obj index back from find_alloced_obj

[Ganesh Mahendran]

the obj index value should be updated after return from
find_alloced_obj() to avoid CPU buring caused by unnecessary
object scanning.

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
----
v2:
  - update commit description

Hi, Minchan:
find_alloced_obj() already has the argument which use the obj_idx
name. So I use index as the local variable name in this function.
Is this OK?
---
 mm/zsmalloc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 8915a1d..92b043c 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1744,10 +1744,11 @@ static void zs_object_copy(struct size_class *class, unsigned long dst,
  * return handle.
  */
 static unsigned long find_alloced_obj(struct size_class *class,
-					struct page *page, int index)
+					struct page *page, int *obj_idx)
 {
 	unsigned long head;
 	int offset = 0;
+	int index = *obj_idx;
 	unsigned long handle = 0;
 	void *addr = kmap_atomic(page);
 
@@ -1768,6 +1769,9 @@ static unsigned long find_alloced_obj(struct size_class *class,
 	}
 
 	kunmap_atomic(addr);
+
+	*obj_idx = index;
+
 	return handle;
 }
 
@@ -1793,7 +1797,7 @@ static int migrate_zspage(struct zs_pool *pool, struct size_class *class,
 	int ret = 0;
 
 	while (1) {
-		handle = find_alloced_obj(class, s_page, obj_idx);
+		handle = find_alloced_obj(class, s_page, &obj_idx);
 		if (!handle) {
 			s_page = get_next_page(s_page);
 			if (!s_page)
-- 
1.9.1

Re: [PATCH v2 3/8] mm/zsmalloc: take obj index back from find_alloced_obj

[Sergey Senozhatsky]

On (07/04/16 14:49), Ganesh Mahendran wrote:
> the obj index value should be updated after return from
> find_alloced_obj() to avoid CPU buring caused by unnecessary
> object scanning.

'burning'

> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

Re: [PATCH v2 3/8] mm/zsmalloc: take obj index back from find_alloced_obj

[Minchan Kim]

On Mon, Jul 04, 2016 at 02:49:54PM +0800, Ganesh Mahendran wrote:
> the obj index value should be updated after return from
> find_alloced_obj() to avoid CPU buring caused by unnecessary
> object scanning.
> 
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>

[PATCH v2 4/8] mm/zsmalloc: use class->objs_per_zspage to get num of max objects

[Ganesh Mahendran]

num of max objects in zspage is stored in each size_class now.
So there is no need to re-calculate it.

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>
---
 mm/zsmalloc.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 92b043c..ee8a29a 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -638,8 +638,7 @@ static int zs_stats_size_show(struct seq_file *s, void *v)
 		freeable = zs_can_compact(class);
 		spin_unlock(&class->lock);
 
-		objs_per_zspage = get_maxobj_per_zspage(class->size,
-				class->pages_per_zspage);
+		objs_per_zspage = class->objs_per_zspage;
 		pages_used = obj_allocated / objs_per_zspage *
 				class->pages_per_zspage;
 
@@ -1017,8 +1016,7 @@ static void __free_zspage(struct zs_pool *pool, struct size_class *class,
 
 	cache_free_zspage(pool, zspage);
 
-	zs_stat_dec(class, OBJ_ALLOCATED, get_maxobj_per_zspage(
-			class->size, class->pages_per_zspage));
+	zs_stat_dec(class, OBJ_ALLOCATED, class->objs_per_zspage);
 	atomic_long_sub(class->pages_per_zspage,
 					&pool->pages_allocated);
 }
@@ -1369,7 +1367,7 @@ static bool can_merge(struct size_class *prev, int size, int pages_per_zspage)
 	if (prev->pages_per_zspage != pages_per_zspage)
 		return false;
 
-	if (get_maxobj_per_zspage(prev->size, prev->pages_per_zspage)
+	if (prev->objs_per_zspage
 		!= get_maxobj_per_zspage(size, pages_per_zspage))
 		return false;
 
@@ -1595,8 +1593,7 @@ unsigned long zs_malloc(struct zs_pool *pool, size_t size, gfp_t gfp)
 	record_obj(handle, obj);
 	atomic_long_add(class->pages_per_zspage,
 				&pool->pages_allocated);
-	zs_stat_inc(class, OBJ_ALLOCATED, get_maxobj_per_zspage(
-			class->size, class->pages_per_zspage));
+	zs_stat_inc(class, OBJ_ALLOCATED, class->objs_per_zspage);
 
 	/* We completely set up zspage so mark them as movable */
 	SetZsPageMovable(pool, zspage);
@@ -2268,8 +2265,7 @@ static unsigned long zs_can_compact(struct size_class *class)
 		return 0;
 
 	obj_wasted = obj_allocated - obj_used;
-	obj_wasted /= get_maxobj_per_zspage(class->size,
-			class->pages_per_zspage);
+	obj_wasted /= class->objs_per_zspage;
 
 	return obj_wasted * class->pages_per_zspage;
 }
@@ -2482,8 +2478,8 @@ struct zs_pool *zs_create_pool(const char *name)
 		class->size = size;
 		class->index = i;
 		class->pages_per_zspage = pages_per_zspage;
-		class->objs_per_zspage = class->pages_per_zspage *
-						PAGE_SIZE / class->size;
+		class->objs_per_zspage = get_maxobj_per_zspage(class->size,
+							class->pages_per_zspage);
 		spin_lock_init(&class->lock);
 		pool->size_class[i] = class;
 		for (fullness = ZS_EMPTY; fullness < NR_ZS_FULLNESS;
-- 
1.9.1

Re: [PATCH v2 4/8] mm/zsmalloc: use class->objs_per_zspage to get num of max objects

[Sergey Senozhatsky]

On (07/04/16 14:49), Ganesh Mahendran wrote:
> num of max objects in zspage is stored in each size_class now.
> So there is no need to re-calculate it.
> 
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
> Acked-by: Minchan Kim <minchan@kernel.org>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

[PATCH v2 5/8] mm/zsmalloc: avoid calculate max objects of zspage twice

[Ganesh Mahendran]

Currently, if a class can not be merged, the max objects of zspage
in that class may be calculated twice.

This patch calculate max objects of zspage at the begin, and pass
the value to can_merge() to decide whether the class can be merged.

Also this patch remove function get_maxobj_per_zspage(), as there
is no other place to call this funtion.

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
----
V2:
    remove get_maxobj_per_zspage()  - Minchan
---
 mm/zsmalloc.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index ee8a29a..2c4549e 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -470,11 +470,6 @@ static struct zpool_driver zs_zpool_driver = {
 MODULE_ALIAS("zpool-zsmalloc");
 #endif /* CONFIG_ZPOOL */
 
-static unsigned int get_maxobj_per_zspage(int size, int pages_per_zspage)
-{
-	return pages_per_zspage * PAGE_SIZE / size;
-}
-
 /* per-cpu VM mapping areas for zspage accesses that cross page boundaries */
 static DEFINE_PER_CPU(struct mapping_area, zs_map_area);
 
@@ -1362,16 +1357,14 @@ static void init_zs_size_classes(void)
 	zs_size_classes = nr;
 }
 
-static bool can_merge(struct size_class *prev, int size, int pages_per_zspage)
+static bool can_merge(struct size_class *prev, int pages_per_zspage,
+					int objs_per_zspage)
 {
-	if (prev->pages_per_zspage != pages_per_zspage)
-		return false;
+	if (prev->pages_per_zspage == pages_per_zspage &&
+		prev->objs_per_zspage == objs_per_zspage)
+		return true;
 
-	if (prev->objs_per_zspage
-		!= get_maxobj_per_zspage(size, pages_per_zspage))
-		return false;
-
-	return true;
+	return false;
 }
 
 static bool zspage_full(struct size_class *class, struct zspage *zspage)
@@ -2447,6 +2440,7 @@ struct zs_pool *zs_create_pool(const char *name)
 	for (i = zs_size_classes - 1; i >= 0; i--) {
 		int size;
 		int pages_per_zspage;
+		int objs_per_zspage;
 		struct size_class *class;
 		int fullness = 0;
 
@@ -2454,6 +2448,7 @@ struct zs_pool *zs_create_pool(const char *name)
 		if (size > ZS_MAX_ALLOC_SIZE)
 			size = ZS_MAX_ALLOC_SIZE;
 		pages_per_zspage = get_pages_per_zspage(size);
+		objs_per_zspage = pages_per_zspage * PAGE_SIZE / size;
 
 		/*
 		 * size_class is used for normal zsmalloc operation such
@@ -2465,7 +2460,7 @@ struct zs_pool *zs_create_pool(const char *name)
 		 * previous size_class if possible.
 		 */
 		if (prev_class) {
-			if (can_merge(prev_class, size, pages_per_zspage)) {
+			if (can_merge(prev_class, pages_per_zspage, objs_per_zspage)) {
 				pool->size_class[i] = prev_class;
 				continue;
 			}
@@ -2478,8 +2473,7 @@ struct zs_pool *zs_create_pool(const char *name)
 		class->size = size;
 		class->index = i;
 		class->pages_per_zspage = pages_per_zspage;
-		class->objs_per_zspage = get_maxobj_per_zspage(class->size,
-							class->pages_per_zspage);
+		class->objs_per_zspage = objs_per_zspage;
 		spin_lock_init(&class->lock);
 		pool->size_class[i] = class;
 		for (fullness = ZS_EMPTY; fullness < NR_ZS_FULLNESS;
-- 
1.9.1

Re: [PATCH v2 5/8] mm/zsmalloc: avoid calculate max objects of zspage twice

[Sergey Senozhatsky]

On (07/04/16 14:49), Ganesh Mahendran wrote:
> Currently, if a class can not be merged, the max objects of zspage
> in that class may be calculated twice.
> 
> This patch calculate max objects of zspage at the begin, and pass
> the value to can_merge() to decide whether the class can be merged.
> 
> Also this patch remove function get_maxobj_per_zspage(), as there
> is no other place to call this funtion.
> 
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

Re: [PATCH v2 5/8] mm/zsmalloc: avoid calculate max objects of zspage twice

[Minchan Kim]

On Mon, Jul 04, 2016 at 02:49:56PM +0800, Ganesh Mahendran wrote:
> Currently, if a class can not be merged, the max objects of zspage
> in that class may be calculated twice.
> 
> This patch calculate max objects of zspage at the begin, and pass
> the value to can_merge() to decide whether the class can be merged.
> 
> Also this patch remove function get_maxobj_per_zspage(), as there
> is no other place to call this funtion.
> 
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>

[PATCH v2 6/8] mm/zsmalloc: keep comments consistent with code

[Ganesh Mahendran]

some minor change of comments:
1). update zs_malloc(),zs_create_pool() function header
2). update "Usage of struct page fields"

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
----
v2:
    change *object index* to *object offset* - Minchan
---
 mm/zsmalloc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 2c4549e..df804b8 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -20,6 +20,7 @@
  *	page->freelist(index): links together all component pages of a zspage
  *		For the huge page, this is always 0, so we use this field
  *		to store handle.
+ *	page->units: first object offset in a subpage of zspage
  *
  * Usage of struct page flags:
  *	PG_private: identifies the first component page
@@ -140,9 +141,6 @@
  */
 #define ZS_SIZE_CLASS_DELTA	(PAGE_SIZE >> CLASS_BITS)
 
-/*
- * We do not maintain any list for completely empty or full pages
- */
 enum fullness_group {
 	ZS_EMPTY,
 	ZS_ALMOST_EMPTY,
@@ -1535,6 +1533,7 @@ static unsigned long obj_malloc(struct size_class *class,
  * zs_malloc - Allocate block of given size from pool.
  * @pool: pool to allocate from
  * @size: size of block to allocate
+ * @gfp: gfp flags when allocating object
  *
  * On success, handle to the allocated object is returned,
  * otherwise 0.
@@ -2400,7 +2399,7 @@ static int zs_register_shrinker(struct zs_pool *pool)
 
 /**
  * zs_create_pool - Creates an allocation pool to work from.
- * @flags: allocation flags used to allocate pool metadata
+ * @name: pool name to be created
  *
  * This function must be called before anything when using
  * the zsmalloc allocator.
-- 
1.9.1

Re: [PATCH v2 6/8] mm/zsmalloc: keep comments consistent with code

[Sergey Senozhatsky]

On (07/04/16 14:49), Ganesh Mahendran wrote:
> some minor change of comments:
> 1). update zs_malloc(),zs_create_pool() function header
> 2). update "Usage of struct page fields"
> 
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

Re: [PATCH v2 6/8] mm/zsmalloc: keep comments consistent with code

[Minchan Kim]

On Mon, Jul 04, 2016 at 02:49:57PM +0800, Ganesh Mahendran wrote:
> some minor change of comments:
> 1). update zs_malloc(),zs_create_pool() function header
> 2). update "Usage of struct page fields"
> 
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>

[PATCH v2 7/8] mm/zsmalloc: add __init,__exit attribute

[Ganesh Mahendran]

Add __init,__exit attribute for function that only called in
module init/exit to save memory.

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
----
v2:
    add __init/__exit for zs_register_cpu_notifier/zs_unregister_cpu_notifier
---
 mm/zsmalloc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index df804b8..756f839 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1314,7 +1314,7 @@ static struct notifier_block zs_cpu_nb = {
 	.notifier_call = zs_cpu_notifier
 };
 
-static int zs_register_cpu_notifier(void)
+static int __init zs_register_cpu_notifier(void)
 {
 	int cpu, uninitialized_var(ret);
 
@@ -1331,7 +1331,7 @@ static int zs_register_cpu_notifier(void)
 	return notifier_to_errno(ret);
 }
 
-static void zs_unregister_cpu_notifier(void)
+static void __exit zs_unregister_cpu_notifier(void)
 {
 	int cpu;
 
@@ -1344,7 +1344,7 @@ static void zs_unregister_cpu_notifier(void)
 	cpu_notifier_register_done();
 }
 
-static void init_zs_size_classes(void)
+static void __init init_zs_size_classes(void)
 {
 	int nr;
 
@@ -1887,7 +1887,7 @@ static struct file_system_type zsmalloc_fs = {
 	.kill_sb	= kill_anon_super,
 };
 
-static int zsmalloc_mount(void)
+static int __init zsmalloc_mount(void)
 {
 	int ret = 0;
 
@@ -1898,7 +1898,7 @@ static int zsmalloc_mount(void)
 	return ret;
 }
 
-static void zsmalloc_unmount(void)
+static void __exit zsmalloc_unmount(void)
 {
 	kern_unmount(zsmalloc_mnt);
 }
-- 
1.9.1

Re: [PATCH v2 7/8] mm/zsmalloc: add __init,__exit attribute

[Sergey Senozhatsky]

On (07/04/16 14:49), Ganesh Mahendran wrote:
[..]
> -static void zs_unregister_cpu_notifier(void)
> +static void __exit zs_unregister_cpu_notifier(void)
>  {

this __exit symbol is called from `__init zs_init()' and thus is
free to crash.

	-ss

Re: [PATCH v2 7/8] mm/zsmalloc: add __init,__exit attribute

[Ganesh Mahendran]

Hi, Sergey

2016-07-04 16:43 GMT+08:00 Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com>:
> On (07/04/16 14:49), Ganesh Mahendran wrote:
> [..]
>> -static void zs_unregister_cpu_notifier(void)
>> +static void __exit zs_unregister_cpu_notifier(void)
>>  {
>
> this __exit symbol is called from `__init zs_init()' and thus is
> free to crash.

I change code to force the code goto notifier_fail where the
zs_unregister_cpu_notifier will be called.
I tested with zsmalloc module buildin and built as a module.

Please correct me, if I miss something.

Thanks.


>
>         -ss

Re: [PATCH v2 7/8] mm/zsmalloc: add __init,__exit attribute

[Sergey Senozhatsky]

Hello Ganesh,

On (07/04/16 17:21), Ganesh Mahendran wrote:
> > On (07/04/16 14:49), Ganesh Mahendran wrote:
> > [..]
> >> -static void zs_unregister_cpu_notifier(void)
> >> +static void __exit zs_unregister_cpu_notifier(void)
> >>  {
> >
> > this __exit symbol is called from `__init zs_init()' and thus is
> > free to crash.
> 
> I change code to force the code goto notifier_fail where the
> zs_unregister_cpu_notifier will be called.
> I tested with zsmalloc module buildin and built as a module.

sorry, not sure I understand what do you mean by this.


> Please correct me, if I miss something.

you have an __exit section function being called from
__init section:

static void __exit zs_unregister_cpu_notifier(void)
{
}

static int __init zs_init(void)
{
	zs_unregister_cpu_notifier();
}

it's no good.

	-ss

Re: [PATCH v2 7/8] mm/zsmalloc: add __init,__exit attribute

[Minchan Kim]

On Tue, Jul 05, 2016 at 10:00:28AM +0900, Sergey Senozhatsky wrote:
> Hello Ganesh,
> 
> On (07/04/16 17:21), Ganesh Mahendran wrote:
> > > On (07/04/16 14:49), Ganesh Mahendran wrote:
> > > [..]
> > >> -static void zs_unregister_cpu_notifier(void)
> > >> +static void __exit zs_unregister_cpu_notifier(void)
> > >>  {
> > >
> > > this __exit symbol is called from `__init zs_init()' and thus is
> > > free to crash.
> > 
> > I change code to force the code goto notifier_fail where the
> > zs_unregister_cpu_notifier will be called.
> > I tested with zsmalloc module buildin and built as a module.
> 
> sorry, not sure I understand what do you mean by this.

It seems he tested it both builtin and module with simulating to fail
zs_register_cpu_notifier so that finally called zs_unergister_cpu_notifier.
With that, he cannot find any problem.
> 

> 
> > Please correct me, if I miss something.
> 
> you have an __exit section function being called from
> __init section:
> 
> static void __exit zs_unregister_cpu_notifier(void)
> {
> }
> 
> static int __init zs_init(void)
> {
> 	zs_unregister_cpu_notifier();
> }
> 
> it's no good.

Agree.

I didn't look at linker script how to handle it. Although it works well,
it would be not desirable to mark __exit to the function we already
know it would be called from non-exit functions.

Re: [PATCH v2 7/8] mm/zsmalloc: add __init,__exit attribute

[Ganesh Mahendran]

2016-07-06 10:48 GMT+08:00 Minchan Kim <minchan@kernel.org>:
> On Tue, Jul 05, 2016 at 10:00:28AM +0900, Sergey Senozhatsky wrote:
>> Hello Ganesh,
>>
>> On (07/04/16 17:21), Ganesh Mahendran wrote:
>> > > On (07/04/16 14:49), Ganesh Mahendran wrote:
>> > > [..]
>> > >> -static void zs_unregister_cpu_notifier(void)
>> > >> +static void __exit zs_unregister_cpu_notifier(void)
>> > >>  {
>> > >
>> > > this __exit symbol is called from `__init zs_init()' and thus is
>> > > free to crash.
>> >
>> > I change code to force the code goto notifier_fail where the
>> > zs_unregister_cpu_notifier will be called.
>> > I tested with zsmalloc module buildin and built as a module.
>>
>> sorry, not sure I understand what do you mean by this.
>
> It seems he tested it both builtin and module with simulating to fail
> zs_register_cpu_notifier so that finally called zs_unergister_cpu_notifier.
> With that, he cannot find any problem.

Yes, This is what I mean.

>>
>
>>
>> > Please correct me, if I miss something.
>>
>> you have an __exit section function being called from
>> __init section:
>>
>> static void __exit zs_unregister_cpu_notifier(void)
>> {
>> }
>>
>> static int __init zs_init(void)
>> {
>>       zs_unregister_cpu_notifier();
>> }
>>
>> it's no good.
>
> Agree.
>
> I didn't look at linker script how to handle it. Although it works well,
> it would be not desirable to mark __exit to the function we already
> know it would be called from non-exit functions.

I will revert change in this patch.
Thanks.

[PATCH v2 8/8] mm/zsmalloc: use helper to clear page->flags bit

[Ganesh Mahendran]

user ClearPagePrivate/ClearPagePrivate2 helper to clear
PG_private/PG_private_2 in page->flags

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>
----
v2: none
---
 mm/zsmalloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 756f839..297f25b 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -940,8 +940,8 @@ static void unpin_tag(unsigned long handle)
 static void reset_page(struct page *page)
 {
 	__ClearPageMovable(page);
-	clear_bit(PG_private, &page->flags);
-	clear_bit(PG_private_2, &page->flags);
+	ClearPagePrivate(page);
+	ClearPagePrivate2(page);
 	set_page_private(page, 0);
 	page_mapcount_reset(page);
 	ClearPageHugeObject(page);
-- 
1.9.1

Re: [PATCH v2 8/8] mm/zsmalloc: use helper to clear page->flags bit

[Sergey Senozhatsky]

On (07/04/16 14:49), Ganesh Mahendran wrote:
> Date: Mon,  4 Jul 2016 14:49:59 +0800
> From: Ganesh Mahendran <opensource.ganesh@gmail.com>
> To: linux-kernel@vger.kernel.org, linux-mm@kvack.org
> Cc: akpm@linux-foundation.org, minchan@kernel.org, ngupta@vflare.org,
>  sergey.senozhatsky.work@gmail.com, rostedt@goodmis.org, mingo@redhat.com,
>  Ganesh Mahendran <opensource.ganesh@gmail.com>
> Subject: [PATCH v2 8/8] mm/zsmalloc: use helper to clear page->flags bit
> X-Mailer: git-send-email 1.9.1
> 
> user ClearPagePrivate/ClearPagePrivate2 helper to clear
> PG_private/PG_private_2 in page->flags
> 
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
> Acked-by: Minchan Kim <minchan@kernel.org>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

> ----
> v2: none
> ---
>  mm/zsmalloc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 756f839..297f25b 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -940,8 +940,8 @@ static void unpin_tag(unsigned long handle)
>  static void reset_page(struct page *page)
>  {
>  	__ClearPageMovable(page);
> -	clear_bit(PG_private, &page->flags);
> -	clear_bit(PG_private_2, &page->flags);
> +	ClearPagePrivate(page);
> +	ClearPagePrivate2(page);
>  	set_page_private(page, 0);
>  	page_mapcount_reset(page);
>  	ClearPageHugeObject(page);
> -- 
> 1.9.1
> 

Re: [PATCH v2 1/8] mm/zsmalloc: modify zs compact trace interface

[Steven Rostedt]

On Mon,  4 Jul 2016 14:49:52 +0800
Ganesh Mahendran <opensource.ganesh@gmail.com> wrote:

> This patch changes trace_zsmalloc_compact_start[end] to
> trace_zs_compact_start[end] to keep function naming consistent
> with others in zsmalloc
> 
> Also this patch remove pages_total_compacted information which
> may not really needed.
> 
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>

This looks fine to me, as long as the zs maintainers are OK with the
tracepoint changes (it is visible from user space).

-- Steve

Re: [PATCH v2 1/8] mm/zsmalloc: modify zs compact trace interface

[Minchan Kim]

Hi Ganesh,

On Mon, Jul 04, 2016 at 02:49:52PM +0800, Ganesh Mahendran wrote:
> This patch changes trace_zsmalloc_compact_start[end] to
> trace_zs_compact_start[end] to keep function naming consistent
> with others in zsmalloc
> 
> Also this patch remove pages_total_compacted information which
> may not really needed.
> 
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>

Once we decide to add event trace, I prefer getting more detailed
information which is hard to get it via /sys/block/zram/.
So, we can add trace __zs_compact as well as zs_compact with
some changes.

IOW,

zs_compact
        trace_zs_compact_start(pool->name)
        __zs_compact
                trace_zs_compact(class, scanned_obj, freed_pages)
        trace_zs_compact_end(pool->name)

Re: [PATCH v2 1/8] mm/zsmalloc: modify zs compact trace interface

[Ganesh Mahendran]

2016-07-06 10:32 GMT+08:00 Minchan Kim <minchan@kernel.org>:
> Hi Ganesh,
>
> On Mon, Jul 04, 2016 at 02:49:52PM +0800, Ganesh Mahendran wrote:
>> This patch changes trace_zsmalloc_compact_start[end] to
>> trace_zs_compact_start[end] to keep function naming consistent
>> with others in zsmalloc
>>
>> Also this patch remove pages_total_compacted information which
>> may not really needed.
>>
>> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
>
> Once we decide to add event trace, I prefer getting more detailed
> information which is hard to get it via /sys/block/zram/.
> So, we can add trace __zs_compact as well as zs_compact with
> some changes.
>
> IOW,
>
> zs_compact
>         trace_zs_compact_start(pool->name)
>         __zs_compact
>                 trace_zs_compact(class, scanned_obj, freed_pages)
>         trace_zs_compact_end(pool->name)

Thanks, I will do it.