Memory controller soft limit patches (v3)

From: Balbir Singh <[email protected]>

Changelog v2...v1
1. Add support for res_counter_check_soft_limit_locked. This is used by the hierarchy code.

Add an interface to allow get/set of soft limits. Soft limits for memory plus swap controller (memsw) is currently not supported. Resource counters have been enhanced to support soft limits and new type RES_SOFT_LIMIT has been added. Unlike hard limits, soft limits can be directly set and do not need any reclaim or checks before setting them to a newer value.

Kamezawa-San raised a question as to whether soft limit should belong to res_counter. Since all resources understand the basic concepts of hard and soft limits, it is justified to add soft limits here. Soft limits are a generic resource usage feature, even file system quotas support soft limits.

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

---

 include/linux/res_counter.h |   47 +++++++++++++++++++++++++++++++++++++++++++
 kernel/res_counter.c        |    3 +++
 mm/memcontrol.c             |   20 ++++++++++++++++++
 3 files changed, 70 insertions(+), 0 deletions(-)

include/linux/res_counter.h +47 -0

index 4c5bcf6..b5f14fa 100644
@@ -35,6 +35,10 @@ struct res_counter {
 	 */
 	unsigned long long limit;
 	/*
+	 * the limit that usage can be exceed
+	 */
+	unsigned long long soft_limit;
+	/*
 	 * the number of unsuccessful attempts to consume the resource
 	 */
 	unsigned long long failcnt;

@@ -85,6 +89,7 @@ enum {
 	RES_MAX_USAGE,
 	RES_LIMIT,
 	RES_FAILCNT,
+	RES_SOFT_LIMIT,
 };
 
 /*

@@ -130,6 +135,36 @@ static inline bool res_counter_limit_check_locked(struct res_counter *cnt)
 	return false;
 }
 
+static inline bool res_counter_soft_limit_check_locked(struct res_counter *cnt)
+{
+	if (cnt->usage < cnt->soft_limit)
+		return true;
+
+	return false;
+}
+
+/**
+ * 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;
+}
+
 /*
  * Helper function to detect if the cgroup is within it's limit or
  * not. It's currently called from cgroup_rss_prepare()

@@ -178,4 +213,16 @@ static inline int res_counter_set_limit(struct res_counter *cnt,
 	return ret;
 }
 
+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;
+}
+
 #endif

kernel/res_counter.c +3 -0

index bf8e753..4e6dafe 100644
@@ -19,6 +19,7 @@ void res_counter_init(struct res_counter *counter, struct res_counter *parent)
 {
 	spin_lock_init(&counter->lock);
 	counter->limit = (unsigned long long)LLONG_MAX;
+	counter->soft_limit = (unsigned long long)LLONG_MAX;
 	counter->parent = parent;
 }
 

@@ -101,6 +102,8 @@ res_counter_member(struct res_counter *counter, int member)
 		return &counter->limit;
 	case RES_FAILCNT:
 		return &counter->failcnt;
+	case RES_SOFT_LIMIT:
+		return &counter->soft_limit;
 	};
 
 	BUG();

mm/memcontrol.c +20 -0

index 7bb14fd..75a7b1a 100644
@@ -1939,6 +1939,20 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
 		else
 			ret = mem_cgroup_resize_memsw_limit(memcg, val);
 		break;
+	case RES_SOFT_LIMIT:
+		ret = res_counter_memparse_write_strategy(buffer, &val);
+		if (ret)
+			break;
+		/*
+		 * For memsw, soft limits are hard to implement in terms
+		 * of semantics, for now, we support soft limits for
+		 * control without swap
+		 */
+		if (type == _MEM)
+			ret = res_counter_set_soft_limit(&memcg->res, val);
+		else
+			ret = -EINVAL;
+		break;
 	default:
 		ret = -EINVAL; /* should be BUG() ? */
 		break;

@@ -2188,6 +2202,12 @@ static struct cftype mem_cgroup_files[] = {
 		.read_u64 = mem_cgroup_read,
 	},
 	{
+		.name = "soft_limit_in_bytes",
+		.private = MEMFILE_PRIVATE(_MEM, RES_SOFT_LIMIT),
+		.write_string = mem_cgroup_write,
+		.read_u64 = mem_cgroup_read,
+	},
+	{
 		.name = "failcnt",
 		.private = MEMFILE_PRIVATE(_MEM, RES_FAILCNT),
 		.trigger = mem_cgroup_reset,

Signature · 7 lines

-- 
	Balbir
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 13 lines · On Sun, 01 Mar 2009 12:00:11 +0530 Balbir Singh <[email protected]>…

From: Balbir Singh <[email protected]>

Changelog v2...v1
1. Add support for res_counter_check_soft_limit_locked. This is used by the hierarchy code.

Add an interface to allow get/set of soft limits. Soft limits for memory plus swap controller (memsw) is currently not supported. Resource counters have been enhanced to support soft limits and new type RES_SOFT_LIMIT has been added. Unlike hard limits, soft limits can be directly set and do not need any reclaim or checks before setting them to a newer value.

Quoted 6 lines

Kamezawa-San raised a question as to whether soft limit should belong to res_counter. Since all resources understand the basic concepts of hard and soft limits, it is justified to add soft limits here. Soft limits are a generic resource usage feature, even file system quotas support soft limits.

I don't convice adding more logics to res_counter is a good to do, yet.

Quoted 22 lines · KAMEZAWA Hiroyuki

Quoted 19 lines · On Sun, 01 Mar 2009 12:00:11 +0530 Balbir Singh <[email protected]>…

From: Balbir Singh <[email protected]>

Changelog v2...v1
1. Add support for res_counter_check_soft_limit_locked. This is used by the hierarchy code.

Add an interface to allow get/set of soft limits. Soft limits for memory plus swap controller (memsw) is currently not supported. Resource counters have been enhanced to support soft limits and new type RES_SOFT_LIMIT has been added. Unlike hard limits, soft limits can be directly set and do not need any reclaim or checks before setting them to a newer value.

Kamezawa-San raised a question as to whether soft limit should belong to res_counter. Since all resources understand the basic concepts of hard and soft limits, it is justified to add soft limits here. Soft limits are a generic resource usage feature, even file system quotas support soft limits.

Quoted 2 lines

I don't convice adding more logics to res_counter is a good to do, yet.

Even though it is extensible and you pay the cost only when soft limits is turned on? Can you show me why you are not convinced?

Quoted 24 lines · On Mon, 2 Mar 2009 10:16:31 +0530 Balbir Singh <[email protected]>…

Quoted 22 lines · KAMEZAWA Hiroyuki

Quoted 19 lines · On Sun, 01 Mar 2009 12:00:11 +0530 Balbir Singh <[email protected]>…

From: Balbir Singh <[email protected]>

Changelog v2...v1
1. Add support for res_counter_check_soft_limit_locked. This is used by the hierarchy code.

Add an interface to allow get/set of soft limits. Soft limits for memory plus swap controller (memsw) is currently not supported. Resource counters have been enhanced to support soft limits and new type RES_SOFT_LIMIT has been added. Unlike hard limits, soft limits can be directly set and do not need any reclaim or checks before setting them to a newer value.

Kamezawa-San raised a question as to whether soft limit should belong to res_counter. Since all resources understand the basic concepts of hard and soft limits, it is justified to add soft limits here. Soft limits are a generic resource usage feature, even file system quotas support soft limits.

Quoted 6 lines

Quoted 2 lines

I don't convice adding more logics to res_counter is a good to do, yet.

Even though it is extensible and you pay the cost only when soft limits is turned on? Can you show me why you are not convinced?

Inserting more codes (like "if") to res_counter itself is not welcome.. I think res_counter is too complex as counter already.

I'm now searching a way to reduce res_counter->lock ping-pong but have no good idea.

Quoted 30 lines · KAMEZAWA Hiroyuki

Quoted 27 lines · On Mon, 2 Mar 2009 10:16:31 +0530 Balbir Singh <[email protected]>…

Quoted 24 lines · KAMEZAWA Hiroyuki

Quoted 13 lines · On Sun, 01 Mar 2009 12:00:11 +0530 Balbir Singh <[email protected]>…

From: Balbir Singh <[email protected]>

Changelog v2...v1
1. Add support for res_counter_check_soft_limit_locked. This is used by the hierarchy code.

Add an interface to allow get/set of soft limits. Soft limits for memory plus swap controller (memsw) is currently not supported. Resource counters have been enhanced to support soft limits and new type RES_SOFT_LIMIT has been added. Unlike hard limits, soft limits can be directly set and do not need any reclaim or checks before setting them to a newer value.

Quoted 6 lines

Kamezawa-San raised a question as to whether soft limit should belong to res_counter. Since all resources understand the basic concepts of hard and soft limits, it is justified to add soft limits here. Soft limits are a generic resource usage feature, even file system quotas support soft limits.

I don't convice adding more logics to res_counter is a good to do, yet.

Quoted 6 lines

Quoted 3 lines

Even though it is extensible and you pay the cost only when soft limits is turned on? Can you show me why you are not convinced?

Inserting more codes (like "if") to res_counter itself is not welcome.. I think res_counter is too complex as counter already.

Darn.. we better stop all code development!

Quoted 35 lines · On Mon, 2 Mar 2009 11:37:26 +0530 Balbir Singh <[email protected]>…

Quoted 33 lines · KAMEZAWA Hiroyuki

Quoted 30 lines · On Mon, 2 Mar 2009 10:16:31 +0530 Balbir Singh <[email protected]>…

Quoted 16 lines · KAMEZAWA Hiroyuki

Quoted 13 lines · On Sun, 01 Mar 2009 12:00:11 +0530 Balbir Singh <[email protected]>…

From: Balbir Singh <[email protected]>

Changelog v2...v1
1. Add support for res_counter_check_soft_limit_locked. This is used by the hierarchy code.

Add an interface to allow get/set of soft limits. Soft limits for memory plus swap controller (memsw) is currently not supported. Resource counters have been enhanced to support soft limits and new type RES_SOFT_LIMIT has been added. Unlike hard limits, soft limits can be directly set and do not need any reclaim or checks before setting them to a newer value.

Quoted 8 lines

Quoted 6 lines

Kamezawa-San raised a question as to whether soft limit should belong to res_counter. Since all resources understand the basic concepts of hard and soft limits, it is justified to add soft limits here. Soft limits are a generic resource usage feature, even file system quotas support soft limits.

I don't convice adding more logics to res_counter is a good to do, yet.

Even though it is extensible and you pay the cost only when soft limits is turned on? Can you show me why you are not convinced?

Quoted 6 lines

Quoted 3 lines

Inserting more codes (like "if") to res_counter itself is not welcome.. I think res_counter is too complex as counter already.

Darn.. we better stop all code development!

I don't say such a thing. My point is we have to keep res_counter as light-weight as possible. If there are alternatives, we should use that.

Hi Balbir,

mm/vmscan.c static int kswapd(void *p) · line 2027

Quoted 13 lines

mm/vmscan.c +3 -0

@@ -2015,9 +2016,12 @@ static int kswapd(void *p)
 		finish_wait(&pgdat->kswapd_wait, &wait);
 
 		if (!try_to_freeze()) {
+			struct zonelist *zl = pgdat->node_zonelists;
 			/* We can speed up thawing tasks if we don't call
 			 * balance_pgdat after returning from the refrigerator
 			 */
+			if (!order)
+				mem_cgroup_soft_limit_reclaim(zl, GFP_KERNEL);
 			balance_pgdat(pgdat, order);
 		}
 	}

kswapd's roll is increasing free pages until zone->pages_high in "own node". mem_cgroup_soft_limit_reclaim() free one (or more) exceed page in any node.

Oh, well.
I think it is not consistency.

if mem_cgroup_soft_limit_reclaim() is aware to target node and its pages_high, I'm glad.

Quoted 20 lines

Hi Balbir,

mm/vmscan.c static int kswapd(void *p) · line 2027

Quoted 13 lines

mm/vmscan.c +3 -0

@@ -2015,9 +2016,12 @@ static int kswapd(void *p)
 		finish_wait(&pgdat->kswapd_wait, &wait);
 
 		if (!try_to_freeze()) {
+			struct zonelist *zl = pgdat->node_zonelists;
 			/* We can speed up thawing tasks if we don't call
 			 * balance_pgdat after returning from the refrigerator
 			 */
+			if (!order)
+				mem_cgroup_soft_limit_reclaim(zl, GFP_KERNEL);
 			balance_pgdat(pgdat, order);
 		}
 	}

