3.19-rc3 : memcg: Hang in mount memcg

We have hit a hang on ARM64 defconfig, while running LTP tests on 3.19-rc3. We are
in the process of a git bisect and will update the results as and when we find the commit.

During the ksm ltp run, the test hangs trying to mount memcg with the following strace
output:

mount("memcg", "/dev/cgroup", "cgroup", 0, "memory") = ? ERESTARTNOINTR (To be restarted)
mount("memcg", "/dev/cgroup", "cgroup", 0, "memory") = ? ERESTARTNOINTR (To be restarted)
[ ... repeated forever ... ]

At this point, one can try mounting the memcg to verify the problem.

# mount -t cgroup -o memory memcg memcg_dir
--hangs--

Strangely, if we run the mount command from a cold boot (i.e. without running LTP first),
then it succeeds.

Quoted 15 lines · Suzuki K. Poulose

We have hit a hang on ARM64 defconfig, while running LTP tests on 3.19-rc3. We are
in the process of a git bisect and will update the results as and when we find the commit.

During the ksm ltp run, the test hangs trying to mount memcg with the following strace
output:

mount("memcg", "/dev/cgroup", "cgroup", 0, "memory") = ? ERESTARTNOINTR (To be restarted)
mount("memcg", "/dev/cgroup", "cgroup", 0, "memory") = ? ERESTARTNOINTR (To be restarted)
[ ... repeated forever ... ]

Quoted 7 lines

At this point, one can try mounting the memcg to verify the problem.

# mount -t cgroup -o memory memcg memcg_dir
--hangs--

Strangely, if we run the mount command from a cold boot (i.e. without running LTP first),
then it succeeds.

I don't know what LTP is doing and this could actually be hitting on an actual bug but if it's trying to move memcg back from unified hierarchy to an old one, that might hang - it should prolly made to just fail at that point. Anyways, any chance you can find out what happened, in terms of cgroup mounting, to memcg upto that point?

Thanks.

--
tejun

Hi

We have hit a hang on ARM64 defconfig, while running LTP tests on 3.19-rc3. We are
in the process of a git bisect and will update the results as and when we find the commit.

During the ksm ltp run, the test hangs trying to mount memcg with the following strace
output:

mount("memcg", "/dev/cgroup", "cgroup", 0, "memory") = ? ERESTARTNOINTR (To be restarted)
mount("memcg", "/dev/cgroup", "cgroup", 0, "memory") = ? ERESTARTNOINTR (To be restarted)
[ ... repeated forever ... ]

At this point, one can try mounting the memcg to verify the problem.

# mount -t cgroup -o memory memcg memcg_dir
--hangs--

Strangely, if we run the mount command from a cold boot (i.e. without running LTP first),
then it succeeds.

Upon a quick look we are hitting the following code :
kernel/cgroup.c: cgroup_mount() :

1779 for_each_subsys(ss, i) {
1780 if (!(opts.subsys_mask & (1 << i)) ||
1781 ss->root == &cgrp_dfl_root)
1782 continue;
1783
1784 if
(!percpu_ref_tryget_live(&ss->root->cgrp.self.refcnt)) {

1785                         mutex_unlock(&cgroup_mutex);
1786                         msleep(10);
1787                         ret = restart_syscall(); <=====
1788                         goto out_free;

1789 }
1790 cgroup_put(&ss->root->cgrp);
1791 }

with ss->root->cgrp.self.refct.percpu_count_ptr == __PERCPU_REF_ATOMIC_DEAD

Any ideas?

May be, we should kill the ref counter to the memory controller root in cgroup_kill_sb only if there is no children at all, neither online nor offline.

Currently, if a hierarchy doesn't have any live children when it's unmounted, the hierarchy starts dying by killing its refcnt. The expectation is that even if there are lingering dead children which are lingering due to remaining references, they'll be put in a finite amount of time. When the children are finally released, the hierarchy is destroyed and all controllers bound to it also are released.

However, for memcg, the premise that the lingering refs will be put in a finite amount time is not true. In the absense of memory pressure, dead memcg's may hang around indefinitely pinned by its pages. This unfortunately may lead to indefinite hang on the next mount attempt involving memcg as the mount logic waits for it to get released.

