Date:	Mon, 6 Jul 2015 16:52:41 +0900
From:	Minchan Kim <minchan@...nel.org>
To:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org,
	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [RFC][PATCHv4 6/7] zsmalloc: account the number of compacted
 pages

Hi Sergey,

On Wed, Jul 01, 2015 at 04:29:52PM +0900, Sergey Senozhatsky wrote:
> On (06/30/15 21:35), Sergey Senozhatsky wrote:
> [..]
> >  	if (src_page)
> >  		putback_zspage(pool, class, src_page);
> >  
> > -	pool->num_migrated += cc.nr_migrated;
> > +	cc.nr_migrated /= get_maxobj_per_zspage(class->size,
> > +			class->pages_per_zspage);
> > +
> > +	pool->num_migrated += cc.nr_migrated *
> > +		get_pages_per_zspage(class->size);
> >  
> >  	spin_unlock(&class->lock);
> 
> Oh, well. This is bloody wrong, sorry. We don't pick up src_page-s that we
> can completely drain. Thus, the fact that we can't compact (!zs_can_compact())
> anymore doesn't mean that we actually have released any zspages.
> 
> So...
> 
> (a) we can isolate_source_page() more accurately -- iterate list and
> look for pages that have ->inuse less or equal to the amount of unused
> objects. So we can guarantee that this particular zspage will be released
> at the end. It adds O(n) every time we isolate_source_page(), because
> the number of unused objects changes. But it's sort of worth it, I
> think. Otherwise we still can move M objects w/o releasing any pages
> after all. If we consider compaction as a slow path (and I think we do)
> then this option doesn't look so bad.
> 
> 
> 
> (b) if (a) is not an option, then we need to know that we have drained the
> src_page. And it seems that the easiest way to do it is to change
> 'void putback_zspage(...)' to 'bool putback_zspage(...)' and return `true'
> from putback_zspage() when putback resulted in free_zspage() (IOW, the page
> was ZS_EMPTY). And in __zs_compact() do something like

Just nit:

putback_zspage is not related to "free zspage" so I want to handle it in
caller. For it, putback_zspage could return fullness type page is added.
Something like that.

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 99133e8..32e3bb9 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1653,7 +1653,17 @@ static struct page *isolate_target_page(struct size_class *class)
 	return page;
 }
 
-static void putback_zspage(struct zs_pool *pool, struct size_class *class,
+/*
+ * putback_zspage - add @first_page into right class's fullness list
+ * @pool: target pool
+ * @class: destination class
+ * @first_page: target page
+ *
+ * Return:
+ * The fullness_group @fist_page is added
+ */
+static enum fullness_group putback_zspage(struct zs_pool *pool,
+				struct size_class *class,
 				struct page *first_page)
 {
 	enum fullness_group fullness;
@@ -1672,6 +1682,8 @@ static void putback_zspage(struct zs_pool *pool, struct size_class *class,
 
 		free_zspage(first_page);
 	}
+
+	return fullness;
 }
 
 static struct page *isolate_source_page(struct size_class *class)
@@ -1707,11 +1719,13 @@ static unsigned long zs_can_compact(struct size_class *class)
 	return obj_wasted * get_pages_per_zspage(class->size);
 }
 
-static void __zs_compact(struct zs_pool *pool, struct size_class *class)
+static unsigned long __zs_compact(struct zs_pool *pool,
+				struct size_class *class)
 {
 	struct zs_compact_control cc;
 	struct page *src_page;
 	struct page *dst_page = NULL;
+	unsigned long nr_freed = 0;
 
 	cc.nr_migrated = 0;
 	spin_lock(&class->lock);
@@ -1742,7 +1756,8 @@ static void __zs_compact(struct zs_pool *pool, struct size_class *class)
 			break;
 
 		putback_zspage(pool, class, dst_page);
-		putback_zspage(pool, class, src_page);
+		if (ZS_EMPTY == putback_zspage(pool, class, src_page))
+			nr_freed += get_pages_per_zspage(class->size);
 		spin_unlock(&class->lock);
 		cond_resched();
 		spin_lock(&class->lock);
@@ -1758,22 +1773,36 @@ static void __zs_compact(struct zs_pool *pool, struct size_class *class)
 		get_pages_per_zspage(class->size);
 
 	spin_unlock(&class->lock);
+
+	return nr_freed;
 }
 
+/*
+ * zs_compact - migrate objects and free empty zspage in the @pool
+ * @pool: target pool for compaction
+ *
+ * Return:
+ * The number of freed pages by compaction
+ */
 unsigned long zs_compact(struct zs_pool *pool)
 {
 	int i;
 	struct size_class *class;
+	unsigned long nr_freed = 0;
 
 	for (i = zs_size_classes - 1; i >= 0; i--) {
+
 		class = pool->size_class[i];
 		if (!class)
 			continue;
+
 		if (class->index != i)
 			continue;
-		__zs_compact(pool, class);
+
+		nr_freed += __zs_compact(pool, class);
 	}
-	return pool->num_migrated;
+
+	return nr_freed;
 }
 EXPORT_SYMBOL_GPL(zs_compact);
 
-- 
1.9.3



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

Powered by blists - more mailing lists