kswapd's roll is increasing free pages until zone->pages_high in "own node". mem_cgroup_soft_limit_reclaim() free one (or more) exceed page in any node.

Quoted 5 lines

Oh, well.
I think it is not consistency.

if mem_cgroup_soft_limit_reclaim() is aware to target node and its pages_high, I'm glad.

Yes, correct the role of kswapd is to keep increasing free pages until zone->pages_high and the first set of pages to consider is the memory controller over their soft limits. We pass the zonelist to ensure that while doing soft reclaim, we focus on the zonelist associated with the node. Kamezawa had concernes over calling the soft limit reclaim from __alloc_pages_internal(), did you prefer that call path?

Quoted 27 lines · KOSAKI Motohiro

Quoted 20 lines

Hi Balbir,

mm/vmscan.c static int kswapd(void *p) · line 2027

Quoted 13 lines

mm/vmscan.c +3 -0

@@ -2015,9 +2016,12 @@ static int kswapd(void *p)
 		finish_wait(&pgdat->kswapd_wait, &wait);
 
 		if (!try_to_freeze()) {
+			struct zonelist *zl = pgdat->node_zonelists;
 			/* We can speed up thawing tasks if we don't call
 			 * balance_pgdat after returning from the refrigerator
 			 */
+			if (!order)
+				mem_cgroup_soft_limit_reclaim(zl, GFP_KERNEL);
 			balance_pgdat(pgdat, order);
 		}
 	}

kswapd's roll is increasing free pages until zone->pages_high in "own node". mem_cgroup_soft_limit_reclaim() free one (or more) exceed page in any node.

Quoted 5 lines

Oh, well.
I think it is not consistency.

if mem_cgroup_soft_limit_reclaim() is aware to target node and its pages_high, I'm glad.

Quoted 6 lines

Yes, correct the role of kswapd is to keep increasing free pages until zone->pages_high and the first set of pages to consider is the memory controller over their soft limits. We pass the zonelist to ensure that while doing soft reclaim, we focus on the zonelist associated with the node. Kamezawa had concernes over calling the soft limit reclaim from __alloc_pages_internal(), did you prefer that call path?

I read your patch again.
So, mem_cgroup_soft_limit_reclaim() caller place seems in balance_pgdat() is better.

Please imazine most bad scenario.
CPU0 (kswapd) take to continue shrinking.
CPU1 take another activity and charge memcg conteniously. At that time, balance_pgdat() don't exit very long time. then mem_cgroup_soft_limit_reclaim() is never called.

In ideal, if another cpu take another charge, kswapd should shrink soft limit again.

btw, I don't like "if (!order)" condition. memcg soft limit sould be always shrinked although
it's the order of because wakeup_kswapd() argument is merely hint.

another process want another order.

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

Quoted 9 lines

Quoted 8 lines

kswapd's roll is increasing free pages until zone->pages_high in "own node". mem_cgroup_soft_limit_reclaim() free one (or more) exceed page in any node.

Oh, well.
I think it is not consistency.

if mem_cgroup_soft_limit_reclaim() is aware to target node and its pages_high, I'm glad.

Quoted 6 lines

Yes, correct the role of kswapd is to keep increasing free pages until zone->pages_high and the first set of pages to consider is the memory controller over their soft limits. We pass the zonelist to ensure that while doing soft reclaim, we focus on the zonelist associated with the node. Kamezawa had concernes over calling the soft limit reclaim from __alloc_pages_internal(), did you prefer that call path?