While we can change hierarchy destruction logic such that a hierarchy is only destroyed when it's not mounted anywhere and all its children, live or dead, are gone, this makes whether the hierarchy gets destroyed or not to be determined by factors opaque to userland. Userland may or may not get a new hierarchy on the next mount attempt. Worse, if it explicitly wants to create a new hierarchy with different options or controller compositions involving memcg, it will fail in an essentially arbitrary manner.

We want to guarantee that a hierarchy is destroyed once the conditions, unmounted and no visible children, are met. To aid it, this patch introduces a new callback cgroup_subsys->unbind() which is invoked right before the hierarchy a subsystem is bound to starts dying. memcg can implement this callback and initiate draining of remaining refs so that the hierarchy can eventually be released in a finite amount of time.

Signed-off-by Tejun Heo <[email protected]>

Cc Li Zefan <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Vladimir Davydov <[email protected]>

---
Hello,

Quoted 3 lines

May be, we should kill the ref counter to the memory controller root in cgroup_kill_sb only if there is no children at all, neither online nor offline.

Ah, thanks for the analysis, but I really wanna avoid making hierarchy destruction conditions opaque to userland. This is userland visible behavior. It shouldn't be determined by kernel internals invisible outside. This patch adds ss->unbind() which memcg can hook into to kick off draining of residual refs. If this would work, I'll add this patch to cgroup/for-3.19-fixes, possibly with stable cc'd.

Quoted 10 lines · Tejun Heo

Quoted 3 lines

May be, we should kill the ref counter to the memory controller root in cgroup_kill_sb only if there is no children at all, neither online nor offline.

Ah, thanks for the analysis, but I really wanna avoid making hierarchy destruction conditions opaque to userland. This is userland visible behavior. It shouldn't be determined by kernel internals invisible outside. This patch adds ss->unbind() which memcg can hook into to kick off draining of residual refs. If this would work, I'll add this patch to cgroup/for-3.19-fixes, possibly with stable cc'd.

How about this ->unbind() for memcg?

From d527ba1dbfdb58e1f7c7c4ee12b32ef2e5461990 Mon Sep 17 00:00:00 2001
From: Johannes Weiner <[email protected]>
Date: Sun, 11 Jan 2015 10:29:05 -0500
Subject: [patch] mm: memcontrol: zap outstanding cache/swap references during
unbind

Cgroup core assumes that any outstanding css references after offlining are temporary in nature, and e.g. mount waits for them to disappear and release the root cgroup. But leftover page cache and swapout records in an offlined memcg are only dropped when the pages get reclaimed under pressure or the swapped out pages get faulted in from other cgroups, and so those cgroup operations can hang forever.

Implement the ->unbind() callback to actively get rid of outstanding references when cgroup core wants them gone. Swap out records are deleted, such that the swap-in path will charge those pages to the faulting task. Page cache pages are moved to the root memory cgroup.

Come to think of it, I wonder how many users actually want to mount different controllers subset after unmount. Because we could allow

mounting the same subset perfectly well, even if it includes memcg. BTW, AFAIU in the unified hierarchy we won't have this problem at all, because by definition it mounts all controllers IIRC, so do we need to bother fixing this in such a complicated manner at all for the setup that's going to be deprecated anyway?

Quoted 2 lines · Vladimir Davydov

Come to think of it, I wonder how many users actually want to mount different controllers subset after unmount. Because we could allow

It wouldn't be a common use case but, on the face of it, we still support it. If we collecctively decide that once a sub cgroup is created for any controller no further hierarchy configuration for that controller is allowed, that'd work too, but one way or the other, the behavior, I believe, should be well-defined. As it currently stands, the conditions and failure mode are opaque to userland, which is never a good thing.

Quoted 5 lines

mounting the same subset perfectly well, even if it includes memcg. BTW, AFAIU in the unified hierarchy we won't have this problem at all, because by definition it mounts all controllers IIRC, so do we need to bother fixing this in such a complicated manner at all for the setup that's going to be deprecated anyway?

There will likely be a quite long transition period and if and when the old things can be removed, this added cleanup logic can go away with it. It depends on how complex the implementation would get but as long as it isn't too much and stays mostly isolated from the saner paths, I think it's probably the right thing to do.

I haven't dug deep into the cgroup core, but may be we could detach the old root in cgroup_kill_sb() and leave it dangling until the last reference to it has gone?

