On Thu, 25 Aug 2011 10:37:05 -0700 (PDT)
Dan Magenheimer [off-list ref] wrote:

quoted

From: KAMEZAWA Hiroyuki [mailto:kamezawa.hiroyu@jp.fujitsu.com]
Subject: Re: Subject: [PATCH V7 2/4] mm: frontswap: core code

quoted

BTW, Do I have a chance to implement frontswap accounting per cgroup
(under memcg) ? Or Do I need to enable/disale switch for frontswap per memcg ?
Do you think it is worth to do ?

I'm not very familiar with cgroups or memcg but I think it may be possible
to implement transcendent memory with cgroup as the "guest" and the default
cgroup as the "host" to allow for more memory elasticity for cgroups.
(See http://lwn.net/Articles/454795/ for a good overview of all of
transcendent memory.)

Ok, I'll see it.

I just wonder following case.

Assume 2 memcgs.
	memcg X: memory limit = 300M.
	memcg Y: memory limit = 300M.

This limitation is done for performance isolation.
When using frontswap, X and Y can cause resource confliction in frontswap and
performance of X and Y cannot be predictable.

quoted

quoted

+/*
+ * This global enablement flag reduces overhead on systems where frontswap_ops
+ * has not been registered, so is preferred to the slower alternative: a
+ * function call that checks a non-global.
+ */
+int frontswap_enabled;
+EXPORT_SYMBOL(frontswap_enabled);
+
+/* useful stats available in /sys/kernel/mm/frontswap */
+static unsigned long frontswap_gets;
+static unsigned long frontswap_succ_puts;
+static unsigned long frontswap_failed_puts;
+static unsigned long frontswap_flushes;
+

What lock guard these ? swap_lock ?

These are informational statistics so do not need to be protected
by a lock or an atomic-type.  If an increment is lost due to a cpu
race, it is not a problem.

Hmm...Personally, I don't like incorrect counters. Could you add comments ?
Or How anout using percpu_counter ? (see lib/percpu_counter.c)

quoted

quoted

+/* Called when a swap device is swapon'd */
+void __frontswap_init(unsigned type)
+{
+	struct swap_info_struct *sis = swap_info[type];
+
+	BUG_ON(sis == NULL);
+	if (sis->frontswap_map == NULL)
+		return;
+	if (frontswap_enabled)
+		(*frontswap_ops.init)(type);
+}
+EXPORT_SYMBOL(__frontswap_init);
+
+/*
+ * "Put" data from a page to frontswap and associate it with the page's
+ * swaptype and offset.  Page must be locked and in the swap cache.
+ * If frontswap already contains a page with matching swaptype and
+ * offset, the frontswap implmentation may either overwrite the data
+ * and return success or flush the page from frontswap and return failure
+ */

What lock should be held to guard global variables ? swap_lock ?

Which global variables do you mean and in what routines?  I think the
page lock is required for put/get (as documented in the comments)
but not the swap_lock.

My concern was race in counters. Even you allow race in frontswap_succ_puts++,

Don't you need some lock for
	sis->frontswap_pages++
	sis->frontswap_pages--
?

Thanks,
-Kame





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