Quoted 9 lines

I read your patch again.
So, mem_cgroup_soft_limit_reclaim() caller place seems in balance_pgdat() is better.

Please imazine most bad scenario.
CPU0 (kswapd) take to continue shrinking.
CPU1 take another activity and charge memcg conteniously. At that time, balance_pgdat() don't exit very long time. then mem_cgroup_soft_limit_reclaim() is never called.

Yes, true... that is why I added the hooks in __alloc_pages_internal() in the first two revisions, but Kamezawa objected to them. In the scenario that you mention that balance_pgdat() is busy, if we are under global system memory pressure, even after freeing memory from soft limited cgroups, we don't have sufficient free memory. We need to go reclaim from the whole system. An administrator can easily avoid the above scenario by using hard limits on the cgroup running on CPU1.

From: Balbir Singh <[email protected]>

Changelog v3...v2
1. Implemented several review comments from Kosaki-San and Kamezawa-San Please see individual changelogs for changes

Changelog v2...v1
1. Soft limits now support hierarchies
2. Use spinlocks instead of mutexes for synchronization of the RB tree

Here is v3 of the new soft limit implementation. Soft limits is a new feature for the memory resource controller, something similar has existed in the group scheduler in the form of shares. The CPU controllers interpretation of shares is very different though.

Soft limits are the most useful feature to have for environments where the administrator wants to overcommit the system, such that only on memory contention do the limits become active. The current soft limits implementation provides a soft_limit_in_bytes interface for the memory controller and not for memory+swap controller. The implementation maintains an RB-Tree of groups that exceed their soft limit and starts reclaiming from the group that exceeds this limit by the maximum amount.

If there are no major objections to the patches, I would like to get them included in -mm.

TODOs

1. The current implementation maintains the delta from the soft limit and pushes back groups to their soft limits, a ratio of delta/soft_limit might be more useful
2. It would be nice to have more targetted reclaim (in terms of pages to recalim) interface. So that groups are pushed back, close to their soft limits.

Tests
-----

I've run two memory intensive workloads with differing soft limits and seen that they are pushed back to their soft limit on contention. Their usage was their soft limit plus additional memory that they were able to grab on the system. Soft limit can take a while before we see the expected results.

Please review, comment.

Quoted 45 lines · On Sun, 01 Mar 2009 11:59:59 +0530 Balbir Singh <[email protected]>…

From: Balbir Singh <[email protected]>

Changelog v3...v2
1. Implemented several review comments from Kosaki-San and Kamezawa-San Please see individual changelogs for changes

Changelog v2...v1
1. Soft limits now support hierarchies
2. Use spinlocks instead of mutexes for synchronization of the RB tree

Here is v3 of the new soft limit implementation. Soft limits is a new feature for the memory resource controller, something similar has existed in the group scheduler in the form of shares. The CPU controllers interpretation of shares is very different though.

Soft limits are the most useful feature to have for environments where the administrator wants to overcommit the system, such that only on memory contention do the limits become active. The current soft limits implementation provides a soft_limit_in_bytes interface for the memory controller and not for memory+swap controller. The implementation maintains an RB-Tree of groups that exceed their soft limit and starts reclaiming from the group that exceeds this limit by the maximum amount.

If there are no major objections to the patches, I would like to get them included in -mm.

TODOs

1. The current implementation maintains the delta from the soft limit and pushes back groups to their soft limits, a ratio of delta/soft_limit might be more useful
2. It would be nice to have more targetted reclaim (in terms of pages to recalim) interface. So that groups are pushed back, close to their soft limits.

Tests
-----

I've run two memory intensive workloads with differing soft limits and seen that they are pushed back to their soft limit on contention. Their usage was their soft limit plus additional memory that they were able to grab on the system. Soft limit can take a while before we see the expected results.

Quoted 2 lines

Please review, comment.

Please forgive me to say....that the code itself is getting better but far from what I want. Maybe I have to show my own implementation to show my idea and the answer is between yours and mine. If now was the last year, I have enough time until distro's target kernel and may welcome any innovative patches even if it seems to give me concerns, but I have to be conservative now.

At first, it's said "When cgroup people adds something, the kernel gets slow". This is my start point of reviewing. Below is comments to this version of patch.

1. I think it's bad to add more hooks to res_counter. It's enough slow to give up adding more fancy things..

2. please avoid to add hooks to hot-path. In your patch, especially a hook to mem_cgroup_uncharge_common() is annoying me.

3. please avoid to use global spinlock more.
no lock is best. mutex is better, maybe.

4. RB-tree seems broken. Following is example. (please note you do all ops in lazy manner (once in HZ/4.)

i). while running, the tree is constructed as following

             R           R=exceed=300M
            / \ 
           A   B      A=exceed=200M  B=exceed=400M

ii) A process B exits, but and usage goes down.

   iii)      R          R=exceed=300M
            / \
           A   B      A=exceed=200M  B=exceed=10M

   vi) A new node inserted
             R         R=exceed=300M
            / \       
           A   B       A=exceed=200M B=exceed=10M
              / \
             nil C     C=exceed=310M

v) Time expires and remove "R" and do rotate.

Hmm ? Is above status is allowed ? I'm sorry if I misunderstand RBtree.

I'll post my own version in this week (more conservative version, maybe). please discuss and compare trafe-offs.

Quoted 7 lines · KAMEZAWA Hiroyuki

Quoted 2 lines · On Sun, 01 Mar 2009 11:59:59 +0530 Balbir Singh <[email protected]>…

From: Balbir Singh <[email protected]>

Quoted 5 lines

At first, it's said "When cgroup people adds something, the kernel gets slow". This is my start point of reviewing. Below is comments to this version of patch.

1. I think it's bad to add more hooks to res_counter. It's enough slow to give up adding more fancy things..

res_counters was desgined to be extensible, why is adding anything to it going to make it slow, unless we turn on soft_limits?

Quoted 3 lines

2. please avoid to add hooks to hot-path. In your patch, especially a hook to mem_cgroup_uncharge_common() is annoying me.

If soft limits are not enabled, the function does a small check and leaves.

Quoted 4 lines

3. please avoid to use global spinlock more.
no lock is best. mutex is better, maybe.

No lock to update a tree which is update concurrently?

Quoted 9 lines

4. RB-tree seems broken. Following is example. (please note you do all ops in lazy manner (once in HZ/4.)

i). while running, the tree is constructed as following

             R           R=exceed=300M
            / \ 
           A   B      A=exceed=200M  B=exceed=400M

ii) A process B exits, but and usage goes down.

That is why we have the hook in uncharge. Even if we update and the usage goes down, the tree is ordered by usage_in_excess which is updated only when the tree is updated. So what you show below does not occur. I think I should document the design better.

Quoted 9 lines · On Mon, 2 Mar 2009 10:10:43 +0530 Balbir Singh <[email protected]>…

Quoted 7 lines · KAMEZAWA Hiroyuki

Quoted 2 lines · On Sun, 01 Mar 2009 11:59:59 +0530 Balbir Singh <[email protected]>…

From: Balbir Singh <[email protected]>

Quoted 9 lines

Quoted 5 lines

At first, it's said "When cgroup people adds something, the kernel gets slow". This is my start point of reviewing. Below is comments to this version of patch.

1. I think it's bad to add more hooks to res_counter. It's enough slow to give up adding more fancy things..

res_counters was desgined to be extensible, why is adding anything to it going to make it slow, unless we turn on soft_limits?

You inserted new "if" logic in the core loop.
(What I want to say here is not that this is definitely bad but that "isn't there
any alternatives which is less overhead.)

Quoted 1 line

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

2. please avoid to add hooks to hot-path. In your patch, especially a hook to mem_cgroup_uncharge_common() is annoying me.

If soft limits are not enabled, the function does a small check and leaves.

&soft_fail_res is passed always even if memory.soft_limit==ULONG_MAX res_counter_soft_limit_excess() adds one more function call and spinlock, and irq-off.

Quoted 7 lines