BTW, IIRC the problem always existed for kmem-active memory cgroups, because we never had kmem reparenting. May be, we could therefore just document somewhere that kmem accounting is highly discouraged to be used in the legacy hierarchy and merge these two patches as is to handle page cache and swap charges? We won't break anything, because it was always broken :-)

Quoted 42 lines · Tejun Heo

Currently, if a hierarchy doesn't have any live children when it's unmounted, the hierarchy starts dying by killing its refcnt. The expectation is that even if there are lingering dead children which are lingering due to remaining references, they'll be put in a finite amount of time. When the children are finally released, the hierarchy is destroyed and all controllers bound to it also are released.

However, for memcg, the premise that the lingering refs will be put in a finite amount time is not true. In the absense of memory pressure, dead memcg's may hang around indefinitely pinned by its pages. This unfortunately may lead to indefinite hang on the next mount attempt involving memcg as the mount logic waits for it to get released.

While we can change hierarchy destruction logic such that a hierarchy is only destroyed when it's not mounted anywhere and all its children, live or dead, are gone, this makes whether the hierarchy gets destroyed or not to be determined by factors opaque to userland. Userland may or may not get a new hierarchy on the next mount attempt. Worse, if it explicitly wants to create a new hierarchy with different options or controller compositions involving memcg, it will fail in an essentially arbitrary manner.

We want to guarantee that a hierarchy is destroyed once the conditions, unmounted and no visible children, are met. To aid it, this patch introduces a new callback cgroup_subsys->unbind() which is invoked right before the hierarchy a subsystem is bound to starts dying. memcg can implement this callback and initiate draining of remaining refs so that the hierarchy can eventually be released in a finite amount of time.

Signed-off-by Tejun Heo <[email protected]>

Cc Li Zefan <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Vladimir Davydov <[email protected]>

---
Hello,

Quoted 3 lines

May be, we should kill the ref counter to the memory controller root in cgroup_kill_sb only if there is no children at all, neither online nor offline.

Quoted 6 lines

Ah, thanks for the analysis, but I really wanna avoid making hierarchy destruction conditions opaque to userland. This is userland visible behavior. It shouldn't be determined by kernel internals invisible outside. This patch adds ss->unbind() which memcg can hook into to kick off draining of residual refs. If this would work, I'll add this patch to cgroup/for-3.19-fixes, possibly with stable cc'd.

How about this ->unbind() for memcg?

From d527ba1dbfdb58e1f7c7c4ee12b32ef2e5461990 Mon Sep 17 00:00:00 2001
From: Johannes Weiner <[email protected]>
Date: Sun, 11 Jan 2015 10:29:05 -0500
Subject: [patch] mm: memcontrol: zap outstanding cache/swap references during
unbind

Cgroup core assumes that any outstanding css references after offlining are temporary in nature, and e.g. mount waits for them to disappear and release the root cgroup. But leftover page cache and swapout records in an offlined memcg are only dropped when the pages get reclaimed under pressure or the swapped out pages get faulted in from other cgroups, and so those cgroup operations can hang forever.

Implement the ->unbind() callback to actively get rid of outstanding references when cgroup core wants them gone. Swap out records are deleted, such that the swap-in path will charge those pages to the faulting task. Page cache pages are moved to the root memory cgroup.

Signed-off-by Johannes Weiner <[email protected]>

---
  include/linux/swap_cgroup.h |   6 +++
  mm/memcontrol.c             | 126 ++++++++++++++++++++++++++++++++++++++++++++
  mm/swap_cgroup.c            |  38 +++++++++++++
  3 files changed, 170 insertions(+)

include/linux/swap_cgroup.h +6 -0

index 145306bdc92f..ffe0866d2997 100644
@@ -9,6 +9,7 @@ extern unsigned short swap_cgroup_cmpxchg(swp_entry_t ent,
  					unsigned short old, unsigned short new);
  extern unsigned short swap_cgroup_record(swp_entry_t ent, unsigned short id);
  extern unsigned short lookup_swap_cgroup_id(swp_entry_t ent);
+extern unsigned long swap_cgroup_zap_records(unsigned short id);
  extern int swap_cgroup_swapon(int type, unsigned long max_pages);
  extern void swap_cgroup_swapoff(int type);
 

@@ -26,6 +27,11 @@ unsigned short lookup_swap_cgroup_id(swp_entry_t ent)
  	return 0;
  }
 
