Memory controller soft limit patches (v5)

(quoted diff) +32 -0

+/**
+ * Get the difference between the usage and the soft limit
+ * @cnt: The counter
+ *
+ * Returns 0 if usage is less than or equal to soft limit
+ * The difference between usage and soft limit, otherwise.
+ */
+static inline unsigned long long
+res_counter_soft_limit_excess(struct res_counter *cnt)
+{
+	unsigned long long excess;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	if (cnt->usage <= cnt->soft_limit)
+		excess = 0;
+	else
+		excess = cnt->usage - cnt->soft_limit;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return excess;
+}
 
...
 
+static inline bool res_counter_check_under_soft_limit(struct res_counter *cnt)
+{
+	bool ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	ret = res_counter_soft_limit_check_locked(cnt);
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return ret;
+}
 
...
 
+static inline int

Quoted 37 lines · On Thu, 12 Mar 2009 23:26:20 +0530 Balbir Singh <[email protected]>…

include/linux/res_counter.h +31 -0

+/**
+ * Get the difference between the usage and the soft limit
+ * @cnt: The counter
+ *
+ * Returns 0 if usage is less than or equal to soft limit
+ * The difference between usage and soft limit, otherwise.
+ */
+static inline unsigned long long
+res_counter_soft_limit_excess(struct res_counter *cnt)
+{
+	unsigned long long excess;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	if (cnt->usage <= cnt->soft_limit)
+		excess = 0;
+	else
+		excess = cnt->usage - cnt->soft_limit;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return excess;
+}
 
...
 
+static inline bool res_counter_check_under_soft_limit(struct res_counter *cnt)
+{
+	bool ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	ret = res_counter_soft_limit_check_locked(cnt);
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return ret;
+}
 
...

include/linux/res_counter.h static inline int res_counter_set_limit(struct res_counter *cnt, · line 237

Quoted 11 lines

include/linux/res_counter.h +11 -0

+static inline int
+res_counter_set_soft_limit(struct res_counter *cnt,
+				unsigned long long soft_limit)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	cnt->soft_limit = soft_limit;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return 0;
+}

These functions look too large to be inlined?

Feature: Organize cgroups over soft limit in a RB-Tree

From: Balbir Singh <[email protected]>

Changelog v5...v4
1. res_counter_uncharge has an additional parameter to indicate if the counter was over its soft limit, before uncharge.

Changelog v4...v3
1. Optimizations to ensure we don't uncessarily get res_counter values
2. Fixed a bug in usage of time_after()

Changelog v3...v2
1. Add only the ancestor to the RB-Tree
2. Use css_tryget/css_put instead of mem_cgroup_get/mem_cgroup_put

Changelog v2...v1
1. Add support for hierarchies
2. The res_counter that is highest in the hierarchy is returned on soft limit being exceeded. Since we do hierarchical reclaim and add all groups exceeding their soft limits, this approach seems to work well in practice.

This patch introduces a RB-Tree for storing memory cgroups that are over their soft limit. The overall goal is to

1. Add a memory cgroup to the RB-Tree when the soft limit is exceeded. We are careful about updates, updates take place only after a particular time interval has passed
2. We remove the node from the RB-Tree when the usage goes below the soft limit

The next set of patches will exploit the RB-Tree to get the group that is over its soft limit by the largest amount and reclaim from it, when we face memory contention.

...

+#define MEM_CGROUP_TREE_UPDATE_INTERVAL (HZ/4)

From a general design point of view it would be better to pace this

Quoted 32 lines · On Thu, 12 Mar 2009 23:26:25 +0530 Balbir Singh <[email protected]>…

Feature: Organize cgroups over soft limit in a RB-Tree

From: Balbir Singh <[email protected]>

Changelog v5...v4
1. res_counter_uncharge has an additional parameter to indicate if the counter was over its soft limit, before uncharge.

Changelog v4...v3
1. Optimizations to ensure we don't uncessarily get res_counter values
2. Fixed a bug in usage of time_after()

Changelog v3...v2
1. Add only the ancestor to the RB-Tree
2. Use css_tryget/css_put instead of mem_cgroup_get/mem_cgroup_put

Changelog v2...v1
1. Add support for hierarchies
2. The res_counter that is highest in the hierarchy is returned on soft limit being exceeded. Since we do hierarchical reclaim and add all groups exceeding their soft limits, this approach seems to work well in practice.

This patch introduces a RB-Tree for storing memory cgroups that are over their soft limit. The overall goal is to

1. Add a memory cgroup to the RB-Tree when the soft limit is exceeded. We are careful about updates, updates take place only after a particular time interval has passed
2. We remove the node from the RB-Tree when the usage goes below the soft limit

Quoted 8 lines

The next set of patches will exploit the RB-Tree to get the group that is over its soft limit by the largest amount and reclaim from it, when we face memory contention.

...

+#define MEM_CGROUP_TREE_UPDATE_INTERVAL (HZ/4)

Wall-clock time is a quite poor way of tracking system activity. There's little correlation between the two things.

Quoted 1 line

From a general design point of view it would be better to pace this

polling activity in a manner which correlates with the amount of system activity. For example, "once per 100,000 pages scanned" is much more adaptive than "once per 250 milliseconds".

mm/memcontrol.c mem_cgroup_create(struct cgroup_subsys *ss, struct cgroup *cont) · line 2564

Quoted 7 lines

...

Quoted 1 line

(quoted diff) +0 -0

@@ -2459,6 +2561,7 @@ mem_cgroup_create(struct cgroup_subsys *ss, struct cgroup *cont)