Quoted 4 lines

3. please avoid to use global spinlock more.
no lock is best. mutex is better, maybe.

No lock to update a tree which is update concurrently?

Using tree/sort itself is nonsense, I believe.

Quoted 11 lines

Quoted 10 lines

4. RB-tree seems broken. Following is example. (please note you do all ops in lazy manner (once in HZ/4.)

i). while running, the tree is constructed as following

             R           R=exceed=300M
            / \ 
           A   B      A=exceed=200M  B=exceed=400M

ii) A process B exits, but and usage goes down.

Quoted 5 lines

That is why we have the hook in uncharge. Even if we update and the usage goes down, the tree is ordered by usage_in_excess which is updated only when the tree is updated. So what you show below does not occur. I think I should document the design better.

time_check==true. So, update-tree at uncharge() only happens once in HZ/4

==

(quoted diff) +1 -0

@@ -1422,6 +1520,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 	mz = page_cgroup_zoneinfo(pc);
 	unlock_page_cgroup(pc);
 
+	mem_cgroup_check_and_update_tree(mem, true);
 	/* at swapout, this memcg will be accessed to record to swap */
 	if (ctype != MEM_CGROUP_CHARGE_TYPE_SWAPOUT)
 		css_put(&mem->css);

==
Then, not-sorted RB-tree can be there.

BTW,
time_after(jiffies, 0)
is buggy (see definition). If you want make this true always,
time_after(jiffies, jiffies +1)

Quoted 7 lines

Then, not-sorted RB-tree can be there.

BTW,
time_after(jiffies, 0)
is buggy (see definition). If you want make this true always,
time_after(jiffies, jiffies +1)

HZ/4 is 250/4 jiffies in the worst case (62). We have time_after(jiffies, next_update_interval) and next_update_interval is set to last_tree_update + 62. Not sure if I got what you are pointing to.

Quoted 13 lines · On Mon, 2 Mar 2009 11:35:19 +0530 Balbir Singh <[email protected]>…

Quoted 7 lines

Then, not-sorted RB-tree can be there.

BTW,
time_after(jiffies, 0)
is buggy (see definition). If you want make this true always,
time_after(jiffies, jiffies +1)

HZ/4 is 250/4 jiffies in the worst case (62). We have time_after(jiffies, next_update_interval) and next_update_interval is set to last_tree_update + 62. Not sure if I got what you are pointing to.

(quoted diff) +20 -0

+	unsigned long next_update = 0;
+	unsigned long flags;
+
+	if (!css_tryget(&mem->css))
+		return;
+	prev_usage_in_excess = mem->usage_in_excess;
+	new_usage_in_excess = res_counter_soft_limit_excess(&mem->res);
+
+	if (time_check)
+		next_update = mem->last_tree_update +
+				MEM_CGROUP_TREE_UPDATE_INTERVAL;
+	if (new_usage_in_excess && time_after(jiffies, next_update)) {
+		if (prev_usage_in_excess)
+			mem_cgroup_remove_exceeded(mem);
+		mem_cgroup_insert_exceeded(mem);
+		updated_tree = true;
+	} else if (prev_usage_in_excess && !new_usage_in_excess) {
+		mem_cgroup_remove_exceeded(mem);
+		updated_tree = true;
+	}

My point is what happens if time_check==false.
time_afrter(jiffies, 0) is buggy.

Quoted 37 lines · KAMEZAWA Hiroyuki

Quoted 13 lines · On Mon, 2 Mar 2009 11:35:19 +0530 Balbir Singh <[email protected]>…

Quoted 7 lines

Then, not-sorted RB-tree can be there.

BTW,
time_after(jiffies, 0)
is buggy (see definition). If you want make this true always,
time_after(jiffies, jiffies +1)

HZ/4 is 250/4 jiffies in the worst case (62). We have time_after(jiffies, next_update_interval) and next_update_interval is set to last_tree_update + 62. Not sure if I got what you are pointing to.

(quoted diff) +20 -0

+	unsigned long next_update = 0;
+	unsigned long flags;
+
+	if (!css_tryget(&mem->css))
+		return;
+	prev_usage_in_excess = mem->usage_in_excess;
+	new_usage_in_excess = res_counter_soft_limit_excess(&mem->res);
+
+	if (time_check)
+		next_update = mem->last_tree_update +
+				MEM_CGROUP_TREE_UPDATE_INTERVAL;
+	if (new_usage_in_excess && time_after(jiffies, next_update)) {
+		if (prev_usage_in_excess)
+			mem_cgroup_remove_exceeded(mem);
+		mem_cgroup_insert_exceeded(mem);
+		updated_tree = true;
+	} else if (prev_usage_in_excess && !new_usage_in_excess) {
+		mem_cgroup_remove_exceeded(mem);
+		updated_tree = true;
+	}

Quoted 3 lines

My point is what happens if time_check==false.
time_afrter(jiffies, 0) is buggy.

I see your point now, but the idea behind doing so is that time_after(jiffies, 0) will always return false, which forces the prev_usage_in_excess and !new_usage_in_excess check to execute. We set the value to false only from __mem_cgroup_free().

Are you suggesting that calling time_after(jiffies, 0) is buggy? The comment

Do this with "<0" and ">=0" to only test the sign of the result. A

I think refers to the comparison check and not to the parameters. I hope I am reading this right.

Quoted 48 lines · On Mon, 2 Mar 2009 23:22:35 +0530 Balbir Singh <[email protected]>…

Quoted 37 lines · KAMEZAWA Hiroyuki

Quoted 13 lines · On Mon, 2 Mar 2009 11:35:19 +0530 Balbir Singh <[email protected]>…

Quoted 7 lines

Then, not-sorted RB-tree can be there.

BTW,
time_after(jiffies, 0)
is buggy (see definition). If you want make this true always,
time_after(jiffies, jiffies +1)

HZ/4 is 250/4 jiffies in the worst case (62). We have time_after(jiffies, next_update_interval) and next_update_interval is set to last_tree_update + 62. Not sure if I got what you are pointing to.

(quoted diff) +20 -0

+	unsigned long next_update = 0;
+	unsigned long flags;
+
+	if (!css_tryget(&mem->css))
+		return;
+	prev_usage_in_excess = mem->usage_in_excess;
+	new_usage_in_excess = res_counter_soft_limit_excess(&mem->res);
+
+	if (time_check)
+		next_update = mem->last_tree_update +
+				MEM_CGROUP_TREE_UPDATE_INTERVAL;
+	if (new_usage_in_excess && time_after(jiffies, next_update)) {
+		if (prev_usage_in_excess)
+			mem_cgroup_remove_exceeded(mem);
+		mem_cgroup_insert_exceeded(mem);
+		updated_tree = true;
+	} else if (prev_usage_in_excess && !new_usage_in_excess) {
+		mem_cgroup_remove_exceeded(mem);
+		updated_tree = true;
+	}

Quoted 3 lines

My point is what happens if time_check==false.
time_afrter(jiffies, 0) is buggy.

I see your point now, but the idea behind doing so is that time_after(jiffies, 0) will always return false, which forces the prev_usage_in_excess and !new_usage_in_excess check to execute. We set the value to false only from __mem_cgroup_free().

Quoted 7 lines

Are you suggesting that calling time_after(jiffies, 0) is buggy? The comment

Do this with "<0" and ">=0" to only test the sign of the result. A

I think refers to the comparison check and not to the parameters. I hope I am reading this right.

106 #define time_after(a,b) \
107 (typecheck(unsigned long, a) && \
108 typecheck(unsigned long, b) && \
109 ((long)(b) - (long)(a) < 0))

Reading above.

if b==0.

     if (long)a <0  -> false
     if (long)a >0  -> true

jiffies is unsigned value. please think of bit-pattern of signed/unsigned value.

Quoted 27 lines · KAMEZAWA Hiroyuki

Quoted 9 lines · On Mon, 2 Mar 2009 10:10:43 +0530 Balbir Singh <[email protected]>…