+static inline unsigned long swap_cgroup_zap_records(unsigned short id)
+{
+	return 0;
+}
+
  static inline int
  swap_cgroup_swapon(int type, unsigned long max_pages)
  {

mm/memcontrol.c +126 -0

index 692e96407627..40c426add613 100644
@@ -5197,6 +5197,131 @@ static void mem_cgroup_bind(struct cgroup_subsys_state *root_css)
  		mem_cgroup_from_css(root_css)->use_hierarchy = true;
  }
 
+static void unbind_lru_list(struct mem_cgroup *memcg,
+			    struct zone *zone, enum lru_list lru)
+{
+	struct lruvec *lruvec = mem_cgroup_zone_lruvec(zone, memcg);
+	struct list_head *list = &lruvec->lists[lru];
+
+	while (!list_empty(list)) {
+		unsigned int nr_pages;
+		unsigned long flags;
+		struct page *page;
+
+		spin_lock_irqsave(&zone->lru_lock, flags);
+		if (list_empty(list)) {
+			spin_unlock_irqrestore(&zone->lru_lock, flags);
+			break;
+		}
+		page = list_last_entry(list, struct page, lru);
+		if (!get_page_unless_zero(page)) {
+			list_move(&page->lru, list);
+			spin_unlock_irqrestore(&zone->lru_lock, flags);
+			continue;
+		}
+		BUG_ON(!PageLRU(page));
+		ClearPageLRU(page);
+		del_page_from_lru_list(page, lruvec, lru);
+		spin_unlock_irqrestore(&zone->lru_lock, flags);
+
+		compound_lock(page);
+		nr_pages = hpage_nr_pages(page);
+
+		if (!mem_cgroup_move_account(page, nr_pages,
+					     memcg, root_mem_cgroup)) {
+			/*
+			 * root_mem_cgroup page counters are not used,
+			 * otherwise we'd have to charge them here.
+			 */
+			page_counter_uncharge(&memcg->memory, nr_pages);
+			if (do_swap_account)
+				page_counter_uncharge(&memcg->memsw, nr_pages);
+			css_put_many(&memcg->css, nr_pages);
+		}
+
+		compound_unlock(page);
+
+		putback_lru_page(page);
+	}
+}
+
+static void unbind_work_fn(struct work_struct *work)
+{
+	struct cgroup_subsys_state *css;
+retry:
+	drain_all_stock(root_mem_cgroup);
+
+	rcu_read_lock();
+	css_for_each_child(css, &root_mem_cgroup->css) {
+		struct mem_cgroup *memcg = mem_cgroup_from_css(css);
+
+		/* Drop references from swap-out records */
+		if (do_swap_account) {
+			long zapped;
+
+			zapped = swap_cgroup_zap_records(memcg->css.id);
+			page_counter_uncharge(&memcg->memsw, zapped);
+			css_put_many(&memcg->css, zapped);
+		}
+
+		/* Drop references from leftover LRU pages */
+		css_get(css);
+		rcu_read_unlock();
+		atomic_inc(&memcg->moving_account);
+		synchronize_rcu();
+		while (page_counter_read(&memcg->memory) -
+		       page_counter_read(&memcg->kmem) > 0) {
+			struct zone *zone;
+			enum lru_list lru;
+
+			lru_add_drain_all();
+
+			for_each_zone(zone)
+				for_each_lru(lru)
+					unbind_lru_list(memcg, zone, lru);
+
+			cond_resched();
+		}
+		atomic_dec(&memcg->moving_account);
+		rcu_read_lock();
+		css_put(css);
+	}
+	rcu_read_unlock();
+	/*
+	 * Swap-in is racy:
+	 *
+	 * #0                        #1
+	 *                           lookup_swap_cgroup_id()
+	 *                           rcu_read_lock()
+	 *                           mem_cgroup_lookup()
+	 *                           css_tryget_online()
+	 *                           rcu_read_unlock()
+	 * cgroup_kill_sb()
+	 *   !css_has_online_children()
+	 *     ->unbind()
+	 *                           page_counter_try_charge()
+	 *                           css_put()
+	 *                             css_free()
+	 *                           pc->mem_cgroup = dead memcg
+	 *                           add page to lru
+	 *
+	 * Loop until until all references established from previously
+	 * existing swap-out records have been transferred to pages on
+	 * the LRU and then uncharged from there.
+	 */
+	if (!list_empty(&root_mem_cgroup->css.children)) {
+		msleep(10);
+		goto retry;
+	}
+}
+
+static DECLARE_WORK(unbind_work, unbind_work_fn);
+
+static void mem_cgroup_unbind(struct cgroup_subsys_state *root_css)
+{
+	schedule_work(&unbind_work);
+}
+
  static u64 memory_current_read(struct cgroup_subsys_state *css,
  			       struct cftype *cft)
  {

@@ -5360,6 +5485,7 @@ struct cgroup_subsys memory_cgrp_subsys = {
  	.cancel_attach = mem_cgroup_cancel_attach,
  	.attach = mem_cgroup_move_task,
  	.bind = mem_cgroup_bind,
+	.unbind = mem_cgroup_unbind,
  	.dfl_cftypes = memory_files,
  	.legacy_cftypes = mem_cgroup_legacy_files,
  	.early_init = 0,

mm/swap_cgroup.c +31 -0

index b5f7f24b8dd1..665923a558c4 100644
@@ -140,6 +140,44 @@ unsigned short lookup_swap_cgroup_id(swp_entry_t ent)
  	return lookup_swap_cgroup(ent, NULL)->id;
  }
 
+/**
+ * swap_cgroup_zap_records - delete all swapout records of one cgroup
+ * @id: memcg id
+ *
+ * Returns the number of deleted records.
+ */
+unsigned long swap_cgroup_zap_records(unsigned short id)
+{
+	unsigned long zapped = 0;
+	unsigned int type;
+
+	for (type = 0; type < MAX_SWAPFILES; type++) {
+		struct swap_cgroup_ctrl *ctrl;
+		unsigned long flags;
+		unsigned int page;
+
+		ctrl = &swap_cgroup_ctrl[type];
+		spin_lock_irqsave(&ctrl->lock, flags);
+		for (page = 0; page < ctrl->length; page++) {
+			struct swap_cgroup *base;
+			pgoff_t offset;
+
+			base = page_address(ctrl->map[page]);
+			for (offset = 0; offset < SC_PER_PAGE; offset++) {
+				struct swap_cgroup *sc;
+
+				sc = base + offset;
+				if (sc->id == id) {
+					sc->id = 0;
+					zapped++;
+				}

From d527ba1dbfdb58e1f7c7c4ee12b32ef2e5461990 Mon Sep 17 00:00:00 2001
From: Johannes Weiner <[email protected]>
Date: Sun, 11 Jan 2015 10:29:05 -0500
Subject: [patch] mm: memcontrol: zap outstanding cache/swap references during
unbind

Cgroup core assumes that any outstanding css references after offlining are temporary in nature, and e.g. mount waits for them to disappear and release the root cgroup. But leftover page cache and swapout records in an offlined memcg are only dropped when the pages get reclaimed under pressure or the swapped out pages get faulted in from other cgroups, and so those cgroup operations can hang forever.

Implement the ->unbind() callback to actively get rid of outstanding references when cgroup core wants them gone. Swap out records are deleted, such that the swap-in path will charge those pages to the faulting task.

Page cache pages are moved to the root memory cgroup.

(quoted diff) +7 -0

+static void unbind_lru_list(struct mem_cgroup *memcg,
+			    struct zone *zone, enum lru_list lru)
+{
+	struct lruvec *lruvec = mem_cgroup_zone_lruvec(zone, memcg);
+	struct list_head *list = &lruvec->lists[lru];
+
+	while (!list_empty(list)) {

(quoted diff) +45 -0

+		if (!get_page_unless_zero(page)) {
+			list_move(&page->lru, list);
+			spin_unlock_irqrestore(&zone->lru_lock, flags);
+			continue;
+		}
+		BUG_ON(!PageLRU(page));
+		ClearPageLRU(page);
+		del_page_from_lru_list(page, lruvec, lru);
+		spin_unlock_irqrestore(&zone->lru_lock, flags);
+
+		compound_lock(page);
+		nr_pages = hpage_nr_pages(page);
+
+		if (!mem_cgroup_move_account(page, nr_pages,
+					     memcg, root_mem_cgroup)) {
+			/*
+			 * root_mem_cgroup page counters are not used,
+			 * otherwise we'd have to charge them here.
+			 */
+			page_counter_uncharge(&memcg->memory, nr_pages);
+			if (do_swap_account)
+				page_counter_uncharge(&memcg->memsw, nr_pages);
+			css_put_many(&memcg->css, nr_pages);
+		}
+
+		compound_unlock(page);
+
+		putback_lru_page(page);
+	}
+}
+
+static void unbind_work_fn(struct work_struct *work)
+{
+	struct cgroup_subsys_state *css;
+retry:
+	drain_all_stock(root_mem_cgroup);
+
+	rcu_read_lock();
+	css_for_each_child(css, &root_mem_cgroup->css) {
+		struct mem_cgroup *memcg = mem_cgroup_from_css(css);
+
+		/* Drop references from swap-out records */
+		if (do_swap_account) {
+			long zapped;
+

(quoted diff) +31 -0

+		while (page_counter_read(&memcg->memory) -
+		       page_counter_read(&memcg->kmem) > 0) {
+			struct zone *zone;
+			enum lru_list lru;
+
+			lru_add_drain_all();
+
+			for_each_zone(zone)
+				for_each_lru(lru)
+					unbind_lru_list(memcg, zone, lru);
+
+			cond_resched();
+		}
+		atomic_dec(&memcg->moving_account);
+		rcu_read_lock();
+		css_put(css);
+	}
+	rcu_read_unlock();
+	/*
+	 * Swap-in is racy:
+	 *
+	 * #0                        #1
+	 *                           lookup_swap_cgroup_id()
+	 *                           rcu_read_lock()
+	 *                           mem_cgroup_lookup()
+	 *                           css_tryget_online()
+	 *                           rcu_read_unlock()
+	 * cgroup_kill_sb()
+	 *   !css_has_online_children()
+	 *     ->unbind()
+	 *                           page_counter_try_charge()

(quoted diff) +4 -0

+		msleep(10);
+		goto retry;
+	}
+}

Currently, if a hierarchy doesn't have any live children when it's unmounted, the hierarchy starts dying by killing its refcnt. The expectation is that even if there are lingering dead children which are lingering due to remaining references, they'll be put in a finite amount of time. When the children are finally released, the hierarchy is destroyed and all controllers bound to it also are released.

However, for memcg, the premise that the lingering refs will be put in a finite amount time is not true. In the absense of memory pressure, dead memcg's may hang around indefinitely pinned by its pages. This unfortunately may lead to indefinite hang on the next mount attempt involving memcg as the mount logic waits for it to get released.

While we can change hierarchy destruction logic such that a hierarchy is only destroyed when it's not mounted anywhere and all its children, live or dead, are gone, this makes whether the hierarchy gets destroyed or not to be determined by factors opaque to userland. Userland may or may not get a new hierarchy on the next mount attempt. Worse, if it explicitly wants to create a new hierarchy with different options or controller compositions involving memcg, it will fail in an essentially arbitrary manner.

We want to guarantee that a hierarchy is destroyed once the conditions, unmounted and no visible children, are met. To aid it, this patch introduces a new callback cgroup_subsys->unbind() which is invoked right before the hierarchy a subsystem is bound to starts dying. memcg can implement this callback and initiate draining of remaining refs so that the hierarchy can eventually be released in a finite amount of time.

Signed-off-by Tejun Heo <[email protected]>

Cc Li Zefan <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Vladimir Davydov <[email protected]>

Quoted 39 lines · Suzuki K. Poulose

Hi

We have hit a hang on ARM64 defconfig, while running LTP tests on 3.19-rc3. We are
in the process of a git bisect and will update the results as and when we find the commit.

During the ksm ltp run, the test hangs trying to mount memcg with the following strace
output:

mount("memcg", "/dev/cgroup", "cgroup", 0, "memory") = ? ERESTARTNOINTR (To be restarted)
mount("memcg", "/dev/cgroup", "cgroup", 0, "memory") = ? ERESTARTNOINTR (To be restarted)
[ ... repeated forever ... ]

At this point, one can try mounting the memcg to verify the problem.

# mount -t cgroup -o memory memcg memcg_dir
--hangs--

Strangely, if we run the mount command from a cold boot (i.e. without running LTP first),
then it succeeds.

Upon a quick look we are hitting the following code :
kernel/cgroup.c: cgroup_mount() :

1779 for_each_subsys(ss, i) {
1780 if (!(opts.subsys_mask & (1 << i)) ||
1781 ss->root == &cgrp_dfl_root)
1782 continue;
1783
1784 if
(!percpu_ref_tryget_live(&ss->root->cgrp.self.refcnt)) {

1785                         mutex_unlock(&cgroup_mutex);
1786                         msleep(10);
1787                         ret = restart_syscall(); <=====
1788                         goto out_free;

Quoted 7 lines

1789 }
1790 cgroup_put(&ss->root->cgrp);
1791 }

with ss->root->cgrp.self.refct.percpu_count_ptr == __PERCPU_REF_ATOMIC_DEAD

Any ideas?

The problem is that the memory cgroup controller takes a css reference per each charged page and does not reparent charged pages on css offline, while cgroup_mount/cgroup_kill_sb expect all css references to offline cgroups to be gone soon, restarting the syscall if the ref count != 0. As a result, if you create a memory cgroup, charge some page cache to it, and then remove it, unmount/mount will hang forever.

May be, we should kill the ref counter to the memory controller root in cgroup_kill_sb only if there is no children at all, neither online nor offline.

Quoted 7 lines · Vladimir Davydov

The problem is that the memory cgroup controller takes a css reference per each charged page and does not reparent charged pages on css offline, while cgroup_mount/cgroup_kill_sb expect all css references to offline cgroups to be gone soon, restarting the syscall if the ref count != 0. As a result, if you create a memory cgroup, charge some page cache to it, and then remove it, unmount/mount will hang forever.

Quoted 4 lines

May be, we should kill the ref counter to the memory controller root in cgroup_kill_sb only if there is no children at all, neither online nor offline.

Still reproducible on 3.19-rc5 with the same setup.

From git bisect, the last good commit is :

commit 8df0c2dcf61781d2efa8e6e5b06870f6c6785735

Author: Pranith Kumar <[email protected]>
Date:   Wed Dec 10 15:42:28 2014 -0800

slab: replace smp_read_barrier_depends() with lockless_dereference()

Quoted 8 lines · Suzuki K. Poulose

Quoted 7 lines · Vladimir Davydov

The problem is that the memory cgroup controller takes a css reference per each charged page and does not reparent charged pages on css offline, while cgroup_mount/cgroup_kill_sb expect all css references to offline cgroups to be gone soon, restarting the syscall if the ref count != 0. As a result, if you create a memory cgroup, charge some page cache to it, and then remove it, unmount/mount will hang forever.

Quoted 6 lines

Quoted 4 lines

May be, we should kill the ref counter to the memory controller root in cgroup_kill_sb only if there is no children at all, neither online nor offline.

Still reproducible on 3.19-rc5 with the same setup.

Yeah, I'm seeing the same failure on my setup too.

Quoted 2 lines

From git bisect, the last good commit is :

Quoted 5 lines

commit 8df0c2dcf61781d2efa8e6e5b06870f6c6785735

Author: Pranith Kumar <[email protected]>
Date:   Wed Dec 10 15:42:28 2014 -0800

slab: replace smp_read_barrier_depends() with lockless_dereference()

So that points at 3e32cb2e0a12 ("mm: memcontrol: lockless page counters") as the offending commit.

kernel/cgroup.c +0 -0

index bb263d0caab3..9a09308c8066 100644

Quoted 8 lines · Johannes Weiner

kernel/cgroup.c +0 -0

index bb263d0caab3..9a09308c8066 100644
@@ -1819,8 +1819,11 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
 			goto out_unlock;
 		}
 

kernel/cgroup.c static struct dentry *cgroup_mount(struct file_system_type *fs_type, · line 1826

Quoted 7 lines

kernel/cgroup.c +5 -2

-		if (root->flags ^ opts.flags)
-			pr_warn("new mount options do not match the existing superblock, will be ignored\n");
+		if (root->flags ^ opts.flags) {
+			pr_warn("new mount options do not match the existing superblock\n");
+			ret = -EBUSY;
+			goto out_unlock;
+		}

Do we really need the above chunk?

Quoted 5 lines

kernel/cgroup.c +0 -1

@@ -1909,7 +1912,7 @@ static void cgroup_kill_sb(struct super_block *sb)
 	 *
 	 * And don't kill the default root.
 	 */
-	if (css_has_online_children(&root->cgrp.self) ||

kernel/cgroup.c static void cgroup_kill_sb(struct super_block *sb) · line 1917

Quoted 3 lines

kernel/cgroup.c +1 -0

+	if (!list_empty(&root->cgrp.self.children) ||
 	    root == &cgrp_dfl_root)
 		cgroup_put(&root->cgrp);

I tried to do something a bit more advanced so that eventual async release of dying children, if they happen, can also release the hierarchy but I don't think it really matters unless we can forcefully drain. So, shouldn't just the above part be enough?

From 3d7ae5aeb16ce6118d8bff17194e791339a1f06c Mon Sep 17 00:00:00 2001
From: Johannes Weiner <[email protected]>
Date: Thu, 22 Jan 2015 08:16:31 -0500
Subject: [patch] kernel: cgroup: prevent mount hang due to memory controller
lifetime

Since b2052564e66d ("mm: memcontrol: continue cache reclaim from offlined groups"), re-mounting the memory controller after using it is very likely to hang.

The cgroup core assumes that any remaining references after deleting a cgroup are temporary in nature, and synchroneously waits for them, but the above-mentioned commit has left-over page cache pin its css until it is reclaimed naturally. That being said, swap entries and charged kernel memory have been doing the same indefinite pinning forever, the bug is just more likely to trigger with left-over page cache.

Reparenting kernel memory is highly impractical, which leaves changing the cgroup assumptions to reflect this: once a controller has been mounted and used, it has internal state that is independent from mount and cgroup lifetime. It can be unmounted and remounted, but it can't be reconfigured during subsequent mounts.

Don't offline the controller root as long as there are any children, dead or alive. A remount will no longer wait for these old references to drain, it will simply mount the persistent controller state again.

Reported-by "Suzuki K. Poulose" <[email protected]>

Reported-by Will Deacon <[email protected]>

Signed-off-by Johannes Weiner <[email protected]>

Quoted 20 lines · Will Deacon

Quoted 8 lines · Suzuki K. Poulose

Quoted 7 lines · Vladimir Davydov

The problem is that the memory cgroup controller takes a css reference per each charged page and does not reparent charged pages on css offline, while cgroup_mount/cgroup_kill_sb expect all css references to offline cgroups to be gone soon, restarting the syscall if the ref count != 0. As a result, if you create a memory cgroup, charge some page cache to it, and then remove it, unmount/mount will hang forever.

Quoted 6 lines

Quoted 4 lines

May be, we should kill the ref counter to the memory controller root in cgroup_kill_sb only if there is no children at all, neither online nor offline.

Still reproducible on 3.19-rc5 with the same setup.

Yeah, I'm seeing the same failure on my setup too.

Quoted 2 lines

From git bisect, the last good commit is :

Quoted 8 lines

Quoted 5 lines

commit 8df0c2dcf61781d2efa8e6e5b06870f6c6785735

Author: Pranith Kumar <[email protected]>
Date:   Wed Dec 10 15:42:28 2014 -0800

slab: replace smp_read_barrier_depends() with lockless_dereference()

So that points at 3e32cb2e0a12 ("mm: memcontrol: lockless page counters") as the offending commit.

With b2052564e66d ("mm: memcontrol: continue cache reclaim from offlined groups"), page cache can pin an old css and its ancestors indefinitely, making that hang in a second mount() very likely.

However, swap entries have also been doing that for quite a while now, and as Vladimir pointed out, the same is true for kernel memory. This latest change just makes this existing bug easier to trigger.

I think we have to update the lifetime rules to reflect reality here:
memory and swap lifetime is indefinite, so once the memory controller is used, it has state that is independent from whether its mounted or not. We can support an identical remount, but have to fail mounting with new parameters that would change the behavior of the controller.

Suzuki, Will, could you give the following patch a shot?

Tejun, would that route be acceptable to you?

Thanks

---
From c5e88d02d185c52748df664aa30a2c5f8949b0f7 Mon Sep 17 00:00:00 2001
From: Johannes Weiner <[email protected]>
Date: Thu, 22 Jan 2015 08:16:31 -0500
Subject: [patch] kernel: cgroup: prevent mount hang due to memory controller
lifetime

Don't offline the controller root as long as there are any children, dead or alive. A remount will no longer wait for these old references to drain, it will simply mount the persistent controller state again.

Reported-by "Suzuki K. Poulose" <[email protected]>

Reported-by Will Deacon <[email protected]>

Signed-off-by Johannes Weiner <[email protected]>