Hi,

On Mon 12-09-11 12:57:18, Johannes Weiner wrote:

Memory control groups are currently bolted onto the side of
traditional memory management in places where better integration would
be preferrable.  To reclaim memory, for example, memory control groups
maintain their own LRU list and reclaim strategy aside from the global
per-zone LRU list reclaim.  But an extra list head for each existing
page frame is expensive and maintaining it requires additional code.

This patchset disables the global per-zone LRU lists on memory cgroup
configurations and converts all its users to operate on the per-memory
cgroup lists instead.  As LRU pages are then exclusively on one list,
this saves two list pointers for each page frame in the system:

page_cgroup array size with 4G physical memory

  vanilla: [    0.000000] allocated 31457280 bytes of page_cgroup
  patched: [    0.000000] allocated 15728640 bytes of page_cgroup

At the same time, system performance for various workloads is
unaffected:

100G sparse file cat, 4G physical memory, 10 runs, to test for code
bloat in the traditional LRU handling and kswapd & direct reclaim
paths, without/with the memory controller configured in

  vanilla: 71.603(0.207) seconds
  patched: 71.640(0.156) seconds

  vanilla: 79.558(0.288) seconds
  patched: 77.233(0.147) seconds

100G sparse file cat in 1G memory cgroup, 10 runs, to test for code
bloat in the traditional memory cgroup LRU handling and reclaim path

  vanilla: 96.844(0.281) seconds
  patched: 94.454(0.311) seconds

4 unlimited memcgs running kbuild -j32 each, 4G physical memory, 500M
swap on SSD, 10 runs, to test for regressions in kswapd & direct
reclaim using per-memcg LRU lists with multiple memcgs and multiple
allocators within each memcg

  vanilla: 717.722(1.440) seconds [ 69720.100(11600.835) majfaults ]
  patched: 714.106(2.313) seconds [ 71109.300(14886.186) majfaults ]

16 unlimited memcgs running kbuild, 1900M hierarchical limit, 500M
swap on SSD, 10 runs, to test for regressions in hierarchical memcg
setups

  vanilla: 2742.058(1.992) seconds [ 26479.600(1736.737) majfaults ]
  patched: 2743.267(1.214) seconds [ 27240.700(1076.063) majfaults ]

I guess you want to have this in the first patch to have it for
reference once it gets to the tree, right? I have no objections but it
seems unrelated to the patch and so it might be confusing a bit. I
haven't seen other patches in the series so there is probably no better
place to put this.

This patch:

There are currently two different implementations of iterating over a
memory cgroup hierarchy tree.

Consolidate them into one worker function and base the convenience
looping-macros on top of it.

Signed-off-by: Johannes Weiner <redacted>

Looks mostly good. There is just one issue I spotted and I guess we
want some comments. After the issue is fixed:
Reviewed-by: Michal Hocko <redacted>

---
 mm/memcontrol.c |  196 ++++++++++++++++++++----------------------------------
 1 files changed, 73 insertions(+), 123 deletions(-)

Nice diet.

Shouldn't we have a similar comment that we have to keep a reference to
root if non-NULL. A mention about remember parameter and what is it used
for (hierarchical reclaim) would be helpful as well.

/*
 * Find a next cgroup under the hierarchy tree with the given root (or
 * root_mem_cgroup if NULL) starting from the given prev (iterator)
 * position and releasing a reference to it. Start from the root if
 * iterator is NULL.
 * Ignore iterator position if remember is true and follow with the
 * last_scanned_child instead and remember the new value (used during
 * hierarchical reclaim).
 * Caller is supposed to grab a reference to the root (if non NULL) before
 * it calls us for the first time.
 *
 * Returns a cgroup with increased reference count (except for the root)
 * or NULL if there are no more groups to visit.
 *
 * Use for_each_mem_cgroup_tree and for_each_mem_cgroup instead and
 * mem_cgroup_iter_break for the final clean up if you are using this
 * function directly.
 */

-static struct mem_cgroup *mem_cgroup_start_loop(struct mem_cgroup *memcg)
+static struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
+					  struct mem_cgroup *prev,
+					  bool remember)

[...]

Hmm, we are returning total but it doesn't get set to ret for shrinking
case so we are alway returning 0. You want to move the line bellow up.

 		total += ret;
 		if (check_soft) {
 			if (!res_counter_soft_limit_excess(&root_memcg->res))
-				return total;
+				break;
 		} else if (mem_cgroup_margin(root_memcg))
-			return total;
+			break;
 	}
+	mem_cgroup_iter_break(root_memcg, victim);
 	return total;
 }

[...]

Thanks
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

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