Quoted 7 lines · KAMEZAWA Hiroyuki

Quoted 2 lines · On Sun, 01 Mar 2009 11:59:59 +0530 Balbir Singh <[email protected]>…

From: Balbir Singh <[email protected]>

Quoted 9 lines

Quoted 5 lines

At first, it's said "When cgroup people adds something, the kernel gets slow". This is my start point of reviewing. Below is comments to this version of patch.

1. I think it's bad to add more hooks to res_counter. It's enough slow to give up adding more fancy things..

res_counters was desgined to be extensible, why is adding anything to it going to make it slow, unless we turn on soft_limits?

You inserted new "if" logic in the core loop.
(What I want to say here is not that this is definitely bad but that "isn't there
any alternatives which is less overhead.)

Quoted 1 line

Quoted 1 line

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

2. please avoid to add hooks to hot-path. In your patch, especially a hook to mem_cgroup_uncharge_common() is annoying me.

If soft limits are not enabled, the function does a small check and leaves.

&soft_fail_res is passed always even if memory.soft_limit==ULONG_MAX res_counter_soft_limit_excess() adds one more function call and spinlock, and irq-off.

OK, I see that overhead.. I'll figure out a way to work around it.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

3. please avoid to use global spinlock more.
no lock is best. mutex is better, maybe.

Quoted 4 lines

Quoted 2 lines

No lock to update a tree which is update concurrently?

Using tree/sort itself is nonsense, I believe.

I tried using prio trees in the past, but they are not easy to update either. I won't mind asking for suggestions for a data structure that can scaled well, allow quick insert/delete and search.

Quoted 51 lines · On Mon, 2 Mar 2009 11:35:19 +0530 Balbir Singh <[email protected]>…

Quoted 27 lines · KAMEZAWA Hiroyuki

Quoted 9 lines · On Mon, 2 Mar 2009 10:10:43 +0530 Balbir Singh <[email protected]>…

Quoted 7 lines · KAMEZAWA Hiroyuki

Quoted 2 lines · On Sun, 01 Mar 2009 11:59:59 +0530 Balbir Singh <[email protected]>…

From: Balbir Singh <[email protected]>

Quoted 9 lines

Quoted 5 lines

At first, it's said "When cgroup people adds something, the kernel gets slow". This is my start point of reviewing. Below is comments to this version of patch.

1. I think it's bad to add more hooks to res_counter. It's enough slow to give up adding more fancy things..

res_counters was desgined to be extensible, why is adding anything to it going to make it slow, unless we turn on soft_limits?

You inserted new "if" logic in the core loop.
(What I want to say here is not that this is definitely bad but that "isn't there
any alternatives which is less overhead.)

Quoted 1 line

Quoted 1 line

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

2. please avoid to add hooks to hot-path. In your patch, especially a hook to mem_cgroup_uncharge_common() is annoying me.

If soft limits are not enabled, the function does a small check and leaves.

&soft_fail_res is passed always even if memory.soft_limit==ULONG_MAX res_counter_soft_limit_excess() adds one more function call and spinlock, and irq-off.

OK, I see that overhead.. I'll figure out a way to work around it.

Quoted 9 lines

Quoted 7 lines

Quoted 4 lines

3. please avoid to use global spinlock more.
no lock is best. mutex is better, maybe.

No lock to update a tree which is update concurrently?

Using tree/sort itself is nonsense, I believe.

Quoted 4 lines

I tried using prio trees in the past, but they are not easy to update either. I won't mind asking for suggestions for a data structure that can scaled well, allow quick insert/delete and search.

Now, because the routine is called by kswapd() not by try_to_free.....

It's not necessary to be very very fast. That's my point.

Quoted 58 lines · KAMEZAWA Hiroyuki

Quoted 55 lines · On Mon, 2 Mar 2009 11:35:19 +0530 Balbir Singh <[email protected]>…

Quoted 27 lines · KAMEZAWA Hiroyuki

Quoted 9 lines · On Mon, 2 Mar 2009 10:10:43 +0530 Balbir Singh <[email protected]>…

Quoted 7 lines · KAMEZAWA Hiroyuki

Quoted 2 lines · On Sun, 01 Mar 2009 11:59:59 +0530 Balbir Singh <[email protected]>…

From: Balbir Singh <[email protected]>

Quoted 9 lines

Quoted 5 lines

At first, it's said "When cgroup people adds something, the kernel gets slow". This is my start point of reviewing. Below is comments to this version of patch.

1. I think it's bad to add more hooks to res_counter. It's enough slow to give up adding more fancy things..

res_counters was desgined to be extensible, why is adding anything to it going to make it slow, unless we turn on soft_limits?

You inserted new "if" logic in the core loop.
(What I want to say here is not that this is definitely bad but that "isn't there
any alternatives which is less overhead.)

Quoted 1 line

Quoted 1 line

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

2. please avoid to add hooks to hot-path. In your patch, especially a hook to mem_cgroup_uncharge_common() is annoying me.

If soft limits are not enabled, the function does a small check and leaves.

&soft_fail_res is passed always even if memory.soft_limit==ULONG_MAX res_counter_soft_limit_excess() adds one more function call and spinlock, and irq-off.

OK, I see that overhead.. I'll figure out a way to work around it.

Quoted 9 lines

Quoted 7 lines

Quoted 4 lines

3. please avoid to use global spinlock more.
no lock is best. mutex is better, maybe.

No lock to update a tree which is update concurrently?

Using tree/sort itself is nonsense, I believe.

I tried using prio trees in the past, but they are not easy to update either. I won't mind asking for suggestions for a data structure that can scaled well, allow quick insert/delete and search.

Quoted 4 lines

Now, because the routine is called by kswapd() not by try_to_free.....

It's not necessary to be very very fast. That's my point.

OK, I get your point, but whay does that make RB-Tree data structure non-sense?