mm/memcontrol.c +1 -0

 	if (cont->parent == NULL) {
 		enable_swap_cgroup();
 		parent = NULL;
+		mem_cgroup_soft_limit_tree = RB_ROOT;

This can be done at compile time?

mm/memcontrol.c struct mem_cgroup_lru_info { · line 138

Quoted 7 lines

/*
+ * Cgroups above their limits are maintained in a RB-Tree, independent of
+ * their hierarchy representation
+ */

mm/memcontrol.c +3 -0

+
+static struct rb_root mem_cgroup_soft_limit_tree;
+static DEFINE_SPINLOCK(memcg_soft_limit_tree_lock);

I have objection to this.
Please don't use global spin lock.

Quoted 10 lines

mm/memcontrol.c struct mem_cgroup_lru_info { · line 138

Quoted 7 lines

/*
+ * Cgroups above their limits are maintained in a RB-Tree, independent of
+ * their hierarchy representation
+ */

mm/memcontrol.c +3 -0

+
+static struct rb_root mem_cgroup_soft_limit_tree;
+static DEFINE_SPINLOCK(memcg_soft_limit_tree_lock);

I have objection to this.
Please don't use global spin lock.

We need a global data structure, per node, per zone is no good, since the limits (soft limit in this case) is for the entire cgroup.

Quoted 11 lines

Quoted 10 lines

mm/memcontrol.c struct mem_cgroup_lru_info { · line 138

Quoted 7 lines

/*
+ * Cgroups above their limits are maintained in a RB-Tree, independent of
+ * their hierarchy representation
+ */

mm/memcontrol.c +3 -0

+
+static struct rb_root mem_cgroup_soft_limit_tree;
+static DEFINE_SPINLOCK(memcg_soft_limit_tree_lock);

I have objection to this.
Please don't use global spin lock.

Quoted 2 lines

We need a global data structure, per node, per zone is no good, since the limits (soft limit in this case) is for the entire cgroup.

this smell the data structure is wrong.

rb-tree soring is one of efficient reclaiming technique. but global lock bust due to this patch's good side.

if its updating is really rare, rcu is better?
or couldn't you select another data structure?

Feature: Organize cgroups over soft limit in a RB-Tree

From: Balbir Singh <[email protected]>

Changelog v5...v4
1. res_counter_uncharge has an additional parameter to indicate if the counter was over its soft limit, before uncharge.

Changelog v4...v3
1. Optimizations to ensure we don't uncessarily get res_counter values
2. Fixed a bug in usage of time_after()

Changelog v3...v2
1. Add only the ancestor to the RB-Tree
2. Use css_tryget/css_put instead of mem_cgroup_get/mem_cgroup_put

Changelog v2...v1
1. Add support for hierarchies
2. The res_counter that is highest in the hierarchy is returned on soft limit being exceeded. Since we do hierarchical reclaim and add all groups exceeding their soft limits, this approach seems to work well in practice.

This patch introduces a RB-Tree for storing memory cgroups that are over their
soft limit. The overall goal is to

1. Add a memory cgroup to the RB-Tree when the soft limit is exceeded. We are careful about updates, updates take place only after a
particular
time interval has passed
2. We remove the node from the RB-Tree when the usage goes below the soft limit

The next set of patches will exploit the RB-Tree to get the group that is over its soft limit by the largest amount and reclaim from it, when we face memory contention.

Signed-off-by Balbir Singh <[email protected]>

---

 include/linux/res_counter.h |    6 +-
 kernel/res_counter.c        |   18 +++++
 mm/memcontrol.c             |  141
++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 143 insertions(+), 22 deletions(-)

include/linux/res_counter.h +2 -1

index 5c821fd..5bbf8b1 100644
@@ -112,7 +112,8 @@ void res_counter_init(struct res_counter *counter,
 struct res_counter *parent);
 int __must_check res_counter_charge_locked(struct res_counter *counter,
 		unsigned long val);
 int __must_check res_counter_charge(struct res_counter *counter,
-		unsigned long val, struct res_counter **limit_fail_at);
+		unsigned long val, struct res_counter **limit_fail_at,
+		struct res_counter **soft_limit_at);
 
 /*

* uncharge - tell that some portion of the resource is released

include/linux/res_counter.h +2 -1

@@ -125,7 +126,8 @@ int __must_check res_counter_charge(struct res_counter
 *counter,
  */
 
 void res_counter_uncharge_locked(struct res_counter *counter, unsigned
 long val);
-void res_counter_uncharge(struct res_counter *counter, unsigned long
 val);
+void res_counter_uncharge(struct res_counter *counter, unsigned long val,
+				bool *was_soft_limit_excess);

static inline bool res_counter_limit_check_locked(struct res_counter
*cnt)
{

kernel/res_counter.c +11 -1

index 4e6dafe..51ec438 100644
@@ -37,17 +37,27 @@ int res_counter_charge_locked(struct res_counter
 *counter, unsigned long val)
 }
 
 int res_counter_charge(struct res_counter *counter, unsigned long val,
-			struct res_counter **limit_fail_at)
+			struct res_counter **limit_fail_at,
+			struct res_counter **soft_limit_fail_at)
 {
 	int ret;
 	unsigned long flags;
 	struct res_counter *c, *u;
 
 	*limit_fail_at = NULL;
+	if (soft_limit_fail_at)
+		*soft_limit_fail_at = NULL;
 	local_irq_save(flags);
 	for (c = counter; c != NULL; c = c->parent) {
 		spin_lock(&c->lock);
 		ret = res_counter_charge_locked(c, val);
+		/*
+		 * With soft limits, we return the highest ancestor
+		 * that exceeds its soft limit
+		 */
+		if (soft_limit_fail_at &&
+			!res_counter_soft_limit_check_locked(c))
+			*soft_limit_fail_at = c;
 		spin_unlock(&c->lock);
 		if (ret < 0) {

*limit_fail_at = c;

+/*
* The memory controller data structure. The memory controller controls
both
* page cache and RSS per cgroup. We would eventually like to provide
* statistics based on the statistics developed by Rik Van Riel for
clock-pro,

kernel/res_counter.c +4 -0

@@ -176,12 +185,20 @@ struct mem_cgroup {
 
 	unsigned int	swappiness;
 
+	struct rb_node mem_cgroup_node;		/* RB tree node */
+	unsigned long long usage_in_excess;	/* Set to the value by which */
+						/* the soft limit is exceeded*/
+	unsigned long last_tree_update;		/* Last time the tree was */

kernel/res_counter.c +59 -0

 enum charge_type {
 	MEM_CGROUP_CHARGE_TYPE_CACHE = 0,
 	MEM_CGROUP_CHARGE_TYPE_MAPPED,

@@ -214,6 +231,41 @@ static void mem_cgroup_get(struct mem_cgroup *mem);
 static void mem_cgroup_put(struct mem_cgroup *mem);
 static struct mem_cgroup *parent_mem_cgroup(struct mem_cgroup *mem);
 
+static void mem_cgroup_insert_exceeded(struct mem_cgroup *mem)
+{
+	struct rb_node **p = &mem_cgroup_soft_limit_tree.rb_node;
+	struct rb_node *parent = NULL;
+	struct mem_cgroup *mem_node;
+	unsigned long flags;
+
+	spin_lock_irqsave(&memcg_soft_limit_tree_lock, flags);
+	while (*p) {
+		parent = *p;
+		mem_node = rb_entry(parent, struct mem_cgroup, mem_cgroup_node);
+		if (mem->usage_in_excess < mem_node->usage_in_excess)
+			p = &(*p)->rb_left;
+		/*
+		 * We can't avoid mem cgroups that are over their soft
+		 * limit by the same amount
+		 */
+		else if (mem->usage_in_excess >= mem_node->usage_in_excess)
+			p = &(*p)->rb_right;
+	}
+	rb_link_node(&mem->mem_cgroup_node, parent, p);
+	rb_insert_color(&mem->mem_cgroup_node,
+			&mem_cgroup_soft_limit_tree);
+	mem->last_tree_update = jiffies;
+	spin_unlock_irqrestore(&memcg_soft_limit_tree_lock, flags);
+}
+
+static void mem_cgroup_remove_exceeded(struct mem_cgroup *mem)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&memcg_soft_limit_tree_lock, flags);
+	rb_erase(&mem->mem_cgroup_node, &mem_cgroup_soft_limit_tree);
+	spin_unlock_irqrestore(&memcg_soft_limit_tree_lock, flags);
+}
+
 static void mem_cgroup_charge_statistics(struct mem_cgroup *mem,
 					 struct page_cgroup *pc,
 					 bool charge)

@@ -897,6 +949,40 @@ static void record_last_oom(struct mem_cgroup *mem)
 	mem_cgroup_walk_tree(mem, NULL, record_last_oom_cb);
 }
 
+static void mem_cgroup_check_and_update_tree(struct mem_cgroup *mem,
+						bool time_check)
+{
+	unsigned long long prev_usage_in_excess, new_usage_in_excess;
+	bool updated_tree = false;
+	unsigned long next_update = 0;
+	unsigned long flags;
+
+	prev_usage_in_excess = mem->usage_in_excess;
+
+	if (time_check)
+		next_update = mem->last_tree_update +
+				MEM_CGROUP_TREE_UPDATE_INTERVAL;
+
+	if (!time_check || time_after(jiffies, next_update)) {
+		new_usage_in_excess = res_counter_soft_limit_excess(&mem->res);
+		if (prev_usage_in_excess) {
+			mem_cgroup_remove_exceeded(mem);
+			updated_tree = true;
+		}
+		if (!new_usage_in_excess)
+			goto done;
+		mem_cgroup_insert_exceeded(mem);
+		updated_tree = true;

Quoted 110 lines · Balbir Singh

Feature: Organize cgroups over soft limit in a RB-Tree

From: Balbir Singh <[email protected]>

Changelog v5...v4
1. res_counter_uncharge has an additional parameter to indicate if the counter was over its soft limit, before uncharge.

Changelog v4...v3
1. Optimizations to ensure we don't uncessarily get res_counter values
2. Fixed a bug in usage of time_after()

Changelog v3...v2
1. Add only the ancestor to the RB-Tree
2. Use css_tryget/css_put instead of mem_cgroup_get/mem_cgroup_put

Changelog v2...v1
1. Add support for hierarchies
2. The res_counter that is highest in the hierarchy is returned on soft limit being exceeded. Since we do hierarchical reclaim and add all groups exceeding their soft limits, this approach seems to work well in practice.

This patch introduces a RB-Tree for storing memory cgroups that are over their
soft limit. The overall goal is to

1. Add a memory cgroup to the RB-Tree when the soft limit is exceeded. We are careful about updates, updates take place only after a
particular
time interval has passed
2. We remove the node from the RB-Tree when the usage goes below the soft limit

The next set of patches will exploit the RB-Tree to get the group that is over its soft limit by the largest amount and reclaim from it, when we face memory contention.

Signed-off-by Balbir Singh <[email protected]>

---

 include/linux/res_counter.h |    6 +-
 kernel/res_counter.c        |   18 +++++
 mm/memcontrol.c             |  141
++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 143 insertions(+), 22 deletions(-)

include/linux/res_counter.h +2 -1

index 5c821fd..5bbf8b1 100644
@@ -112,7 +112,8 @@ void res_counter_init(struct res_counter *counter,
 struct res_counter *parent);
 int __must_check res_counter_charge_locked(struct res_counter *counter,
 		unsigned long val);
 int __must_check res_counter_charge(struct res_counter *counter,
-		unsigned long val, struct res_counter **limit_fail_at);
+		unsigned long val, struct res_counter **limit_fail_at,
+		struct res_counter **soft_limit_at);
 
 /*

* uncharge - tell that some portion of the resource is released

include/linux/res_counter.h +2 -1

@@ -125,7 +126,8 @@ int __must_check res_counter_charge(struct res_counter
 *counter,
  */
 
 void res_counter_uncharge_locked(struct res_counter *counter, unsigned
 long val);
-void res_counter_uncharge(struct res_counter *counter, unsigned long
 val);
+void res_counter_uncharge(struct res_counter *counter, unsigned long val,
+				bool *was_soft_limit_excess);

static inline bool res_counter_limit_check_locked(struct res_counter
*cnt)
{

kernel/res_counter.c +11 -1

index 4e6dafe..51ec438 100644
@@ -37,17 +37,27 @@ int res_counter_charge_locked(struct res_counter
 *counter, unsigned long val)
 }
 
 int res_counter_charge(struct res_counter *counter, unsigned long val,
-			struct res_counter **limit_fail_at)
+			struct res_counter **limit_fail_at,
+			struct res_counter **soft_limit_fail_at)
 {
 	int ret;
 	unsigned long flags;
 	struct res_counter *c, *u;
 
 	*limit_fail_at = NULL;
+	if (soft_limit_fail_at)
+		*soft_limit_fail_at = NULL;
 	local_irq_save(flags);
 	for (c = counter; c != NULL; c = c->parent) {
 		spin_lock(&c->lock);
 		ret = res_counter_charge_locked(c, val);
+		/*
+		 * With soft limits, we return the highest ancestor
+		 * that exceeds its soft limit
+		 */
+		if (soft_limit_fail_at &&
+			!res_counter_soft_limit_check_locked(c))
+			*soft_limit_fail_at = c;
 		spin_unlock(&c->lock);
 		if (ret < 0) {

Quoted 1 line

*limit_fail_at = c;

Do we need these all check at every call ?
If you just check tree update once per HZ/?? , please check this onece per HZ/??. And please help people who doesn't use softlimit i.e. who doesn't mount cgroup but have to use configured kernel.

kernel/res_counter.c

Quoted 3 lines

kernel/res_counter.c +3 -0

+static struct rb_root mem_cgroup_soft_limit_tree;
+static DEFINE_SPINLOCK(memcg_soft_limit_tree_lock);
+

Can't we breakd down this lock to per node or per cpu or...?

Quoted 22 lines

+/*
* The memory controller data structure. The memory controller controls
both
* page cache and RSS per cgroup. We would eventually like to provide
* statistics based on the statistics developed by Rik Van Riel for
clock-pro,

kernel/res_counter.c +6 -0

@@ -176,12 +185,20 @@ struct mem_cgroup {
 
 	unsigned int	swappiness;
 
+	struct rb_node mem_cgroup_node;		/* RB tree node */
+	unsigned long long usage_in_excess;	/* Set to the value by which */
+						/* the soft limit is exceeded*/
+	unsigned long last_tree_update;		/* Last time the tree was */
+						/* updated in jiffies     */
+
 	/*
 	 * statistics. This must be placed at the end of memcg.
 	 */
 	struct mem_cgroup_stat stat;
 };
 

kernel/res_counter.c struct mem_cgroup { · line 201

Quoted 2 lines

kernel/res_counter.c +2 -0

+#define	MEM_CGROUP_TREE_UPDATE_INTERVAL		(HZ/4)
+

Why HZ/4 again.

Quoted 77 lines

kernel/res_counter.c +63 -0

 enum charge_type {
 	MEM_CGROUP_CHARGE_TYPE_CACHE = 0,
 	MEM_CGROUP_CHARGE_TYPE_MAPPED,

@@ -214,6 +231,41 @@ static void mem_cgroup_get(struct mem_cgroup *mem);
 static void mem_cgroup_put(struct mem_cgroup *mem);
 static struct mem_cgroup *parent_mem_cgroup(struct mem_cgroup *mem);
 
+static void mem_cgroup_insert_exceeded(struct mem_cgroup *mem)
+{
+	struct rb_node **p = &mem_cgroup_soft_limit_tree.rb_node;
+	struct rb_node *parent = NULL;
+	struct mem_cgroup *mem_node;
+	unsigned long flags;
+
+	spin_lock_irqsave(&memcg_soft_limit_tree_lock, flags);
+	while (*p) {
+		parent = *p;
+		mem_node = rb_entry(parent, struct mem_cgroup, mem_cgroup_node);
+		if (mem->usage_in_excess < mem_node->usage_in_excess)
+			p = &(*p)->rb_left;
+		/*
+		 * We can't avoid mem cgroups that are over their soft
+		 * limit by the same amount
+		 */
+		else if (mem->usage_in_excess >= mem_node->usage_in_excess)
+			p = &(*p)->rb_right;
+	}
+	rb_link_node(&mem->mem_cgroup_node, parent, p);
+	rb_insert_color(&mem->mem_cgroup_node,
+			&mem_cgroup_soft_limit_tree);
+	mem->last_tree_update = jiffies;
+	spin_unlock_irqrestore(&memcg_soft_limit_tree_lock, flags);
+}
+
+static void mem_cgroup_remove_exceeded(struct mem_cgroup *mem)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&memcg_soft_limit_tree_lock, flags);
+	rb_erase(&mem->mem_cgroup_node, &mem_cgroup_soft_limit_tree);
+	spin_unlock_irqrestore(&memcg_soft_limit_tree_lock, flags);
+}
+
 static void mem_cgroup_charge_statistics(struct mem_cgroup *mem,
 					 struct page_cgroup *pc,
 					 bool charge)

@@ -897,6 +949,40 @@ static void record_last_oom(struct mem_cgroup *mem)
 	mem_cgroup_walk_tree(mem, NULL, record_last_oom_cb);
 }
 
+static void mem_cgroup_check_and_update_tree(struct mem_cgroup *mem,
+						bool time_check)
+{
+	unsigned long long prev_usage_in_excess, new_usage_in_excess;
+	bool updated_tree = false;
+	unsigned long next_update = 0;
+	unsigned long flags;
+
+	prev_usage_in_excess = mem->usage_in_excess;
+
+	if (time_check)
+		next_update = mem->last_tree_update +
+				MEM_CGROUP_TREE_UPDATE_INTERVAL;
+
+	if (!time_check || time_after(jiffies, next_update)) {
+		new_usage_in_excess = res_counter_soft_limit_excess(&mem->res);
+		if (prev_usage_in_excess) {
+			mem_cgroup_remove_exceeded(mem);
+			updated_tree = true;
+		}
+		if (!new_usage_in_excess)
+			goto done;
+		mem_cgroup_insert_exceeded(mem);
+		updated_tree = true;
+	}
+
+done:
+	if (updated_tree) {

kernel/res_counter.c static void record_last_oom(struct mem_cgroup *mem) · line 985

Quoted 6 lines

kernel/res_counter.c +6 -0

+		spin_lock_irqsave(&memcg_soft_limit_tree_lock, flags);
+		mem->last_tree_update = jiffies;
+		mem->usage_in_excess = new_usage_in_excess;
+		spin_unlock_irqrestore(&memcg_soft_limit_tree_lock, flags);
+	}
+}

Why update key parameter after inserting tree ? Is this bug ? Maybe RB-tree will be not sorted.

Feature: Implement reclaim from groups over their soft limit

From: Balbir Singh <[email protected]>

Changelog v5...v4

1. Throttling is removed, earlier we throttled tasks over their soft limit
2. Reclaim has been moved back to __alloc_pages_internal, several experiments and tests showed that it was the best place to reclaim memory. kswapd has a different goal, that does not work with a single soft limit for the memory cgroup.
3. Soft limit reclaim is more targetted and the pages reclaim depend on the amount by which the soft limit is exceeded.

Changelog v4...v3
1. soft_reclaim is now called from balance_pgdat
2. soft_reclaim is aware of nodes and zones
3. A mem_cgroup will be throttled if it is undergoing soft limit reclaim and at the same time trying to allocate pages and exceed its soft limit.
4. A new mem_cgroup_shrink_zone() routine has been added to shrink zones particular to a mem cgroup.

Changelog v3...v2
1. Convert several arguments to hierarchical reclaim to flags, thereby consolidating them
2. The reclaim for soft limits is now triggered from kswapd
3. try_to_free_mem_cgroup_pages() now accepts an optional zonelist argument

Changelog v2...v1
1. Added support for hierarchical soft limits

This patch allows reclaim from memory cgroups on contention (via the kswapd() path) only if the order is 0.

memory cgroup soft limit reclaim finds the group that exceeds its soft limit by the largest amount and reclaims pages from it and then reinserts the cgroup into its correct place in the rbtree.

...

static int mem_cgroup_hierarchical_reclaim(struct mem_cgroup *root_mem,

mm/memcontrol.c +0 -1

-				   gfp_t gfp_mask, bool noswap, bool shrink)

mm/memcontrol.c +24 -0

+		}
 		victim = mem_cgroup_select_victim(root_mem);
+		/*
+		 * In the first loop, don't reclaim from victims below
+		 * their soft limit
+		 */
 
...
 
+unsigned long mem_cgroup_soft_limit_reclaim(struct zonelist *zl, gfp_t gfp_mask)
+{
+	unsigned long nr_reclaimed = 0;
+	struct mem_cgroup *mem;
+	unsigned long flags;
+	unsigned long reclaimed;
+
+	/*
+	 * This loop can run a while, specially if mem_cgroup's continuously
+	 * keep exceeding their soft limit and putting the system under
+	 * pressure
+	 */
+	do {
+		mem = mem_cgroup_largest_soft_limit_node();
+		if (!mem)
+			break;
+
+		reclaimed = mem_cgroup_hierarchical_reclaim(mem, zl,
+						gfp_mask,

Quoted 29 lines · On Thu, 12 Mar 2009 23:26:31 +0530 Balbir Singh <[email protected]>…

Feature: Implement reclaim from groups over their soft limit

From: Balbir Singh <[email protected]>

Changelog v5...v4

1. Throttling is removed, earlier we throttled tasks over their soft limit
2. Reclaim has been moved back to __alloc_pages_internal, several experiments and tests showed that it was the best place to reclaim memory. kswapd has a different goal, that does not work with a single soft limit for the memory cgroup.
3. Soft limit reclaim is more targetted and the pages reclaim depend on the amount by which the soft limit is exceeded.

Changelog v4...v3
1. soft_reclaim is now called from balance_pgdat
2. soft_reclaim is aware of nodes and zones
3. A mem_cgroup will be throttled if it is undergoing soft limit reclaim and at the same time trying to allocate pages and exceed its soft limit.
4. A new mem_cgroup_shrink_zone() routine has been added to shrink zones particular to a mem cgroup.

Changelog v3...v2
1. Convert several arguments to hierarchical reclaim to flags, thereby consolidating them
2. The reclaim for soft limits is now triggered from kswapd
3. try_to_free_mem_cgroup_pages() now accepts an optional zonelist argument

Quoted 5 lines

Changelog v2...v1
1. Added support for hierarchical soft limits

This patch allows reclaim from memory cgroups on contention (via the kswapd() path) only if the order is 0.

Why for order-0 only?

What are the implications of not handling higher-order pages?

Why kswapd only?

What are the implications of omitting this from direct reclaim?

Quoted 3 lines

memory cgroup soft limit reclaim finds the group that exceeds its soft limit by the largest amount and reclaims pages from it and then reinserts the cgroup into its correct place in the rbtree.

Why trim the single worst-case group rather than (say) trimming all groups by a common proportion? Or other things.

When you say "by the largest amount", is that the "by largest number of pages" or "by the largest percentage"?

What are the implications of <whichever you chose>?

Quoted 12 lines

...

static int mem_cgroup_hierarchical_reclaim(struct mem_cgroup *root_mem,

mm/memcontrol.c +3 -1

-				   gfp_t gfp_mask, bool noswap, bool shrink)
+						struct zonelist *zl,
+						gfp_t gfp_mask,
+						unsigned long flags)
 {
 	struct mem_cgroup *victim;
 	int ret, total = 0;
 	int loop = 0;

mm/memcontrol.c mem_cgroup_select_victim(struct mem_cgroup *root_mem) · line 975

Quoted 3 lines

mm/memcontrol.c +3 -0

+	bool noswap = flags & MEM_CGROUP_RECLAIM_NOSWAP;
+	bool shrink = flags & MEM_CGROUP_RECLAIM_SHRINK;
+	bool check_soft = flags & MEM_CGROUP_RECLAIM_SOFT;

`flags' wasn't a great choice of identifier. It's a bit misleading, and you'll be in a pickle if you later try to add a spin_lock_irqsave(lock, flags) to this function. Maybe choose a more specific name?

mm/memcontrol.c mem_cgroup_select_victim(struct mem_cgroup *root_mem) · line 986

Quoted 13 lines

mm/memcontrol.c +10 -1

+	unsigned long excess = mem_cgroup_get_excess(root_mem);
 
-	while (loop < 2) {
+	while (1) {
+		if (loop >= 2) {
+			/*
+			 * With soft limits, do more targetted reclaim
+			 */
+			if (check_soft && (total >= (excess >> 4)))
+				break;
+			else if (!check_soft)
+				break;

maybe..

			if (!check_soft)
				break;
			if (total >= (excess >> 4))
				break;

dunno.

The ">> 4" magic number would benefit from an explanatory comment. Why not ">> 3"???

Quoted 29 lines

mm/memcontrol.c +24 -0

+		}
 		victim = mem_cgroup_select_victim(root_mem);
+		/*
+		 * In the first loop, don't reclaim from victims below
+		 * their soft limit
+		 */
 
...
 
+unsigned long mem_cgroup_soft_limit_reclaim(struct zonelist *zl, gfp_t gfp_mask)
+{
+	unsigned long nr_reclaimed = 0;
+	struct mem_cgroup *mem;
+	unsigned long flags;
+	unsigned long reclaimed;
+
+	/*
+	 * This loop can run a while, specially if mem_cgroup's continuously
+	 * keep exceeding their soft limit and putting the system under
+	 * pressure
+	 */
+	do {
+		mem = mem_cgroup_largest_soft_limit_node();
+		if (!mem)
+			break;
+
+		reclaimed = mem_cgroup_hierarchical_reclaim(mem, zl,
+						gfp_mask,

mm/memcontrol.c int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg, · line 1994

Quoted 10 lines

mm/memcontrol.c +10 -0

+						MEM_CGROUP_RECLAIM_SOFT);
+		nr_reclaimed += reclaimed;
+		spin_lock_irqsave(&memcg_soft_limit_tree_lock, flags);
+		mem->usage_in_excess = res_counter_soft_limit_excess(&mem->res);
+		__mem_cgroup_remove_exceeded(mem);
+		if (mem->usage_in_excess)
+			__mem_cgroup_insert_exceeded(mem);
+		spin_unlock_irqrestore(&memcg_soft_limit_tree_lock, flags);
+		css_put(&mem->css);
+		cond_resched();

spin_lock_irq() would suffice here. Or the cond_resched() is a bug.

There's a decent argument that spin_lock_irq() is dangerous, and its saving is so small that it's better to use the more robust spin_lock_irqsave() all the time. But was that the intent here?

Quoted 4 lines

mm/memcontrol.c +3 -0

+	} while (!nr_reclaimed);
+	return nr_reclaimed;
+}

mm/page_alloc.c +0 -0

index 46bd24c..b49c90f 100644
@@ -1583,7 +1583,14 @@ nofail_alloc:
 	reclaim_state.reclaimed_slab = 0;
 	p->reclaim_state = &reclaim_state;

Hi

Quoted 7 lines

mm/page_alloc.c +0 -0

index 46bd24c..b49c90f 100644
@@ -1583,7 +1583,14 @@ nofail_alloc:
 	reclaim_state.reclaimed_slab = 0;
 	p->reclaim_state = &reclaim_state;

mm/page_alloc.c nofail_alloc: · line 1593

Quoted 10 lines

mm/page_alloc.c +8 -1

 
-	did_some_progress = try_to_free_pages(zonelist, order, gfp_mask);
+	/*
+	 * Try to free up some pages from the memory controllers soft
+	 * limit queue.
+	 */
+	did_some_progress = mem_cgroup_soft_limit_reclaim(zonelist, gfp_mask);
+	if (!order || !did_some_progress)
+		did_some_progress += try_to_free_pages(zonelist, order,
+							gfp_mask);

I have two objection to this.

- "if (!order || !did_some_progress)" mean no call try_to_free_pages() in order>0 and did_some_progress>0 case.
but mem_cgroup_soft_limit_reclaim() don't have lumpy reclaim. then, it break high order reclaim.

- in global reclaim view, foreground reclaim and background reclaim's reclaim rate is about 1:9 typically.
then, kswapd reclaim the pages by global lru order before proceccing this logic.
IOW, this soft limit is not SOFT.

Quoted 12 lines

mm/page_alloc.c nofail_alloc: · line 1593

Quoted 9 lines

mm/page_alloc.c +8 -1

-	did_some_progress = try_to_free_pages(zonelist, order, gfp_mask);
+	/*
+	 * Try to free up some pages from the memory controllers soft
+	 * limit queue.
+	 */
+	did_some_progress = mem_cgroup_soft_limit_reclaim(zonelist, gfp_mask);
+	if (!order || !did_some_progress)
+		did_some_progress += try_to_free_pages(zonelist, order,
+							gfp_mask);

I have two objection to this.

Quoted 4 lines

- "if (!order || !did_some_progress)" mean no call try_to_free_pages() in order>0 and did_some_progress>0 case.
but mem_cgroup_soft_limit_reclaim() don't have lumpy reclaim. then, it break high order reclaim.

I am sending a fix for this right away. Thanks, the check should be if (order || !did_some_progress)

Quoted 5 lines

- in global reclaim view, foreground reclaim and background reclaim's reclaim rate is about 1:9 typically.
then, kswapd reclaim the pages by global lru order before proceccing this logic.
IOW, this soft limit is not SOFT.

It depends on what you mean by soft. I call them soft since they are imposed only when there is contention. If you mean kswapd runs more often than direct reclaim, that is true, but it does not impact this code extensively since the high water mark is a very small compared to the pages present on the system.

Quoted 9 lines

Quoted 6 lines

I have two objection to this.

- "if (!order || !did_some_progress)" mean no call try_to_free_pages() in order>0 and did_some_progress>0 case.
but mem_cgroup_soft_limit_reclaim() don't have lumpy reclaim. then, it break high order reclaim.

I am sending a fix for this right away. Thanks, the check should be if (order || !did_some_progress)

No.

it isn't enough.
after is does, order-1 allocation case twrice reclaim (soft limit shrinking and normal try_to_free_pages()).
then, order-1 reclaim makes slower about 2 times.

unfortunately, order-1 allocation is very frequent. it is used for kernel stack.

Quoted 11 lines

Quoted 5 lines

- in global reclaim view, foreground reclaim and background reclaim's reclaim rate is about 1:9 typically.
then, kswapd reclaim the pages by global lru order before proceccing this logic.
IOW, this soft limit is not SOFT.

It depends on what you mean by soft. I call them soft since they are imposed only when there is contention. If you mean kswapd runs more often than direct reclaim, that is true, but it does not impact this code extensively since the high water mark is a very small compared to the pages present on the system.

No.

My point is, contention case kswapd wakeup. and kswapd reclaim by global lru order before soft limit shrinking.
Therefore, In typical usage, mem_cgroup_soft_limit_reclaim() almost don't call properly.

soft limit shrinking should run before processing global reclaim.

Quoted 12 lines

Quoted 9 lines

Quoted 6 lines

I have two objection to this.

- "if (!order || !did_some_progress)" mean no call try_to_free_pages() in order>0 and did_some_progress>0 case.
but mem_cgroup_soft_limit_reclaim() don't have lumpy reclaim. then, it break high order reclaim.

I am sending a fix for this right away. Thanks, the check should be if (order || !did_some_progress)

No.

Quoted 7 lines

it isn't enough.
after is does, order-1 allocation case twrice reclaim (soft limit shrinking and normal try_to_free_pages()).
then, order-1 reclaim makes slower about 2 times.

unfortunately, order-1 allocation is very frequent. it is used for kernel stack.

in normal order-1 reclaim is:

1. try_to_free_pages()
2. get_page_from_freelist()
3. retry if order-1 page don't exist

Coundn't you use the same logic?

Quoted 14 lines

Quoted 11 lines

Quoted 5 lines

- in global reclaim view, foreground reclaim and background reclaim's reclaim rate is about 1:9 typically.
then, kswapd reclaim the pages by global lru order before proceccing this logic.
IOW, this soft limit is not SOFT.

It depends on what you mean by soft. I call them soft since they are imposed only when there is contention. If you mean kswapd runs more often than direct reclaim, that is true, but it does not impact this code extensively since the high water mark is a very small compared to the pages present on the system.

No.

Quoted 6 lines

My point is, contention case kswapd wakeup. and kswapd reclaim by global lru order before soft limit shrinking.
Therefore, In typical usage, mem_cgroup_soft_limit_reclaim() almost don't call properly.

soft limit shrinking should run before processing global reclaim.

Do you have the reason of disliking call from kswapd ?

Quoted 9 lines

Quoted 6 lines

My point is, contention case kswapd wakeup. and kswapd reclaim by global lru order before soft limit shrinking.
Therefore, In typical usage, mem_cgroup_soft_limit_reclaim() almost don't call properly.

soft limit shrinking should run before processing global reclaim.

Do you have the reason of disliking call from kswapd ?

Yes, I sent that reason out as comments to Kame's patches. kswapd or balance_pgdat controls the zones, priority and in effect how many pages we scan while doing reclaim. I did lots of experiments and found that if soft limit reclaim occurred from kswapd, soft_limit_reclaim would almost always fail and shrink_zone() would succeed, since it looks at the whole zone and is always able to find some pages at all priority levels. It also does not allow for targetted reclaim based on how much we exceed the soft limit by.

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

My point is, contention case kswapd wakeup. and kswapd reclaim by global lru order before soft limit shrinking.
Therefore, In typical usage, mem_cgroup_soft_limit_reclaim() almost don't call properly.

soft limit shrinking should run before processing global reclaim.

Do you have the reason of disliking call from kswapd ?

Quoted 8 lines

Yes, I sent that reason out as comments to Kame's patches. kswapd or balance_pgdat controls the zones, priority and in effect how many pages we scan while doing reclaim. I did lots of experiments and found that if soft limit reclaim occurred from kswapd, soft_limit_reclaim would almost always fail and shrink_zone() would succeed, since it looks at the whole zone and is always able to find some pages at all priority levels. It also does not allow for targetted reclaim based on how much we exceed the soft limit by.

hm
I read past discussion. so, I think we discuss many aspect at once. So, my current thinking is below,

(1) if the group don't have any soft limit shrinking page, mem_cgroup_soft_limit_reclaim() spent time unnecessary. -> right.

actually, past global reclaim had similar problem. then zone_is_all_unreclaimable() was introduced. maybe we can use similar technique to memcg.

(2) mem_cgroup_soft_limit_reclaim() should be called from? -> under discussion.
we should solve (1) at first for proper constructive discussion.

(3) What's "fairness" of soft limit?

    -> perfectly another aspect.

So, I'd like to discuss (1) at first. Although we don't kswapd shrinking, (1) is problem.

Quoted 7 lines

hm
I read past discussion. so, I think we discuss many aspect at once. So, my current thinking is below,

(1) if the group don't have any soft limit shrinking page, mem_cgroup_soft_limit_reclaim() spent time unnecessary. -> right.

If the soft limit RB tree is empty, we don't spend any time at all. Are you referring to something else? Am I missing something? The tree will be empty if no group is over the soft limit.

I experimented a *lot* with zone reclaim and found it to be not so effective. Here is why

1. We have no control over priority or how much to scan, that is controlled by balance_pgdat(). If we find that we are unable to scan anything, we continue scanning with the scan > 0 check, but we scan the same pages and the same number, because shrink_zone does scan >> priority.

Yes, I sent that reason out as comments to Kame's patches. kswapd or balance_pgdat controls the zones, priority and in effect how many pages we scan while doing reclaim. I did lots of experiments and found that if soft limit reclaim occurred from kswapd, soft_limit_reclaim would almost always fail and shrink_zone() would succeed, since it looks at the whole zone and is always able to find some pages at all priority levels. It also does not allow for targetted reclaim based on how much we exceed the soft limit by.

Quoted 11 lines

Quoted 7 lines

hm
I read past discussion. so, I think we discuss many aspect at once. So, my current thinking is below,

(1) if the group don't have any soft limit shrinking page, mem_cgroup_soft_limit_reclaim() spent time unnecessary. -> right.

If the soft limit RB tree is empty, we don't spend any time at all. Are you referring to something else? Am I missing something? The tree will be empty if no group is over the soft limit.

maybe, I am missing anything.
May I ask your following paragraph meaning?

Quoted 3 lines

I experimented a *lot* with zone reclaim and found it to be not so effective. Here is why

Quoted 5 lines

1. We have no control over priority or how much to scan, that is controlled by balance_pgdat(). If we find that we are unable to scan anything, we continue scanning with the scan > 0 check, but we scan the same pages and the same number, because shrink_zone does scan >> priority.

I thought this sentense mean soft-limit-shrinking spent a lot of time. if not, could you please tell me what makes so slower?

Quoted 9 lines · you

Yes, I sent that reason out as comments to Kame's patches. kswapd or balance_pgdat controls the zones, priority and in effect how many pages we scan while doing reclaim. I did lots of experiments and found that if soft limit reclaim occurred from kswapd, soft_limit_reclaim would almost always fail and shrink_zone() would succeed, since it looks at the whole zone and is always able to find some pages at all priority levels. It also does not allow for targetted reclaim based on how much we exceed the soft limit by.

but, if "soft_limit_reclaim fail and shrink_zone() succeed" don't cause any performance degression, I don't find why kswapd is wrong.

I guess you and kamezawa-san know it detail. but my understanding don't reach it. Could you please tell me what so slowness.

* KOSAKI Motohiro <[email protected]> [2009-03-13 16:17:25]:

1. Kame's scan logic, selects shrink_zone for the mem cgroup, but the pages scanned and reclaimed from depend on priority and watermarks of the zone and *not* at all on the soft limit parameters.

2. Because soft limit reclaim fails to reclaim anythoing (due to 1), shrink_zone which is called, does reclaiming indepedent of any knowledge of soft limits, which does not work as expected.

--
Balbir

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 12 lines

Quoted 9 lines

Quoted 6 lines

I have two objection to this.

- "if (!order || !did_some_progress)" mean no call try_to_free_pages() in order>0 and did_some_progress>0 case.
but mem_cgroup_soft_limit_reclaim() don't have lumpy reclaim. then, it break high order reclaim.

I am sending a fix for this right away. Thanks, the check should be if (order || !did_some_progress)

No.

Quoted 7 lines

it isn't enough.
after is does, order-1 allocation case twrice reclaim (soft limit shrinking and normal try_to_free_pages()).
then, order-1 reclaim makes slower about 2 times.

unfortunately, order-1 allocation is very frequent. it is used for kernel stack.

in normal order-1 reclaim is:

1. try_to_free_pages()
2. get_page_from_freelist()
3. retry if order-1 page don't exist

Coundn't you use the same logic?

Quoted 27 lines

Quoted 13 lines

Quoted 10 lines

Quoted 7 lines

I have two objection to this.

- "if (!order || !did_some_progress)" mean no call try_to_free_pages() in order>0 and did_some_progress>0 case.
but mem_cgroup_soft_limit_reclaim() don't have lumpy reclaim. then, it break high order reclaim.

I am sending a fix for this right away. Thanks, the check should be if (order || !did_some_progress)

No.

Quoted 7 lines

it isn't enough.
after is does, order-1 allocation case twrice reclaim (soft limit shrinking and normal try_to_free_pages()).
then, order-1 reclaim makes slower about 2 times.

unfortunately, order-1 allocation is very frequent. it is used for kernel stack.

in normal order-1 reclaim is:

1. try_to_free_pages()
2. get_page_from_freelist()
3. retry if order-1 page don't exist

Quoted 1 line

Coundn't you use the same logic?

Sorry, forgot to answer this question earlier.

I assume that by order-1 you mean 2 pages (2^1).

Are you suggesting that if soft limit reclaim fails, we retry? Not sure if I understand your suggestion completely.

Quoted 12 lines

Quoted 12 lines

mm/page_alloc.c nofail_alloc: · line 1593

Quoted 9 lines

mm/page_alloc.c +8 -1

-	did_some_progress = try_to_free_pages(zonelist, order, gfp_mask);
+	/*
+	 * Try to free up some pages from the memory controllers soft
+	 * limit queue.
+	 */
+	did_some_progress = mem_cgroup_soft_limit_reclaim(zonelist, gfp_mask);
+	if (!order || !did_some_progress)
+		did_some_progress += try_to_free_pages(zonelist, order,
+							gfp_mask);

I have two objection to this.

Quoted 7 lines

Quoted 4 lines

- "if (!order || !did_some_progress)" mean no call try_to_free_pages() in order>0 and did_some_progress>0 case.
but mem_cgroup_soft_limit_reclaim() don't have lumpy reclaim. then, it break high order reclaim.

I am sending a fix for this right away. Thanks, the check should be if (order || !did_some_progress)

No.

it isn't enough.
after is does, order-1 allocation case twrice reclaim (soft limit shrinking and normal try_to_free_pages()).
then, order-1 reclaim makes slower about 2 times.

unfortunately, order-1 allocation is very frequent. it is used for kernel stack.

Quoted 11 lines

Quoted 5 lines

- in global reclaim view, foreground reclaim and background reclaim's reclaim rate is about 1:9 typically.
then, kswapd reclaim the pages by global lru order before proceccing this logic.
IOW, this soft limit is not SOFT.

It depends on what you mean by soft. I call them soft since they are imposed only when there is contention. If you mean kswapd runs more often than direct reclaim, that is true, but it does not impact this code extensively since the high water mark is a very small compared to the pages present on the system.

No.

My point is, contention case kswapd wakeup. and kswapd reclaim by global lru order before soft limit shrinking.

Feature: Implement reclaim from groups over their soft limit

From: Balbir Singh <[email protected]>

+unsigned long mem_cgroup_soft_limit_reclaim(struct zonelist *zl, gfp_t gfp_mask)

mm/memcontrol.c +22 -0

+{
+	unsigned long nr_reclaimed = 0;
+	struct mem_cgroup *mem;
+	unsigned long flags;
+	unsigned long reclaimed;
+
+	/*
+	 * This loop can run a while, specially if mem_cgroup's continuously
+	 * keep exceeding their soft limit and putting the system under
+	 * pressure
+	 */
+	do {
+		mem = mem_cgroup_largest_soft_limit_node();
+		if (!mem)
+			break;
+
+		reclaimed = mem_cgroup_hierarchical_reclaim(mem, zl,
+						gfp_mask,
+						MEM_CGROUP_RECLAIM_SOFT);
+		nr_reclaimed += reclaimed;
+		spin_lock_irqsave(&memcg_soft_limit_tree_lock, flags);
+		mem->usage_in_excess = res_counter_soft_limit_excess(&mem->res);

Quoted 28 lines · Balbir Singh

Feature: Implement reclaim from groups over their soft limit

From: Balbir Singh <[email protected]>

+unsigned long mem_cgroup_soft_limit_reclaim(struct zonelist *zl, gfp_t gfp_mask)

mm/memcontrol.c +22 -0

+{
+	unsigned long nr_reclaimed = 0;
+	struct mem_cgroup *mem;
+	unsigned long flags;
+	unsigned long reclaimed;
+
+	/*
+	 * This loop can run a while, specially if mem_cgroup's continuously
+	 * keep exceeding their soft limit and putting the system under
+	 * pressure
+	 */
+	do {
+		mem = mem_cgroup_largest_soft_limit_node();
+		if (!mem)
+			break;
+
+		reclaimed = mem_cgroup_hierarchical_reclaim(mem, zl,
+						gfp_mask,
+						MEM_CGROUP_RECLAIM_SOFT);
+		nr_reclaimed += reclaimed;
+		spin_lock_irqsave(&memcg_soft_limit_tree_lock, flags);
+		mem->usage_in_excess = res_counter_soft_limit_excess(&mem->res);

mm/memcontrol.c int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg, · line 1998

Quoted 10 lines

mm/memcontrol.c +10 -0

+		__mem_cgroup_remove_exceeded(mem);
+		if (mem->usage_in_excess)
+			__mem_cgroup_insert_exceeded(mem);
+		spin_unlock_irqrestore(&memcg_soft_limit_tree_lock, flags);
+		css_put(&mem->css);
+		cond_resched();
+	} while (!nr_reclaimed);
+	return nr_reclaimed;
+}
+

Why do you never consider bad corner case....
As I wrote many times, "order of global usage" doesn't mean the biggest user of memcg containes memory in zones which we want. So, please don't pust "mem" back to RB-tree if reclaimed is 0.

This routine seems toooo bad as v4.

* KAMEZAWA Hiroyuki <[email protected]> [2009-03-13 15:51:25]:

Quoted 45 lines

Quoted 5 lines · Balbir Singh

Feature: Implement reclaim from groups over their soft limit

From: Balbir Singh <[email protected]>

+unsigned long mem_cgroup_soft_limit_reclaim(struct zonelist *zl,

gfp_t

Quoted 23 lines

gfp_mask)

mm/memcontrol.c +22 -0

+{
+	unsigned long nr_reclaimed = 0;
+	struct mem_cgroup *mem;
+	unsigned long flags;
+	unsigned long reclaimed;
+
+	/*
+	 * This loop can run a while, specially if mem_cgroup's continuously
+	 * keep exceeding their soft limit and putting the system under
+	 * pressure
+	 */
+	do {
+		mem = mem_cgroup_largest_soft_limit_node();
+		if (!mem)
+			break;
+
+		reclaimed = mem_cgroup_hierarchical_reclaim(mem, zl,
+						gfp_mask,
+						MEM_CGROUP_RECLAIM_SOFT);
+		nr_reclaimed += reclaimed;
+		spin_lock_irqsave(&memcg_soft_limit_tree_lock, flags);
+		mem->usage_in_excess = res_counter_soft_limit_excess(&mem->res);

mm/memcontrol.c int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg, · line 1998

Quoted 10 lines

mm/memcontrol.c +10 -0

+		__mem_cgroup_remove_exceeded(mem);
+		if (mem->usage_in_excess)
+			__mem_cgroup_insert_exceeded(mem);
+		spin_unlock_irqrestore(&memcg_soft_limit_tree_lock, flags);
+		css_put(&mem->css);
+		cond_resched();
+	} while (!nr_reclaimed);
+	return nr_reclaimed;
+}
+

Why do you never consider bad corner case....
As I wrote many times, "order of global usage" doesn't mean the biggest user of memcg containes memory in zones which we want. So, please don't pust "mem" back to RB-tree if reclaimed is 0.

Quoted 1 line

This routine seems toooo bad as v4.

Are you talking about cases where a particular mem cgroup never allocated from a node? Thanks.. let me take a look at it

From: Balbir Singh <[email protected]>

New Feature: Soft limits for memory resource controller.

Changelog v5...v4
1. Several changes to the reclaim logic, please see the patch 4 (reclaim on
contention). I've experimented with several possibilities for reclaim and chose to come back to this due to the excellent behaviour seen
while
testing the patchset.
2. Reduced the overhead of soft limits on resource counters very significantly.
Reaim benchmark now shows almost no drop in performance.

Balbir Singh さんは書きました：

Quoted 5 lines

From: Balbir Singh <[email protected]>

New Feature: Soft limits for memory resource controller.

Quoted 11 lines

Changelog v5...v4
1. Several changes to the reclaim logic, please see the patch 4 (reclaim on
contention). I've experimented with several possibilities for reclaim and chose to come back to this due to the excellent behaviour seen
while
testing the patchset.
2. Reduced the overhead of soft limits on resource counters very significantly.
Reaim benchmark now shows almost no drop in performance.

It seems there are no changes to answer my last comments.

Nack again. I'll update my own version again.

KAMEZAWA Hiroyuki ?$B$5$s$O=q$-$^$7$?!'

Quoted 17 lines

Balbir Singh ?$B$5$s$O=q$-$^$7$?!'

Quoted 16 lines

From: Balbir Singh <[email protected]>

New Feature: Soft limits for memory resource controller.

Changelog v5...v4
1. Several changes to the reclaim logic, please see the patch 4 (reclaim on
contention). I've experimented with several possibilities for reclaim and chose to come back to this due to the excellent behaviour seen
while
testing the patchset.
2. Reduced the overhead of soft limits on resource counters very significantly.
Reaim benchmark now shows almost no drop in performance.

Quoted 4 lines

It seems there are no changes to answer my last comments.

Nack again. I'll update my own version again.

Sigh, this is in -mm ? okay...I'll update onto -mm as much as I can. Very heavy work, maybe.

Quoted 2 lines

Nack again. I'll update my own version again.

Sigh, this is in -mm ? okay...I'll update onto -mm as much as I can. Very heavy work, maybe.

Quoted 4 lines · On Fri, 13 Mar 2009 16:07:35 +0900 (JST) "KAMEZAWA Hiroyuki"…

Quoted 2 lines

Nack again. I'll update my own version again.

Sigh, this is in -mm ? okay...I'll update onto -mm as much as I can. Very heavy work, maybe.

I dropped them all again. it appears that quite a few changes are needed and I don't think we want these patches interfering with other cgroup and general MM development.