Considering above 2, it's not bad to find victim by proper logic
from balance_pgdat() by using mem_cgroup_select_victim().
like this:
==
struct mem_cgroup *select_vicitim_at_soft_limit_via_balance_pgdat(int nid, int zid) {
while (?) {
vitcim = mem_cgroup_select_victim(init_mem_cgroup); #need some modification.

Quoted 65 lines · On Mon, 2 Mar 2009 12:06:49 +0530 Balbir Singh <[email protected]>…

Quoted 62 lines · KAMEZAWA Hiroyuki

Quoted 46 lines · On Mon, 2 Mar 2009 11:35:19 +0530 Balbir Singh <[email protected]>…

Quoted 27 lines · KAMEZAWA Hiroyuki

Quoted 9 lines · On Mon, 2 Mar 2009 10:10:43 +0530 Balbir Singh <[email protected]>…

Quoted 7 lines · KAMEZAWA Hiroyuki

Quoted 2 lines · On Sun, 01 Mar 2009 11:59:59 +0530 Balbir Singh <[email protected]>…

From: Balbir Singh <[email protected]>

Quoted 9 lines

Quoted 5 lines

At first, it's said "When cgroup people adds something, the kernel gets slow". This is my start point of reviewing. Below is comments to this version of patch.

1. I think it's bad to add more hooks to res_counter. It's enough slow to give up adding more fancy things..

res_counters was desgined to be extensible, why is adding anything to it going to make it slow, unless we turn on soft_limits?

You inserted new "if" logic in the core loop.
(What I want to say here is not that this is definitely bad but that "isn't there
any alternatives which is less overhead.)

Quoted 1 line

Quoted 1 line

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

2. please avoid to add hooks to hot-path. In your patch, especially a hook to mem_cgroup_uncharge_common() is annoying me.

If soft limits are not enabled, the function does a small check and leaves.

&soft_fail_res is passed always even if memory.soft_limit==ULONG_MAX res_counter_soft_limit_excess() adds one more function call and spinlock, and irq-off.

OK, I see that overhead.. I'll figure out a way to work around it.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

3. please avoid to use global spinlock more.
no lock is best. mutex is better, maybe.

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

No lock to update a tree which is update concurrently?

Using tree/sort itself is nonsense, I believe.

I tried using prio trees in the past, but they are not easy to update either. I won't mind asking for suggestions for a data structure that can scaled well, allow quick insert/delete and search.

Now, because the routine is called by kswapd() not by try_to_free.....

It's not necessary to be very very fast. That's my point.

Quoted 2 lines

OK, I get your point, but whay does that make RB-Tree data structure non-sense?

1. Until memory-shortage, rb-tree is kept to be updated and the users(kernel) has to pay its maintainace/check cost, whici is unnecessary. Considering trade-off, paying cost only when memory-shortage happens tend to be reasonable way.

2. Current "exceed" just shows "How much we got over my soft limit" but doesn't tell any information per-node/zone. Considering this, this rb-tree information will not be able to help kswapd (on NUMA).
But maintain per-node information uses too much resource.

Considering above 2, it's not bad to find victim by proper logic
from balance_pgdat() by using mem_cgroup_select_victim().
like this:
==
struct mem_cgroup *select_vicitim_at_soft_limit_via_balance_pgdat(int nid, int zid)

 {
     while (?) {
        vitcim = mem_cgroup_select_victim(init_mem_cgroup);  #need some modification.
        if (victim is not over soft-limit)
             continue;
        /* Ok this is candidate */
        usage = mem_cgroup_nid_zid_usage(mem, nid, zid); #get sum of active/inactive
        if (usage_is_enough_big)
              return victim;

     }
 }
 balance_pgdat()
 ...... find target zone....
 ...
 mem = select_victime_at_soft_limit_via_balance_pgdat(nid, zid)
 if (mem)
   sc->mem = mem;
 shrink_zone();
 if (mem) {
   sc->mem = NULL;
   css_put(&mem->css);

}
==

We have to pay scan cost but it will not be too big(if there are not thousands of memcg.) Under above, round-robin rotation is used rather than sort.

Maybe I can show you sample.....(but I'm a bit busy.)

* KAMEZAWA Hiroyuki <[email protected]> [2009-03-02 16:06:02]:

Quoted 6 lines

Quoted 1 line · On Mon, 2 Mar 2009 12:06:49 +0530 Balbir Singh <[email protected]>…

OK, I get your point, but whay does that make RB-Tree data structure

non-sense?

Quoted 1 line

Quoted 6 lines

1. Until memory-shortage, rb-tree is kept to be updated and the
users(kernel)
has to pay its maintainace/check cost, whici is unnecessary. Considering trade-off, paying cost only when memory-shortage happens
tend to
be reasonable way.

As you've seen in the code, the cost is only at an interval HZ/2 currently. The other overhead is the calculation of excess, I can try and see if we can get rid of it.

Quoted 6 lines

2. Current "exceed" just shows "How much we got over my soft limit" but
doesn't
tell any information per-node/zone. Considering this, this rb-tree information will not be able to help kswapd (on NUMA). But maintain per-node information uses too much resource.

Yes, kswapd is per-node and we try to free all pages belonging to a zonelist as specified by pgdat->node_zonelists for the memory control groups that are over their soft limit. Keeping this information per node makes no sense (exceeds information).

Quoted 16 lines

Considering above 2, it's not bad to find victim by proper logic from balance_pgdat() by using mem_cgroup_select_victim(). like this:
==
struct mem_cgroup *select_vicitim_at_soft_limit_via_balance_pgdat(int
nid, int zid)
{
while (?) {
vitcim = mem_cgroup_select_victim(init_mem_cgroup); #need some
modification.
if (victim is not over soft-limit)
continue;
/* Ok this is candidate */
usage = mem_cgroup_nid_zid_usage(mem, nid, zid); #get sum of
active/inactive

Quoted 2 lines

        if (usage_is_enough_big)
              return victim;

We currently track overall usage, so we split into per nid, zid information and use that? Is that your suggestion?

The soft limit is
also an aggregate limit, how do we define usage_is_big_enough or usage_is_enough_big? Through some heuristics?

Quoted 12 lines

     }
 }
 balance_pgdat()
 ...... find target zone....
 ...
 mem = select_victime_at_soft_limit_via_balance_pgdat(nid, zid)
 if (mem)
   sc->mem = mem;
 shrink_zone();
 if (mem) {
   sc->mem = NULL;
   css_put(&mem->css);

Quoted 6 lines

}
==

We have to pay scan cost but it will not be too big(if there are not
thousands of memcg.)
Under above, round-robin rotation is used rather than sort.

Yes, we sort, but not frequently at every page-fault but at a specified interval.

Quoted 2 lines

Maybe I can show you sample.....(but I'm a bit busy.)

Explanation and review is good, but I don't see how not-sorting will help? I need something that can help me point to the culprits quickly enough during soft limit reclaim and RB-Tree works very well for me.

Quoted 50 lines · Balbir Singh

* KAMEZAWA Hiroyuki <[email protected]> [2009-03-02 16:06:02]:

Quoted 6 lines

Quoted 1 line · On Mon, 2 Mar 2009 12:06:49 +0530 Balbir Singh <[email protected]>…

OK, I get your point, but whay does that make RB-Tree data structure

non-sense?

Quoted 1 line

Quoted 6 lines

1. Until memory-shortage, rb-tree is kept to be updated and the
users(kernel)
has to pay its maintainace/check cost, whici is unnecessary. Considering trade-off, paying cost only when memory-shortage happens
tend to
be reasonable way.

As you've seen in the code, the cost is only at an interval HZ/2 currently. The other overhead is the calculation of excess, I can try and see if we can get rid of it.

Quoted 6 lines

2. Current "exceed" just shows "How much we got over my soft limit" but
doesn't
tell any information per-node/zone. Considering this, this rb-tree information will not be able to help kswapd (on NUMA). But maintain per-node information uses too much resource.

Yes, kswapd is per-node and we try to free all pages belonging to a zonelist as specified by pgdat->node_zonelists for the memory control groups that are over their soft limit. Keeping this information per node makes no sense (exceeds information).

Quoted 18 lines

Considering above 2, it's not bad to find victim by proper logic from balance_pgdat() by using mem_cgroup_select_victim(). like this:
==
struct mem_cgroup *select_vicitim_at_soft_limit_via_balance_pgdat(int
nid, int zid)
{
while (?) {
vitcim = mem_cgroup_select_victim(init_mem_cgroup); #need some
modification.
if (victim is not over soft-limit)
continue;
/* Ok this is candidate */
usage = mem_cgroup_nid_zid_usage(mem, nid, zid); #get sum of
active/inactive

        if (usage_is_enough_big)
              return victim;

Quoted 2 lines

We currently track overall usage, so we split into per nid, zid information and use that? Is that your suggestion?

My suggestion is that current per-zone statistics interface of memcg already holds all necessary information. And aggregate usage information is not worth to be tracked becauset it's no help for kswapd.

Quoted 4 lines

The soft limit is
also an aggregate limit, how do we define usage_is_big_enough or usage_is_enough_big? Through some heuristics?

I think that if memcg/zone's page usage is not 0, it's enough big. (and round robin rotation as hierachical reclaim can be used.)

There maybe some threshold to try.

For example)
need_to_reclaim = zone->high - zone->free. if (usage_in_this_zone_of_memcg > need_to_reclaim/4)
select this.

Maybe we can adjust that later.

Quoted 25 lines

Quoted 12 lines

     }
 }
 balance_pgdat()
 ...... find target zone....
 ...
 mem = select_victime_at_soft_limit_via_balance_pgdat(nid, zid)
 if (mem)
   sc->mem = mem;
 shrink_zone();
 if (mem) {
   sc->mem = NULL;
   css_put(&mem->css);

Quoted 6 lines

}
==

We have to pay scan cost but it will not be too big(if there are not
thousands of memcg.)
Under above, round-robin rotation is used rather than sort.

Yes, we sort, but not frequently at every page-fault but at a specified interval.

Quoted 2 lines

Maybe I can show you sample.....(but I'm a bit busy.)

Quoted 4 lines

Explanation and review is good, but I don't see how not-sorting will help? I need something that can help me point to the culprits quickly enough during soft limit reclaim and RB-Tree works very well for me.

I don't think "tracking memcg which exceeds soft limit" is not worth to do in synchronous way. It can be done in lazy way when it's necessary in simpler logic.

BTW, did you do set-softlimit-zero and rmdir() test ?
At quick review, memcg will never be removed from RB tree because force_empty moves account from children to parent. But no tree ops there. plz see mem_cgroup_move_account().

I'm sorry if I missed something.

Quoted 54 lines · KAMEZAWA Hiroyuki

Quoted 52 lines · Balbir Singh

* KAMEZAWA Hiroyuki <[email protected]> [2009-03-02 16:06:02]:

Quoted 6 lines

Quoted 1 line · On Mon, 2 Mar 2009 12:06:49 +0530 Balbir Singh <[email protected]>…

OK, I get your point, but whay does that make RB-Tree data structure

non-sense?

Quoted 1 line

Quoted 6 lines

1. Until memory-shortage, rb-tree is kept to be updated and the
users(kernel)
has to pay its maintainace/check cost, whici is unnecessary. Considering trade-off, paying cost only when memory-shortage happens
tend to
be reasonable way.

As you've seen in the code, the cost is only at an interval HZ/2 currently. The other overhead is the calculation of excess, I can try and see if we can get rid of it.

Quoted 6 lines

2. Current "exceed" just shows "How much we got over my soft limit" but
doesn't
tell any information per-node/zone. Considering this, this rb-tree information will not be able to help kswapd (on NUMA). But maintain per-node information uses too much resource.

Yes, kswapd is per-node and we try to free all pages belonging to a zonelist as specified by pgdat->node_zonelists for the memory control groups that are over their soft limit. Keeping this information per node makes no sense (exceeds information).

Quoted 16 lines

Considering above 2, it's not bad to find victim by proper logic from balance_pgdat() by using mem_cgroup_select_victim(). like this:
==
struct mem_cgroup *select_vicitim_at_soft_limit_via_balance_pgdat(int
nid, int zid)
{
while (?) {
vitcim = mem_cgroup_select_victim(init_mem_cgroup); #need some
modification.
if (victim is not over soft-limit)
continue;
/* Ok this is candidate */
usage = mem_cgroup_nid_zid_usage(mem, nid, zid); #get sum of
active/inactive

Quoted 2 lines

        if (usage_is_enough_big)
              return victim;

We currently track overall usage, so we split into per nid, zid information and use that? Is that your suggestion?

Quoted 4 lines

My suggestion is that current per-zone statistics interface of memcg already holds all necessary information. And aggregate usage information is not worth to be tracked becauset it's no help for kswapd.

We have that data, but we need aggregate data to see who exceeded the limit.

Quoted 14 lines

Quoted 4 lines

The soft limit is
also an aggregate limit, how do we define usage_is_big_enough or usage_is_enough_big? Through some heuristics?

I think that if memcg/zone's page usage is not 0, it's enough big. (and round robin rotation as hierachical reclaim can be used.)

There maybe some threshold to try.

For example)
need_to_reclaim = zone->high - zone->free. if (usage_in_this_zone_of_memcg > need_to_reclaim/4)
select this.

Quoted 2 lines

Maybe we can adjust that later.

No... this looks broken by design. Even if the administrator sets a large enough limit and no soft limits, the cgroup gets reclaimed from?

Quoted 31 lines

Quoted 30 lines

Quoted 13 lines

     }
 }
 balance_pgdat()
 ...... find target zone....
 ...
 mem = select_victime_at_soft_limit_via_balance_pgdat(nid, zid)
 if (mem)
   sc->mem = mem;
 shrink_zone();
 if (mem) {
   sc->mem = NULL;
   css_put(&mem->css);

Quoted 6 lines

}
==

We have to pay scan cost but it will not be too big(if there are not
thousands of memcg.)
Under above, round-robin rotation is used rather than sort.

Yes, we sort, but not frequently at every page-fault but at a specified interval.

Quoted 2 lines

Maybe I can show you sample.....(but I'm a bit busy.)

Explanation and review is good, but I don't see how not-sorting will help? I need something that can help me point to the culprits quickly enough during soft limit reclaim and RB-Tree works very well for me.

Quoted 4 lines

I don't think "tracking memcg which exceeds soft limit" is not worth to do in synchronous way. It can be done in lazy way when it's necessary in simpler logic.

The synchronous way can be harmful if we do it every page fault. THe current logic is quite simple....no?

Quoted 6 lines

BTW, did you do set-softlimit-zero and rmdir() test ?
At quick review, memcg will never be removed from RB tree because force_empty moves account from children to parent. But no tree ops there. plz see mem_cgroup_move_account().

__mme_cgroup_free() has tree ops, shouldn't that catch this scenario?

Quoted 56 lines · On Mon, 2 Mar 2009 23:11:56 +0530 Balbir Singh <[email protected]>…

Quoted 54 lines · KAMEZAWA Hiroyuki

Quoted 52 lines · Balbir Singh

* KAMEZAWA Hiroyuki <[email protected]> [2009-03-02 16:06:02]:

Quoted 6 lines

Quoted 1 line · On Mon, 2 Mar 2009 12:06:49 +0530 Balbir Singh <[email protected]>…

OK, I get your point, but whay does that make RB-Tree data structure

non-sense?

Quoted 1 line

Quoted 6 lines

1. Until memory-shortage, rb-tree is kept to be updated and the
users(kernel)
has to pay its maintainace/check cost, whici is unnecessary. Considering trade-off, paying cost only when memory-shortage happens
tend to
be reasonable way.

As you've seen in the code, the cost is only at an interval HZ/2 currently. The other overhead is the calculation of excess, I can try and see if we can get rid of it.

Quoted 6 lines

2. Current "exceed" just shows "How much we got over my soft limit" but
doesn't
tell any information per-node/zone. Considering this, this rb-tree information will not be able to help kswapd (on NUMA). But maintain per-node information uses too much resource.

Yes, kswapd is per-node and we try to free all pages belonging to a zonelist as specified by pgdat->node_zonelists for the memory control groups that are over their soft limit. Keeping this information per node makes no sense (exceeds information).

Quoted 16 lines

Considering above 2, it's not bad to find victim by proper logic from balance_pgdat() by using mem_cgroup_select_victim(). like this:
==
struct mem_cgroup *select_vicitim_at_soft_limit_via_balance_pgdat(int
nid, int zid)
{
while (?) {
vitcim = mem_cgroup_select_victim(init_mem_cgroup); #need some
modification.
if (victim is not over soft-limit)
continue;
/* Ok this is candidate */
usage = mem_cgroup_nid_zid_usage(mem, nid, zid); #get sum of
active/inactive

Quoted 2 lines

        if (usage_is_enough_big)
              return victim;

We currently track overall usage, so we split into per nid, zid information and use that? Is that your suggestion?

Quoted 8 lines

Quoted 4 lines

My suggestion is that current per-zone statistics interface of memcg already holds all necessary information. And aggregate usage information is not worth to be tracked becauset it's no help for kswapd.

We have that data, but we need aggregate data to see who exceeded the limit.

Aggregate data is in res_counter, already.

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

The soft limit is
also an aggregate limit, how do we define usage_is_big_enough or usage_is_enough_big? Through some heuristics?

I think that if memcg/zone's page usage is not 0, it's enough big. (and round robin rotation as hierachical reclaim can be used.)

There maybe some threshold to try.

For example)
need_to_reclaim = zone->high - zone->free. if (usage_in_this_zone_of_memcg > need_to_reclaim/4)
select this.

Quoted 6 lines

Quoted 2 lines

Maybe we can adjust that later.

No... this looks broken by design. Even if the administrator sets a large enough limit and no soft limits, the cgroup gets reclaimed from?

I wrote
==
if (victim is not over soft-limit)
==
....Maybe this discussion style is bad and I should explain my approach in patch. (I can't write code today, sorry.)

Quoted 36 lines

Quoted 35 lines

Quoted 23 lines

Quoted 13 lines

     }
 }
 balance_pgdat()
 ...... find target zone....
 ...
 mem = select_victime_at_soft_limit_via_balance_pgdat(nid, zid)
 if (mem)
   sc->mem = mem;
 shrink_zone();
 if (mem) {
   sc->mem = NULL;
   css_put(&mem->css);

Quoted 6 lines

}
==

We have to pay scan cost but it will not be too big(if there are not
thousands of memcg.)
Under above, round-robin rotation is used rather than sort.

Yes, we sort, but not frequently at every page-fault but at a specified interval.

Quoted 7 lines

Quoted 2 lines

Maybe I can show you sample.....(but I'm a bit busy.)

Explanation and review is good, but I don't see how not-sorting will help? I need something that can help me point to the culprits quickly enough during soft limit reclaim and RB-Tree works very well for me.

I don't think "tracking memcg which exceeds soft limit" is not worth to do in synchronous way. It can be done in lazy way when it's necessary in simpler logic.

Quoted 2 lines

The synchronous way can be harmful if we do it every page fault. THe current logic is quite simple....no?

In my point of view, No.

For example, I can never be able to explain why Hz/4 is the best and why we have to maintain the tree while there are no memory shortage.

IMHO, Under well controlled system with cgroup, problematic applications and very huge file cache users are udner limitation. Memory shortage can be rare event after all.

But, on NUMA, because memcg just checks "usage" and doesn't check "usage-per-node", there can be memory shortage and this kind of soft-limit sounds attractive for me.

Quoted 9 lines

Quoted 6 lines

BTW, did you do set-softlimit-zero and rmdir() test ?
At quick review, memcg will never be removed from RB tree because force_empty moves account from children to parent. But no tree ops there. plz see mem_cgroup_move_account().

__mme_cgroup_free() has tree ops, shouldn't that catch this scenario?

Ok, I missed that. Thank you for clarification.

Regards.
-Kame

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

* KAMEZAWA Hiroyuki <[email protected]> [2009-03-03 08:59:14]:

Quoted 5 lines

But, on NUMA, because memcg just checks "usage" and doesn't check "usage-per-node", there can be memory shortage and this kind of soft-limit
sounds attractive for me.

Could you please elaborate further on this?

Quoted 11 lines · Balbir Singh

* KAMEZAWA Hiroyuki <[email protected]> [2009-03-03 08:59:14]:

Quoted 5 lines

But, on NUMA, because memcg just checks "usage" and doesn't check "usage-per-node", there can be memory shortage and this kind of soft-limit
sounds attractive for me.

Could you please elaborate further on this?

Try to explain by artificial example..
.
Assume a system with 4 nodes, and 1G of memory per node. ==

     Node0 -- 1G
     Node1 -- 1G
     Node2 -- 1G
     Node3 -- 1G
==
And assume there are 3 memory cgroups of following hard-limit.
==
    GroupA -- 1G
    GroupB -- 0.6G
    GroupC -- 0.6G

==
If the machine is not-NUMA and 4G SMP, we expect 1.8G of free memory and we can assume "global memory shortage" is a rare event.

But on NUMA, memory usage can be following.
==
GroupA -- 950M of usage
GrouoB -- 550M of usage
GroupC -- 550M of usage
and

     Node0 -- usage=1G
     Node1 -- usage=1G
     Node2 -- usage=50M
     Node2 -- Usage=0

==
In this case, kswapd will work on Node0, and Node1.
Softlimit will have chance to work. If the user declares GroupA's softlimit is 800M, GroupA will be victim in this case.

But we have to admit this is hard-to-use scheduling paramter. Almost all administrator will not be able to set proper value.
A useful case I can think of is creating some "victim" group and guard other groups from global memory reclaim. I think I need some study about how-to-use softlimit. But we'll need this kind of paramater,anyway and I don't have onjection to add this kind of scheduling parameter. But implementation should be simple at this stage and we should find best scheduling algorithm under use-case finally...

Quoted 7 lines

I wrote
==
if (victim is not over soft-limit)
==
....Maybe this discussion style is bad and I should explain my approach in patch. (I can't write code today, sorry.)

mm/vmscan.c +7 -2

@@ -1758,6 +1778,7 @@ static unsigned long balance_pgdat(pg_da
 {
 	int all_zones_ok;
 	int priority;
+	int nid = pgdat->node_id;
 	int i;
 	unsigned long total_scanned;
 	struct reclaim_state *reclaim_state = current->reclaim_state;

@@ -1785,6 +1806,8 @@ loop_again:
 	for (i = 0; i < pgdat->nr_zones; i++)
 		temp_priority[i] = DEF_PRIORITY;
 
+	mem_cgroup_update_softlimit_hint(nid);
+
 	for (priority = DEF_PRIORITY; priority >= 0; priority--) {
 		int end_zone = 0;	/* Inclusive.  0 = ZONE_DMA */
 		unsigned long lru_pages = 0;

@@ -1863,8 +1886,17 @@ loop_again:
 			 * zone has way too many pages free already.
 			 */
 			if (!zone_watermark_ok(zone, order, 8*zone->pages_high,
-						end_zone, 0))
-				shrink_zone(priority, zone, &sc);
+					       end_zone, 0)) {
+				/*
+				 * If priority is not so bad, try softlimit
+				 * of memcg.

Quoted 7 lines · On Tue, 3 Mar 2009 16:42:44 +0530 Balbir Singh <[email protected]>…

Quoted 7 lines

I wrote
==
if (victim is not over soft-limit)
==
....Maybe this discussion style is bad and I should explain my approach in patch. (I can't write code today, sorry.)

This is an example of my direction, " do it lazy" softlimit.

Maybe this is not perfect but this addresses almost all my concern. I hope this will be an input for you.
I didn't divide patch into small pieces intentionally to show a big picture. Thanks,
-Kame
==
From: KAMEZAWA Hiroyuki <[email protected]>

An example patch. Don't trust me, this patch may have bugs.

Quoted 21 lines · KAMEZAWA Hiroyuki

Quoted 7 lines · On Tue, 3 Mar 2009 16:42:44 +0530 Balbir Singh <[email protected]>…

Quoted 7 lines

I wrote
==
if (victim is not over soft-limit)
==
....Maybe this discussion style is bad and I should explain my approach in patch. (I can't write code today, sorry.)

This is an example of my direction, " do it lazy" softlimit.

Maybe this is not perfect but this addresses almost all my concern. I hope this will be an input for you.
I didn't divide patch into small pieces intentionally to show a big picture. Thanks,
-Kame
==
From: KAMEZAWA Hiroyuki <[email protected]>

Quoted 2 lines

An example patch. Don't trust me, this patch may have bugs.

Well this is not do it lazy, all memcg's are scanned tree is built everytime kswapd invokes soft limit reclaim.

With 100 cgroups and 5 nodes, we'll
end up scanning cgroups 500 times.

There is no ordering of selected victims,

so the largest victim might still be running unaffected.

Quoted 21 lines · KAMEZAWA Hiroyuki

Quoted 7 lines · On Tue, 3 Mar 2009 16:42:44 +0530 Balbir Singh <[email protected]>…

Quoted 7 lines

I wrote
==
if (victim is not over soft-limit)
==
....Maybe this discussion style is bad and I should explain my approach in patch. (I can't write code today, sorry.)

This is an example of my direction, " do it lazy" softlimit.

Maybe this is not perfect but this addresses almost all my concern. I hope this will be an input for you.
I didn't divide patch into small pieces intentionally to show a big picture. Thanks,
-Kame
==
From: KAMEZAWA Hiroyuki <[email protected]>

Quoted 2 lines

An example patch. Don't trust me, this patch may have bugs.

Well this is not do it lazy, all memcg's are scanned tree is built everytime kswapd invokes soft limit reclaim. With 100 cgroups and 5 nodes, we'll end up scanning cgroups 500 times. There is no ordering of selected victims, so the largest victim might still be running unaffected.