Skip to contentSkip to conversation

memory controller background reclamation

RFC PATCH In review YAMAMOTO Takashi 4 participants 16 messages Oct 22, 2007 – Nov 26, 2007 10 messages not in the archive

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 memory controller background reclamation

hi,

i implemented background reclamation for your memory controller and did a few benchmarks with and without it. any comments?

YAMAMOTO Takashi

-----
"time make -j4 bzImage" in a cgroup with 64MB limit:

	without patch:
		real    22m22.389s
		user    13m35.163s
		sys     6m12.283s

		memory.failcnt after a run: 106647

	with patch:
		real    19m25.860s
		user    14m10.549s
		sys     6m13.831s

		memory.failcnt after a run: 35366

"for x in 1 2 3;do time dd if=/dev/zero bs=1M count=300|tail;done" in a cgroup with 16MB limit:

	without patch:
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 19.0058 seconds, 16.6 MB/s
		u 0.00s s 0.67s e 19.01s maj 90 min 2021 in 0 out 0
		u 0.48s s 7.22s e 93.13s maj 20475 min 210903 in 0 out 0
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 14.8394 seconds, 21.2 MB/s
		u 0.00s s 0.63s e 14.84s maj 53 min 1392 in 0 out 0
		u 0.48s s 7.00s e 94.39s maj 20125 min 211145 in 0 out 0
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 14.0635 seconds, 22.4 MB/s
		u 0.01s s 0.59s e 14.07s maj 50 min 1385 in 0 out 0
		u 0.57s s 6.93s e 91.54s maj 20359 min 210911 in 0 out 0

		memory.failcnt after a run: 8804

	with patch:
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 5.94875 seconds, 52.9 MB/s
		u 0.00s s 0.67s e 5.95s maj 206 min 5393 in 0 out 0
		u 0.45s s 6.63s e 46.07s maj 21350 min 210252 in 0 out 0
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 8.16441 seconds, 38.5 MB/s
		u 0.00s s 0.60s e 8.17s maj 240 min 4614 in 0 out 0
		u 0.45s s 6.56s e 54.56s maj 22298 min 209255 in 0 out 0
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 4.60967 seconds, 68.2 MB/s
		u 0.00s s 0.60s e 4.64s maj 196 min 4733 in 0 out 0
		u 0.46s s 6.53s e 49.28s maj 22223 min 209384 in 0 out 0

		memory.failcnt after a run: 1589

-----

mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +84 -1
@@ -27,6 +27,7 @@
 #include <linux/rcupdate.h>
 #include <linux/swap.h>
 #include <linux/spinlock.h>
+#include <linux/workqueue.h>
 #include <linux/fs.h>
 
 #include <asm/uaccess.h>
@@ -63,6 +64,8 @@ struct mem_cgroup {
 	 */
 	spinlock_t lru_lock;
 	unsigned long control_type;	/* control RSS or RSS+Pagecache */
+
+	struct work_struct reclaim_work;
 };
 
 /*
@@ -95,6 +98,9 @@ enum {
 
 static struct mem_cgroup init_mem_cgroup;
 
+static DEFINE_MUTEX(mem_cgroup_workqueue_init_lock);
+static struct workqueue_struct *mem_cgroup_workqueue;
+
 static inline
 struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)
 {
@@ -250,6 +256,69 @@ unsigned long mem_cgroup_isolate_pages(u
 	return nr_taken;
 }
 
+static int
+mem_cgroup_need_reclaim(struct mem_cgroup *mem)
+{
+	struct res_counter * const cnt = &mem->res;
+	int doreclaim;
+	unsigned long flags;
+
+	/* XXX should be in res_counter */
+	/* XXX should not hardcode a watermark */
+	spin_lock_irqsave(&cnt->lock, flags);
+	doreclaim = cnt->usage > cnt->limit / 4 * 3;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+
+	return doreclaim;
+}
+
+static void
+mem_cgroup_schedule_reclaim_if_necessary(struct mem_cgroup *mem)
+{
+
+	if (mem_cgroup_workqueue == NULL) {
+		BUG_ON(mem->css.cgroup->parent != NULL);
+		return;
+	}
+
+	if (work_pending(&mem->reclaim_work))
+		return;
+
+	if (!mem_cgroup_need_reclaim(mem))
+		return;
+
+	css_get(&mem->css); /* XXX need some thoughts wrt cgroup removal. */
+	/*
+	 * XXX workqueue is not an ideal mechanism for our purpose.
+	 * revisit later.
+	 */
+	if (!queue_work(mem_cgroup_workqueue, &mem->reclaim_work))
+		css_put(&mem->css);
+}
+
+static void
+mem_cgroup_reclaim(struct work_struct *work)
+{
+	struct mem_cgroup * const mem =
+	    container_of(work, struct mem_cgroup, reclaim_work);
+	int batch_count = 128; /* XXX arbitrary */
+
+	for (; batch_count > 0; batch_count--) {
+		if (!mem_cgroup_need_reclaim(mem))
+			break;
+		/*
+		 * XXX try_to_free_foo is not a correct mechanism to
+		 * use here.  eg. ALLOCSTALL counter
+		 * revisit later.
+		 */
+		if (!try_to_free_mem_cgroup_pages(mem, GFP_KERNEL))
+			break;
+	}
+	if (batch_count == 0)
+		mem_cgroup_schedule_reclaim_if_necessary(mem);
+	css_put(&mem->css);
+}
+
 /*
  * Charge the memory controller for page usage.
  * Return
@@ -311,6 +380,9 @@ int mem_cgroup_charge(struct page *page,
 	 */
 	while (res_counter_charge(&mem->res, PAGE_SIZE)) {
 		bool is_atomic = gfp_mask & GFP_ATOMIC;
+
+		mem_cgroup_schedule_reclaim_if_necessary(mem);
+
 		/*
 		 * We cannot reclaim under GFP_ATOMIC, fail the charge
 		 */
@@ -551,8 +623,18 @@ mem_cgroup_create(struct cgroup_subsys *
 	if (unlikely((cont->parent) == NULL)) {
 		mem = &init_mem_cgroup;
 		init_mm.mem_cgroup = mem;
-	} else
+	} else {
+		/* XXX too late for the top-level cgroup */
+		if (mem_cgroup_workqueue == NULL) {
+			mutex_lock(&mem_cgroup_workqueue_init_lock);
+			if (mem_cgroup_workqueue == NULL) {
+				mem_cgroup_workqueue =
+				    create_workqueue("mem_cgroup");
+			}
+			mutex_unlock(&mem_cgroup_workqueue_init_lock);
+		}
 		mem = kzalloc(sizeof(struct mem_cgroup), GFP_KERNEL);
+	}
 
 	if (mem == NULL)
 		return NULL;
@@ -562,6 +644,7 @@ mem_cgroup_create(struct cgroup_subsys *
 	INIT_LIST_HEAD(&mem->inactive_list);
 	spin_lock_init(&mem->lru_lock);
 	mem->control_type = MEM_CGROUP_TYPE_ALL;
+	INIT_WORK(&mem->reclaim_work, mem_cgroup_reclaim);
 	return &mem->css;
 }

A message that is not in the archive (sent off-list or not yet received): 20071022094349.60B351C1015-Pcsii4f/[email protected]

#2 lore raw
Quoted 57 lines · YAMAMOTO Takashi

hi,

i implemented background reclamation for your memory controller and did a few benchmarks with and without it. any comments?

YAMAMOTO Takashi

-----
"time make -j4 bzImage" in a cgroup with 64MB limit:

	without patch:
		real    22m22.389s
		user    13m35.163s
		sys     6m12.283s

		memory.failcnt after a run: 106647

	with patch:
		real    19m25.860s
		user    14m10.549s
		sys     6m13.831s

		memory.failcnt after a run: 35366

"for x in 1 2 3;do time dd if=/dev/zero bs=1M count=300|tail;done" in a cgroup with 16MB limit:

	without patch:
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 19.0058 seconds, 16.6 MB/s
		u 0.00s s 0.67s e 19.01s maj 90 min 2021 in 0 out 0
		u 0.48s s 7.22s e 93.13s maj 20475 min 210903 in 0 out 0
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 14.8394 seconds, 21.2 MB/s
		u 0.00s s 0.63s e 14.84s maj 53 min 1392 in 0 out 0
		u 0.48s s 7.00s e 94.39s maj 20125 min 211145 in 0 out 0
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 14.0635 seconds, 22.4 MB/s
		u 0.01s s 0.59s e 14.07s maj 50 min 1385 in 0 out 0
		u 0.57s s 6.93s e 91.54s maj 20359 min 210911 in 0 out 0

		memory.failcnt after a run: 8804

	with patch:
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 5.94875 seconds, 52.9 MB/s
		u 0.00s s 0.67s e 5.95s maj 206 min 5393 in 0 out 0
		u 0.45s s 6.63s e 46.07s maj 21350 min 210252 in 0 out 0
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 8.16441 seconds, 38.5 MB/s
		u 0.00s s 0.60s e 8.17s maj 240 min 4614 in 0 out 0
Quoted 8 lines
		u 0.45s s 6.56s e 54.56s maj 22298 min 209255 in 0 out 0
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 4.60967 seconds, 68.2 MB/s
		u 0.00s s 0.60s e 4.64s maj 196 min 4733 in 0 out 0
		u 0.46s s 6.53s e 49.28s maj 22223 min 209384 in 0 out 0

		memory.failcnt after a run: 1589

Looks quite nice!

Quoted 35 lines

-----

mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +6 -0
@@ -27,6 +27,7 @@
 #include <linux/rcupdate.h>
 #include <linux/swap.h>
 #include <linux/spinlock.h>
+#include <linux/workqueue.h>
 #include <linux/fs.h>
 
 #include <asm/uaccess.h>
@@ -63,6 +64,8 @@ struct mem_cgroup {
 	 */
 	spinlock_t lru_lock;
 	unsigned long control_type;	/* control RSS or RSS+Pagecache */
+
+	struct work_struct reclaim_work;
 };
 
 /*
@@ -95,6 +98,9 @@ enum {
 
 static struct mem_cgroup init_mem_cgroup;
 
+static DEFINE_MUTEX(mem_cgroup_workqueue_init_lock);
+static struct workqueue_struct *mem_cgroup_workqueue;
+
 static inline
 struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)
 {
@@ -250,6 +256,69 @@ unsigned long mem_cgroup_isolate_pages(u
 	return nr_taken;
 }

mm/memcontrol.c unsigned long mem_cgroup_isolate_pages(u · line 267

Quoted 10 lines
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +9 -0
 
+static int
+mem_cgroup_need_reclaim(struct mem_cgroup *mem)
+{
+	struct res_counter * const cnt = &mem->res;
+	int doreclaim;
+	unsigned long flags;
+
+	/* XXX should be in res_counter */
+	/* XXX should not hardcode a watermark */

We could add the following API to resource counters

res_counter_set_low_watermark
res_counter_set_high_watermark
res_counter_below_low_watermark
res_counter_above_high_watermark

and add

low_watermark
high_watermark

members to the resource group. We could push out data upto the low watermark from the cgroup.

Quoted 37 lines
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +37 -0
+	spin_lock_irqsave(&cnt->lock, flags);
+	doreclaim = cnt->usage > cnt->limit / 4 * 3;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+
+	return doreclaim;
+}
+
+static void
+mem_cgroup_schedule_reclaim_if_necessary(struct mem_cgroup *mem)
+{
+
+	if (mem_cgroup_workqueue == NULL) {
+		BUG_ON(mem->css.cgroup->parent != NULL);
+		return;
+	}
+
+	if (work_pending(&mem->reclaim_work))
+		return;
+
+	if (!mem_cgroup_need_reclaim(mem))
+		return;
+
+	css_get(&mem->css); /* XXX need some thoughts wrt cgroup removal. */
+	/*
+	 * XXX workqueue is not an ideal mechanism for our purpose.
+	 * revisit later.
+	 */
+	if (!queue_work(mem_cgroup_workqueue, &mem->reclaim_work))
+		css_put(&mem->css);
+}
+
+static void
+mem_cgroup_reclaim(struct work_struct *work)
+{
+	struct mem_cgroup * const mem =
+	    container_of(work, struct mem_cgroup, reclaim_work);
+	int batch_count = 128; /* XXX arbitrary */

mm/memcontrol.c unsigned long mem_cgroup_isolate_pages(u · line 314

Quoted 10 lines
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +10 -0
+
+	for (; batch_count > 0; batch_count--) {
+		if (!mem_cgroup_need_reclaim(mem))
+			break;
+		/*
+		 * XXX try_to_free_foo is not a correct mechanism to
+		 * use here.  eg. ALLOCSTALL counter
+		 * revisit later.
+		 */
+		if (!try_to_free_mem_cgroup_pages(mem, GFP_KERNEL))

We could make try_to_free_mem_cgroup_pages, batch aware and pass that in scan_control.

Quoted 48 lines
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +22 -1
+			break;
+	}
+	if (batch_count == 0)
+		mem_cgroup_schedule_reclaim_if_necessary(mem);
+	css_put(&mem->css);
+}
+
 /*
  * Charge the memory controller for page usage.
  * Return
@@ -311,6 +380,9 @@ int mem_cgroup_charge(struct page *page,
 	 */
 	while (res_counter_charge(&mem->res, PAGE_SIZE)) {
 		bool is_atomic = gfp_mask & GFP_ATOMIC;
+
+		mem_cgroup_schedule_reclaim_if_necessary(mem);
+
 		/*
 		 * We cannot reclaim under GFP_ATOMIC, fail the charge
 		 */
@@ -551,8 +623,18 @@ mem_cgroup_create(struct cgroup_subsys *
 	if (unlikely((cont->parent) == NULL)) {
 		mem = &init_mem_cgroup;
 		init_mm.mem_cgroup = mem;
-	} else
+	} else {
+		/* XXX too late for the top-level cgroup */
+		if (mem_cgroup_workqueue == NULL) {
+			mutex_lock(&mem_cgroup_workqueue_init_lock);
+			if (mem_cgroup_workqueue == NULL) {
+				mem_cgroup_workqueue =
+				    create_workqueue("mem_cgroup");
+			}
+			mutex_unlock(&mem_cgroup_workqueue_init_lock);
+		}
 		mem = kzalloc(sizeof(struct mem_cgroup), GFP_KERNEL);
+	}
 
 	if (mem == NULL)
 		return NULL;
@@ -562,6 +644,7 @@ mem_cgroup_create(struct cgroup_subsys *
 	INIT_LIST_HEAD(&mem->inactive_list);
 	spin_lock_init(&mem->lru_lock);
 	mem->control_type = MEM_CGROUP_TYPE_ALL;
+	INIT_WORK(&mem->reclaim_work, mem_cgroup_reclaim);
 	return &mem->css;
 }
 
Signature · 4 lines
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

A message that is not in the archive (sent off-list or not yet received): 471CCB85.3040905-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org

#4 lore raw

hi,

Quoted 21 lines

mm/memcontrol.c unsigned long mem_cgroup_isolate_pages(u · line 267

Quoted 13 lines
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +9 -0
@@ -250,6 +256,69 @@ unsigned long mem_cgroup_isolate_pages(u
 	return nr_taken;
 }
 
+static int
+mem_cgroup_need_reclaim(struct mem_cgroup *mem)
+{
+	struct res_counter * const cnt = &mem->res;
+	int doreclaim;
+	unsigned long flags;
+
+	/* XXX should be in res_counter */
+	/* XXX should not hardcode a watermark */

We could add the following API to resource counters

res_counter_set_low_watermark
res_counter_set_high_watermark
res_counter_below_low_watermark
res_counter_above_high_watermark

Quoted 7 lines

and add

low_watermark
high_watermark

members to the resource group. We could push out data upto the low watermark from the cgroup.

it sounds fine to me.

Quoted 11 lines
Quoted 11 lines
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +11 -0
+static void
+mem_cgroup_reclaim(struct work_struct *work)
+{
+	struct mem_cgroup * const mem =
+	    container_of(work, struct mem_cgroup, reclaim_work);
+	int batch_count = 128; /* XXX arbitrary */
+
+	for (; batch_count > 0; batch_count--) {
+		if (!mem_cgroup_need_reclaim(mem))
+			break;
+		/*
Quoted 8 lines

mm/memcontrol.c unsigned long mem_cgroup_isolate_pages(u · line 314

Quoted 5 lines
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +5 -0
+		 * XXX try_to_free_foo is not a correct mechanism to
+		 * use here.  eg. ALLOCSTALL counter
+		 * revisit later.
+		 */
+		if (!try_to_free_mem_cgroup_pages(mem, GFP_KERNEL))

We could make try_to_free_mem_cgroup_pages, batch aware and pass that in scan_control.

in the comment above, i meant that it might be better to introduce something like balance_pgdat rather than using try_to_free_mem_cgroup_pages. with the current design of cgroup lru lists, probably it doesn't matter much except statistics, tho.

YAMAMOTO Takashi

A message that is not in the archive (sent off-list or not yet received): 20071022234430.EE6B11C1231-Pcsii4f/[email protected]

#6 lore raw
Quoted 7 lines
Quoted 7 lines

We could add the following API to resource counters

res_counter_set_low_watermark
res_counter_set_high_watermark
res_counter_below_low_watermark
res_counter_above_high_watermark

Quoted 7 lines
Quoted 7 lines

and add

low_watermark
high_watermark

members to the resource group. We could push out data upto the low watermark from the cgroup.

i implemented something like that. (and rebased to 2.6.24-rc2-mm1.)

what's the best way to expose watermarks to userland is an open question. i took the simplest way for now. do you have any suggestions?

YAMAMOTO Takashi

include/linux/res_counter.h renamed from include/linux/res_counter.h.orig +31 -0
@@ -32,6 +32,13 @@ struct res_counter {
 	 * the number of unsuccessful attempts to consume the resource
 	 */
 	unsigned long long failcnt;
+
+	/*
+	 * watermarks
+	 */
+	unsigned long long high_watermark;
+	unsigned long long low_watermark;
+
 	/*
 	 * the lock to protect all of the above.
 	 * the routines below consider this to be IRQ-safe
@@ -66,6 +73,8 @@ enum {
 	RES_USAGE,
 	RES_LIMIT,
 	RES_FAILCNT,
+	RES_HIGH_WATERMARK,
+	RES_LOW_WATERMARK,
 };
 
 /*
@@ -124,4 +133,26 @@ static inline bool res_counter_check_und
 	return ret;
 }
 
+static inline bool res_counter_below_low_watermark(struct res_counter *cnt)
+{
+	bool ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	ret = cnt->usage < cnt->low_watermark;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return ret;
+}
+
+static inline bool res_counter_above_high_watermark(struct res_counter *cnt)
+{
+	bool ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	ret = cnt->usage > cnt->high_watermark;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return ret;
+}
+
 #endif
kernel/res_counter.c renamed from kernel/res_counter.c.orig +27 -0
@@ -17,6 +17,8 @@ void res_counter_init(struct res_counter
 {
 	spin_lock_init(&counter->lock);
 	counter->limit = (unsigned long long)LLONG_MAX;
+	counter->high_watermark = (unsigned long long)LLONG_MAX;
+	counter->low_watermark = (unsigned long long)LLONG_MAX;
 }
 
 int res_counter_charge_locked(struct res_counter *counter, unsigned long val)
@@ -69,6 +71,10 @@ res_counter_member(struct res_counter *c
 		return &counter->limit;
 	case RES_FAILCNT:
 		return &counter->failcnt;
+	case RES_HIGH_WATERMARK:
+		return &counter->high_watermark;
+	case RES_LOW_WATERMARK:
+		return &counter->low_watermark;
 	};
 
 	BUG();
@@ -99,6 +105,7 @@ ssize_t res_counter_write(struct res_cou
 	int ret;
 	char *buf, *end;
 	unsigned long long tmp, *val;
+	unsigned long flags;
 
 	buf = kmalloc(nbytes + 1, GFP_KERNEL);
 	ret = -ENOMEM;
@@ -122,9 +129,29 @@ ssize_t res_counter_write(struct res_cou
 			goto out_free;
 	}
 
+	spin_lock_irqsave(&counter->lock, flags);
 	val = res_counter_member(counter, member);
+	/* ensure low_watermark <= high_watermark <= limit */
+	switch (member) {
+	case RES_LIMIT:
+		if (tmp < counter->high_watermark)
+			goto out_locked;
+		break;
+	case RES_HIGH_WATERMARK:
+		if (tmp > counter->limit || tmp < counter->low_watermark)
+			goto out_locked;
+		break;
+	case RES_LOW_WATERMARK:
+		if (tmp > counter->high_watermark)
+			goto out_locked;
+		break;
+	}
 	*val = tmp;
+	BUG_ON(counter->high_watermark > counter->limit);
+	BUG_ON(counter->low_watermark > counter->high_watermark);
 	ret = nbytes;
+out_locked:
+	spin_unlock_irqrestore(&counter->lock, flags);
 out_free:
 	kfree(buf);
 out:
mm/memcontrol.c renamed from mm/memcontrol.c.orig +83 -1

137 lines not on this page · raw message

A message that is not in the archive (sent off-list or not yet received): 20071115061602.856171CD9BF-Pcsii4f/[email protected]

#8 lore raw
Quoted 15 lines
Quoted 7 lines
Quoted 7 lines

We could add the following API to resource counters

res_counter_set_low_watermark
res_counter_set_high_watermark
res_counter_below_low_watermark
res_counter_above_high_watermark

Quoted 7 lines
Quoted 7 lines

and add

low_watermark
high_watermark

members to the resource group. We could push out data upto the low watermark from the cgroup.

Quoted 6 lines

i implemented something like that. (and rebased to 2.6.24-rc2-mm1.)

what's the best way to expose watermarks to userland is an open question. i took the simplest way for now. do you have any suggestions?

YAMAMOTO Takashi

here's a new version.

changes from the previous:
- rebase to linux-2.6.24-rc2-mm1 + kamezawa patchset.
- create workqueue when setting high watermark, rather than cgroup creation which is earlier on boot.

YAMAMOTO Takashi

---

include/linux/res_counter.h renamed from include/linux/res_counter.h.BACKUP +31 -0
@@ -32,6 +32,13 @@ struct res_counter {
 	 * the number of unsuccessful attempts to consume the resource
 	 */
 	unsigned long long failcnt;
+
+	/*
+	 * watermarks
+	 */
+	unsigned long long high_watermark;
+	unsigned long long low_watermark;
+
 	/*
 	 * the lock to protect all of the above.
 	 * the routines below consider this to be IRQ-safe
@@ -66,6 +73,8 @@ enum {
 	RES_USAGE,
 	RES_LIMIT,
 	RES_FAILCNT,
+	RES_HIGH_WATERMARK,
+	RES_LOW_WATERMARK,
 };
 
 /*
@@ -124,4 +133,26 @@ static inline bool res_counter_check_und
 	return ret;
 }
 
+static inline bool res_counter_below_low_watermark(struct res_counter *cnt)
+{
+	bool ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	ret = cnt->usage < cnt->low_watermark;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return ret;
+}
+
+static inline bool res_counter_above_high_watermark(struct res_counter *cnt)
+{
+	bool ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	ret = cnt->usage > cnt->high_watermark;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return ret;
+}
+
 #endif
kernel/res_counter.c renamed from kernel/res_counter.c.BACKUP +27 -0
@@ -17,6 +17,8 @@ void res_counter_init(struct res_counter
 {
 	spin_lock_init(&counter->lock);
 	counter->limit = (unsigned long long)LLONG_MAX;
+	counter->high_watermark = (unsigned long long)LLONG_MAX;
+	counter->low_watermark = (unsigned long long)LLONG_MAX;
 }
 
 int res_counter_charge_locked(struct res_counter *counter, unsigned long val)
@@ -69,6 +71,10 @@ res_counter_member(struct res_counter *c
 		return &counter->limit;
 	case RES_FAILCNT:
 		return &counter->failcnt;
+	case RES_HIGH_WATERMARK:
+		return &counter->high_watermark;
+	case RES_LOW_WATERMARK:
+		return &counter->low_watermark;
 	};
 
 	BUG();
@@ -99,6 +105,7 @@ ssize_t res_counter_write(struct res_cou
 	int ret;
 	char *buf, *end;
 	unsigned long long tmp, *val;
+	unsigned long flags;
 
 	buf = kmalloc(nbytes + 1, GFP_KERNEL);
 	ret = -ENOMEM;
@@ -122,9 +129,29 @@ ssize_t res_counter_write(struct res_cou
 			goto out_free;
 	}
 
+	spin_lock_irqsave(&counter->lock, flags);
 	val = res_counter_member(counter, member);
+	/* ensure low_watermark <= high_watermark <= limit */
+	switch (member) {
+	case RES_LIMIT:
+		if (tmp < counter->high_watermark)
+			goto out_locked;
+		break;
+	case RES_HIGH_WATERMARK:
+		if (tmp > counter->limit || tmp < counter->low_watermark)
+			goto out_locked;
+		break;
+	case RES_LOW_WATERMARK:
+		if (tmp > counter->high_watermark)
+			goto out_locked;
+		break;
+	}
 	*val = tmp;
+	BUG_ON(counter->high_watermark > counter->limit);
+	BUG_ON(counter->low_watermark > counter->high_watermark);
 	ret = nbytes;
+out_locked:
+	spin_unlock_irqrestore(&counter->lock, flags);
 out_free:
 	kfree(buf);
 out:
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +92 -1

143 lines not on this page · raw message

A message that is not in the archive (sent off-list or not yet received): 20071122085733.758AD1CEE9F-Pcsii4f/[email protected]

#10 lore raw
Quoted 3 lines

here's a new version.

Thank you. I'll review and try to merge yours in the next week,

Regards,
-Kame

#11 lore raw
In reply to a message not in the archive
Quoted 66 lines · YAMAMOTO Takashi

changes from the previous:
- rebase to linux-2.6.24-rc2-mm1 + kamezawa patchset.
- create workqueue when setting high watermark, rather than cgroup creation which is earlier on boot.

YAMAMOTO Takashi

---

include/linux/res_counter.h renamed from include/linux/res_counter.h.BACKUP +31 -0
@@ -32,6 +32,13 @@ struct res_counter {
 	 * the number of unsuccessful attempts to consume the resource
 	 */
 	unsigned long long failcnt;
+
+	/*
+	 * watermarks
+	 */
+	unsigned long long high_watermark;
+	unsigned long long low_watermark;
+
 	/*
 	 * the lock to protect all of the above.
 	 * the routines below consider this to be IRQ-safe
@@ -66,6 +73,8 @@ enum {
 	RES_USAGE,
 	RES_LIMIT,
 	RES_FAILCNT,
+	RES_HIGH_WATERMARK,
+	RES_LOW_WATERMARK,
 };
 
 /*
@@ -124,4 +133,26 @@ static inline bool res_counter_check_und
 	return ret;
 }
 
+static inline bool res_counter_below_low_watermark(struct res_counter *cnt)
+{
+	bool ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	ret = cnt->usage < cnt->low_watermark;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return ret;
+}
+
+static inline bool res_counter_above_high_watermark(struct res_counter *cnt)
+{
+	bool ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	ret = cnt->usage > cnt->high_watermark;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return ret;
+}
+
 #endif

kernel/res_counter.c void res_counter_init(struct res_counter · line 21

Quoted 5 lines
kernel/res_counter.c renamed from kernel/res_counter.c.BACKUP +2 -0
@@ -17,6 +17,8 @@ void res_counter_init(struct res_counter
 {
 	spin_lock_init(&counter->lock);
 	counter->limit = (unsigned long long)LLONG_MAX;
+	counter->high_watermark = (unsigned long long)LLONG_MAX;
+	counter->low_watermark = (unsigned long long)LLONG_MAX;

Should low watermark also be LLONG_MAX?

Quoted 116 lines
kernel/res_counter.c renamed from kernel/res_counter.c.BACKUP +25 -0
 }
 
 int res_counter_charge_locked(struct res_counter *counter, unsigned long val)
@@ -69,6 +71,10 @@ res_counter_member(struct res_counter *c
 		return &counter->limit;
 	case RES_FAILCNT:
 		return &counter->failcnt;
+	case RES_HIGH_WATERMARK:
+		return &counter->high_watermark;
+	case RES_LOW_WATERMARK:
+		return &counter->low_watermark;
 	};
 
 	BUG();
@@ -99,6 +105,7 @@ ssize_t res_counter_write(struct res_cou
 	int ret;
 	char *buf, *end;
 	unsigned long long tmp, *val;
+	unsigned long flags;
 
 	buf = kmalloc(nbytes + 1, GFP_KERNEL);
 	ret = -ENOMEM;
@@ -122,9 +129,29 @@ ssize_t res_counter_write(struct res_cou
 			goto out_free;
 	}
 
+	spin_lock_irqsave(&counter->lock, flags);
 	val = res_counter_member(counter, member);
+	/* ensure low_watermark <= high_watermark <= limit */
+	switch (member) {
+	case RES_LIMIT:
+		if (tmp < counter->high_watermark)
+			goto out_locked;
+		break;
+	case RES_HIGH_WATERMARK:
+		if (tmp > counter->limit || tmp < counter->low_watermark)
+			goto out_locked;
+		break;
+	case RES_LOW_WATERMARK:
+		if (tmp > counter->high_watermark)
+			goto out_locked;
+		break;
+	}
 	*val = tmp;
+	BUG_ON(counter->high_watermark > counter->limit);
+	BUG_ON(counter->low_watermark > counter->high_watermark);
 	ret = nbytes;
+out_locked:
+	spin_unlock_irqrestore(&counter->lock, flags);
 out_free:
 	kfree(buf);
 out:
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +37 -0
@@ -28,6 +28,7 @@
 #include <linux/rcupdate.h>
 #include <linux/swap.h>
 #include <linux/spinlock.h>
+#include <linux/workqueue.h>
 #include <linux/fs.h>
 #include <linux/seq_file.h>
 
@@ -138,6 +139,10 @@ struct mem_cgroup {
 	 * statistics.
 	 */
 	struct mem_cgroup_stat stat;
+	/*
+	 * background reclamation.
+	 */
+	struct work_struct reclaim_work;
 };
 
 /*
@@ -240,6 +245,21 @@ static unsigned long mem_cgroup_get_all_
 
 static struct mem_cgroup init_mem_cgroup;
 
+static DEFINE_MUTEX(mem_cgroup_workqueue_init_lock);
+static struct workqueue_struct *mem_cgroup_workqueue;
+
+static void mem_cgroup_create_workqueue(void)
+{
+
+	if (mem_cgroup_workqueue != NULL)
+		return;
+
+	mutex_lock(&mem_cgroup_workqueue_init_lock);
+	if (mem_cgroup_workqueue == NULL)
+		mem_cgroup_workqueue = create_workqueue("mem_cgroup");
+	mutex_unlock(&mem_cgroup_workqueue_init_lock);
+}
+
 static inline
 struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)
 {
@@ -566,6 +586,50 @@ unsigned long mem_cgroup_isolate_pages(u
 	return nr_taken;
 }
 
+static void
+mem_cgroup_schedule_reclaim(struct mem_cgroup *mem)
+{
+
+	if (mem_cgroup_workqueue == NULL) {
+		BUG_ON(mem->css.cgroup->parent != NULL);
+		return;
+	}
+
+	if (work_pending(&mem->reclaim_work))
+		return;
+
+	css_get(&mem->css); /* XXX need some thoughts wrt cgroup removal. */
+	/*
+	 * XXX workqueue is not an ideal mechanism for our purpose.
+	 * revisit later.
+	 */

mm/memcontrol.c unsigned long mem_cgroup_isolate_pages(u · line 615

Quoted 10 lines
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +10 -0
+	if (!queue_work(mem_cgroup_workqueue, &mem->reclaim_work))
+		css_put(&mem->css);
+}
+
+static void
+mem_cgroup_reclaim(struct work_struct *work)
+{
+	struct mem_cgroup * const mem =
+	    container_of(work, struct mem_cgroup, reclaim_work);
+	int batch_count = 128; /* XXX arbitrary */

Could we define and use something like MEM_CGROUP_BATCH_COUNT for now? Later we could consider and see if it needs to be tunable. numbers are hard to read in code.

mm/memcontrol.c unsigned long mem_cgroup_isolate_pages(u · line 416

Quoted 4 lines
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +4 -0
+
+	for (; batch_count > 0; batch_count--) {
+		if (res_counter_below_low_watermark(&mem->res))
+			break;

Shouldn't we also check to see that we start reclaim in background only when we are above the high watermark?

Quoted 67 lines
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +39 -1
+		/*
+		 * XXX try_to_free_foo is not a correct mechanism to
+		 * use here.  eg. ALLOCSTALL counter
+		 * revisit later.
+		 */
+		if (!try_to_free_mem_cgroup_pages(mem, GFP_KERNEL))
+			break;
+	}
+	if (batch_count == 0)
+		mem_cgroup_schedule_reclaim(mem);
+	css_put(&mem->css);
+}
+
 /*
  * Charge the memory controller for page usage.
  * Return
@@ -631,6 +695,12 @@ retry:
 	rcu_read_unlock();
 
 	/*
+	 * schedule background reclaim if we are above the high watermark.
+	 */
+	if (res_counter_above_high_watermark(&mem->res))
+		mem_cgroup_schedule_reclaim(mem);
+
+	/*
 	 * If we created the page_cgroup, we should free it on exceeding
 	 * the cgroup limit.
 	 */
@@ -939,9 +1009,16 @@ static ssize_t mem_cgroup_write(struct c
 				struct file *file, const char __user *userbuf,
 				size_t nbytes, loff_t *ppos)
 {
-	return res_counter_write(&mem_cgroup_from_cont(cont)->res,
+	ssize_t ret;
+
+	ret = res_counter_write(&mem_cgroup_from_cont(cont)->res,
 				cft->private, userbuf, nbytes, ppos,
 				mem_cgroup_write_strategy);
+
+	if (ret >= 0 && cft->private == RES_HIGH_WATERMARK)
+		mem_cgroup_create_workqueue();
+
+	return ret;
 }
 
 static ssize_t mem_control_type_write(struct cgroup *cont,
@@ -1097,6 +1174,18 @@ static struct cftype mem_cgroup_files[] 
 		.read = mem_cgroup_read,
 	},
 	{
+		.name = "high_watermark_in_bytes",
+		.private = RES_HIGH_WATERMARK,
+		.write = mem_cgroup_write,
+		.read = mem_cgroup_read,
+	},
+	{
+		.name = "low_watermark_in_bytes",
+		.private = RES_LOW_WATERMARK,
+		.write = mem_cgroup_write,
+		.read = mem_cgroup_read,
+	},
+	{
 		.name = "control_type",
 		.write = mem_control_type_write,
 		.read = mem_control_type_read,

mm/memcontrol.c mem_cgroup_create(struct cgroup_subsys * · line 1257

Quoted 8 lines
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +2 -0
@@ -1161,6 +1250,8 @@ mem_cgroup_create(struct cgroup_subsys *
 		if (alloc_mem_cgroup_per_zone_info(mem, node))
 			goto free_out;
 
+	INIT_WORK(&mem->reclaim_work, mem_cgroup_reclaim);
+
 	return &mem->css;
 free_out:
 	for_each_node_state(node, N_POSSIBLE)

I'll start some tests on these patches.

Signature · 4 lines
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

A message that is not in the archive (sent off-list or not yet received): 47464D04.2030906-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org

#13 lore raw

hi,

Quoted 10 lines

kernel/res_counter.c void res_counter_init(struct res_counter · line 21

Quoted 8 lines
kernel/res_counter.c renamed from kernel/res_counter.c.BACKUP +2 -0
@@ -17,6 +17,8 @@ void res_counter_init(struct res_counter
 {
 	spin_lock_init(&counter->lock);
 	counter->limit = (unsigned long long)LLONG_MAX;
+	counter->high_watermark = (unsigned long long)LLONG_MAX;
+	counter->low_watermark = (unsigned long long)LLONG_MAX;

Should low watermark also be LLONG_MAX?

what else do you suggest? 0?
currently it doesn't matter much because low_watermark is not used at all as far as high_watermark is LLONG_MAX.

Quoted 10 lines

kernel/res_counter.c void res_counter_init(struct res_counter

Quoted 6 lines
kernel/res_counter.c renamed from kernel/res_counter.c.BACKUP +6 -0
+static void
+mem_cgroup_reclaim(struct work_struct *work)
+{
+	struct mem_cgroup * const mem =
+	    container_of(work, struct mem_cgroup, reclaim_work);
+	int batch_count = 128; /* XXX arbitrary */

Could we define and use something like MEM_CGROUP_BATCH_COUNT for now? Later we could consider and see if it needs to be tunable. numbers are hard to read in code.

although i don't think it makes sense, i can do so if you prefer.

Quoted 7 lines

kernel/res_counter.c

Quoted 4 lines
kernel/res_counter.c renamed from kernel/res_counter.c.BACKUP +4 -0
+
+	for (; batch_count > 0; batch_count--) {
+		if (res_counter_below_low_watermark(&mem->res))
+			break;

Shouldn't we also check to see that we start reclaim in background only when we are above the high watermark?

i don't understand what you mean. can you explain? highwatermark is checked by mem_cgroup_charge_common before waking these threads.

Quoted 1 line

I'll start some tests on these patches.

thanks.

YAMAMOTO Takashi

A message that is not in the archive (sent off-list or not yet received): 20071126024710.8F4ED1CF411-Pcsii4f/[email protected]

#15 lore raw
Quoted 12 lines · YAMAMOTO Takashi

hi,

Quoted 9 lines

kernel/res_counter.c void res_counter_init(struct res_counter · line 21

Quoted 8 lines
kernel/res_counter.c renamed from kernel/res_counter.c.BACKUP +2 -0
@@ -17,6 +17,8 @@ void res_counter_init(struct res_counter
 {
 	spin_lock_init(&counter->lock);
 	counter->limit = (unsigned long long)LLONG_MAX;
+	counter->high_watermark = (unsigned long long)LLONG_MAX;
+	counter->low_watermark = (unsigned long long)LLONG_MAX;

Should low watermark also be LLONG_MAX?

Quoted 1 line

what else do you suggest? 0?

Something invalid or a good default value. I think LLONG_MAX is good for now, that ensures that no background reclaim happens till the administrator sets it up.

Quoted 3 lines

currently it doesn't matter much because low_watermark is not used at all as far as high_watermark is LLONG_MAX.

Don't we use by checking res_counter_below_low_watermark()?

Quoted 10 lines
Quoted 9 lines

kernel/res_counter.c void res_counter_init(struct res_counter

Quoted 6 lines
kernel/res_counter.c renamed from kernel/res_counter.c.BACKUP +6 -0
+static void
+mem_cgroup_reclaim(struct work_struct *work)
+{
+	struct mem_cgroup * const mem =
+	    container_of(work, struct mem_cgroup, reclaim_work);
+	int batch_count = 128; /* XXX arbitrary */

Could we define and use something like MEM_CGROUP_BATCH_COUNT for now? Later we could consider and see if it needs to be tunable. numbers are hard to read in code.

Quoted 2 lines

although i don't think it makes sense, i can do so if you prefer.

Using numbers like 128 make the code unreadable. I prefer something like MEM_CGROUP_BATCH_COUNT since its more readable than 128. If we ever propagate batch_count to other dependent functions, I'd much rather do it with a well defined name.

Quoted 11 lines
Quoted 6 lines

kernel/res_counter.c

Quoted 4 lines
kernel/res_counter.c renamed from kernel/res_counter.c.BACKUP +4 -0
+
+	for (; batch_count > 0; batch_count--) {
+		if (res_counter_below_low_watermark(&mem->res))
+			break;

Shouldn't we also check to see that we start reclaim in background only when we are above the high watermark?

i don't understand what you mean. can you explain? highwatermark is checked by mem_cgroup_charge_common before waking these threads.

OK, that clarifies

Quoted 5 lines
Quoted 1 line

I'll start some tests on these patches.

thanks.

YAMAMOTO Takashi

Signature · 4 lines
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

A message that is not in the archive (sent off-list or not yet received): [email protected]

#17 lore raw
Quoted 5 lines
Quoted 3 lines

currently it doesn't matter much because low_watermark is not used at all as far as high_watermark is LLONG_MAX.

Don't we use by checking res_counter_below_low_watermark()?

yes, but only when we get above highwatermark.

YAMAMOTO Takashi

#18 lore raw
In reply to a message not in the archive
Quoted 5 lines · Balbir Singh
Quoted 4 lines · YAMAMOTO Takashi
Quoted 4 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+	int batch_count = 128; /* XXX arbitrary */

Could we define and use something like MEM_CGROUP_BATCH_COUNT for now? Later we could consider and see if it needs to be tunable. numbers are hard to read in code.

Quoted 8 lines
Quoted 2 lines

although i don't think it makes sense, i can do so if you prefer.

Using numbers like 128 make the code unreadable. I prefer something like MEM_CGROUP_BATCH_COUNT since its more readable than 128. If we ever propagate batch_count to other dependent functions, I'd much rather do it with a well defined name.

I just checked we already have FORCE_UNCHARGE_BATCH, we could just rename and re-use it.

Signature · 4 lines
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

A message that is not in the archive (sent off-list or not yet received): 474A43D5.8020600-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org

#20 lore raw
Quoted 9 lines
Quoted 8 lines · Balbir Singh
Quoted 6 lines · YAMAMOTO Takashi
Quoted 4 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+	int batch_count = 128; /* XXX arbitrary */

Could we define and use something like MEM_CGROUP_BATCH_COUNT for now? Later we could consider and see if it needs to be tunable. numbers are hard to read in code.

although i don't think it makes sense, i can do so if you prefer.

Quoted 8 lines
Quoted 5 lines

Using numbers like 128 make the code unreadable. I prefer something like MEM_CGROUP_BATCH_COUNT since its more readable than 128. If we ever propagate batch_count to other dependent functions, I'd much rather do it with a well defined name.

I just checked we already have FORCE_UNCHARGE_BATCH, we could just rename and re-use it.

i don't think it's a good idea to use a single constant for completely different things.

YAMAMOTO Takashi

#21 lore raw
In reply to a message not in the archive
Quoted 4 lines

Using numbers like 128 make the code unreadable. I prefer something like MEM_CGROUP_BATCH_COUNT since its more readable than 128. If we ever propagate batch_count to other dependent functions, I'd much rather do it with a well defined name.

here's a new version.

changes from the previous:
- define a constant. (MEM_CGROUP_BG_RECLAIM_BATCH_COUNT) no functional changes.
- don't increment ALLOCSTALL vm event for mem cgroup because it isn't appropriate esp. for background reclamation. introduce MEM_CGROUP_STAT_ALLOCSTALL instead. (its value is same as memory.failcnt unless GFP_ATOMIC.)

YAMAMOTO Takashi

---

include/linux/res_counter.h renamed from include/linux/res_counter.h.BACKUP +31 -0
@@ -32,6 +32,13 @@ struct res_counter {
 	 * the number of unsuccessful attempts to consume the resource
 	 */
 	unsigned long long failcnt;
+
+	/*
+	 * watermarks
+	 */
+	unsigned long long high_watermark;
+	unsigned long long low_watermark;
+
 	/*
 	 * the lock to protect all of the above.
 	 * the routines below consider this to be IRQ-safe
@@ -66,6 +73,8 @@ enum {
 	RES_USAGE,
 	RES_LIMIT,
 	RES_FAILCNT,
+	RES_HIGH_WATERMARK,
+	RES_LOW_WATERMARK,
 };
 
 /*
@@ -124,4 +133,26 @@ static inline bool res_counter_check_und
 	return ret;
 }
 
+static inline bool res_counter_below_low_watermark(struct res_counter *cnt)
+{
+	bool ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	ret = cnt->usage < cnt->low_watermark;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return ret;
+}
+
+static inline bool res_counter_above_high_watermark(struct res_counter *cnt)
+{
+	bool ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cnt->lock, flags);
+	ret = cnt->usage > cnt->high_watermark;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+	return ret;
+}
+
 #endif
kernel/res_counter.c renamed from kernel/res_counter.c.BACKUP +27 -0
@@ -17,6 +17,8 @@ void res_counter_init(struct res_counter
 {
 	spin_lock_init(&counter->lock);
 	counter->limit = (unsigned long long)LLONG_MAX;
+	counter->high_watermark = (unsigned long long)LLONG_MAX;
+	counter->low_watermark = (unsigned long long)LLONG_MAX;
 }
 
 int res_counter_charge_locked(struct res_counter *counter, unsigned long val)
@@ -69,6 +71,10 @@ res_counter_member(struct res_counter *c
 		return &counter->limit;
 	case RES_FAILCNT:
 		return &counter->failcnt;
+	case RES_HIGH_WATERMARK:
+		return &counter->high_watermark;
+	case RES_LOW_WATERMARK:
+		return &counter->low_watermark;
 	};
 
 	BUG();
@@ -99,6 +105,7 @@ ssize_t res_counter_write(struct res_cou
 	int ret;
 	char *buf, *end;
 	unsigned long long tmp, *val;
+	unsigned long flags;
 
 	buf = kmalloc(nbytes + 1, GFP_KERNEL);
 	ret = -ENOMEM;
@@ -122,9 +129,29 @@ ssize_t res_counter_write(struct res_cou
 			goto out_free;
 	}
 
+	spin_lock_irqsave(&counter->lock, flags);
 	val = res_counter_member(counter, member);
+	/* ensure low_watermark <= high_watermark <= limit */
+	switch (member) {
+	case RES_LIMIT:
+		if (tmp < counter->high_watermark)
+			goto out_locked;
+		break;
+	case RES_HIGH_WATERMARK:
+		if (tmp > counter->limit || tmp < counter->low_watermark)
+			goto out_locked;
+		break;
+	case RES_LOW_WATERMARK:
+		if (tmp > counter->high_watermark)
+			goto out_locked;
+		break;
+	}
 	*val = tmp;
+	BUG_ON(counter->high_watermark > counter->limit);
+	BUG_ON(counter->low_watermark > counter->high_watermark);
 	ret = nbytes;
+out_locked:
+	spin_unlock_irqrestore(&counter->lock, flags);
 out_free:
 	kfree(buf);
 out:
mm/vmscan.c renamed from mm/vmscan.c.BACKUP +1 -1
@@ -1333,7 +1333,6 @@ static unsigned long do_try_to_free_page
 	unsigned long lru_pages = 0;
 	int i;
 
-	count_vm_event(ALLOCSTALL);
 	/*
 	 * mem_cgroup will not do shrink_slab.
 	 */
@@ -1432,6 +1431,7 @@ unsigned long try_to_free_pages(struct z
 		.isolate_pages = isolate_pages_global,
 	};
 
+	count_vm_event(ALLOCSTALL);
 	return do_try_to_free_pages(zones, gfp_mask, &sc);
 }
 
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +94 -1

169 lines not on this page · raw message

#22 lore raw
In reply to a message not in the archive
Quoted 4 lines · YAMAMOTO Takashi
Quoted 1 line

I'll start some tests on these patches.

thanks.

I tried testing, but the patch failed to apply. I think thats because it was ported on top of the NUMA patches by KAMEZAWA-San. I'll rebase and test.

Signature · 4 lines
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL
#23 lore raw
In reply to a message not in the archive
Quoted 65 lines · YAMAMOTO Takashi

hi,

i implemented background reclamation for your memory controller and did a few benchmarks with and without it. any comments?

YAMAMOTO Takashi

-----
"time make -j4 bzImage" in a cgroup with 64MB limit:

	without patch:
		real    22m22.389s
		user    13m35.163s
		sys     6m12.283s

		memory.failcnt after a run: 106647

	with patch:
		real    19m25.860s
		user    14m10.549s
		sys     6m13.831s

		memory.failcnt after a run: 35366

"for x in 1 2 3;do time dd if=/dev/zero bs=1M count=300|tail;done" in a cgroup with 16MB limit:

	without patch:
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 19.0058 seconds, 16.6 MB/s
		u 0.00s s 0.67s e 19.01s maj 90 min 2021 in 0 out 0
		u 0.48s s 7.22s e 93.13s maj 20475 min 210903 in 0 out 0
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 14.8394 seconds, 21.2 MB/s
		u 0.00s s 0.63s e 14.84s maj 53 min 1392 in 0 out 0
		u 0.48s s 7.00s e 94.39s maj 20125 min 211145 in 0 out 0
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 14.0635 seconds, 22.4 MB/s
		u 0.01s s 0.59s e 14.07s maj 50 min 1385 in 0 out 0
		u 0.57s s 6.93s e 91.54s maj 20359 min 210911 in 0 out 0

		memory.failcnt after a run: 8804

	with patch:
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 5.94875 seconds, 52.9 MB/s
		u 0.00s s 0.67s e 5.95s maj 206 min 5393 in 0 out 0
		u 0.45s s 6.63s e 46.07s maj 21350 min 210252 in 0 out 0
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 8.16441 seconds, 38.5 MB/s
		u 0.00s s 0.60s e 8.17s maj 240 min 4614 in 0 out 0
		u 0.45s s 6.56s e 54.56s maj 22298 min 209255 in 0 out 0
		300+0 records in
		300+0 records out
		314572800 bytes (315 MB) copied, 4.60967 seconds, 68.2 MB/s
		u 0.00s s 0.60s e 4.64s maj 196 min 4733 in 0 out 0
		u 0.46s s 6.53s e 49.28s maj 22223 min 209384 in 0 out 0

		memory.failcnt after a run: 1589
Quoted 1 line

-----

Results looks good. Background reclaim will definitely help performance. I am sure once we get the thresholds correct, the performance will improve further.

Quoted 131 lines
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +83 -1
@@ -27,6 +27,7 @@
 #include <linux/rcupdate.h>
 #include <linux/swap.h>
 #include <linux/spinlock.h>
+#include <linux/workqueue.h>
 #include <linux/fs.h>
 
 #include <asm/uaccess.h>
@@ -63,6 +64,8 @@ struct mem_cgroup {
 	 */
 	spinlock_t lru_lock;
 	unsigned long control_type;	/* control RSS or RSS+Pagecache */
+
+	struct work_struct reclaim_work;
 };
 
 /*
@@ -95,6 +98,9 @@ enum {
 
 static struct mem_cgroup init_mem_cgroup;
 
+static DEFINE_MUTEX(mem_cgroup_workqueue_init_lock);
+static struct workqueue_struct *mem_cgroup_workqueue;
+
 static inline
 struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)
 {
@@ -250,6 +256,69 @@ unsigned long mem_cgroup_isolate_pages(u
 	return nr_taken;
 }
 
+static int
+mem_cgroup_need_reclaim(struct mem_cgroup *mem)
+{
+	struct res_counter * const cnt = &mem->res;
+	int doreclaim;
+	unsigned long flags;
+
+	/* XXX should be in res_counter */
+	/* XXX should not hardcode a watermark */
+	spin_lock_irqsave(&cnt->lock, flags);
+	doreclaim = cnt->usage > cnt->limit / 4 * 3;
+	spin_unlock_irqrestore(&cnt->lock, flags);
+
+	return doreclaim;
+}
+
+static void
+mem_cgroup_schedule_reclaim_if_necessary(struct mem_cgroup *mem)
+{
+
+	if (mem_cgroup_workqueue == NULL) {
+		BUG_ON(mem->css.cgroup->parent != NULL);
+		return;
+	}
+
+	if (work_pending(&mem->reclaim_work))
+		return;
+
+	if (!mem_cgroup_need_reclaim(mem))
+		return;
+
+	css_get(&mem->css); /* XXX need some thoughts wrt cgroup removal. */
+	/*
+	 * XXX workqueue is not an ideal mechanism for our purpose.
+	 * revisit later.
+	 */
+	if (!queue_work(mem_cgroup_workqueue, &mem->reclaim_work))
+		css_put(&mem->css);
+}
+
+static void
+mem_cgroup_reclaim(struct work_struct *work)
+{
+	struct mem_cgroup * const mem =
+	    container_of(work, struct mem_cgroup, reclaim_work);
+	int batch_count = 128; /* XXX arbitrary */
+
+	for (; batch_count > 0; batch_count--) {
+		if (!mem_cgroup_need_reclaim(mem))
+			break;
+		/*
+		 * XXX try_to_free_foo is not a correct mechanism to
+		 * use here.  eg. ALLOCSTALL counter
+		 * revisit later.
+		 */
+		if (!try_to_free_mem_cgroup_pages(mem, GFP_KERNEL))
+			break;
+	}
+	if (batch_count == 0)
+		mem_cgroup_schedule_reclaim_if_necessary(mem);
+	css_put(&mem->css);
+}
+
 /*
  * Charge the memory controller for page usage.
  * Return
@@ -311,6 +380,9 @@ int mem_cgroup_charge(struct page *page,
 	 */
 	while (res_counter_charge(&mem->res, PAGE_SIZE)) {
 		bool is_atomic = gfp_mask & GFP_ATOMIC;
+
+		mem_cgroup_schedule_reclaim_if_necessary(mem);
+
 		/*
 		 * We cannot reclaim under GFP_ATOMIC, fail the charge
 		 */
@@ -551,8 +623,18 @@ mem_cgroup_create(struct cgroup_subsys *
 	if (unlikely((cont->parent) == NULL)) {
 		mem = &init_mem_cgroup;
 		init_mm.mem_cgroup = mem;
-	} else
+	} else {
+		/* XXX too late for the top-level cgroup */
+		if (mem_cgroup_workqueue == NULL) {
+			mutex_lock(&mem_cgroup_workqueue_init_lock);
+			if (mem_cgroup_workqueue == NULL) {
+				mem_cgroup_workqueue =
+				    create_workqueue("mem_cgroup");
+			}
+			mutex_unlock(&mem_cgroup_workqueue_init_lock);
+		}
 		mem = kzalloc(sizeof(struct mem_cgroup), GFP_KERNEL);
+	}
 
 	if (mem == NULL)
 		return NULL;

mm/memcontrol.c mem_cgroup_create(struct cgroup_subsys * · line 650

Quoted 7 lines
mm/memcontrol.c renamed from mm/memcontrol.c.BACKUP +1 -0
@@ -562,6 +644,7 @@ mem_cgroup_create(struct cgroup_subsys *
 	INIT_LIST_HEAD(&mem->inactive_list);
 	spin_lock_init(&mem->lru_lock);
 	mem->control_type = MEM_CGROUP_TYPE_ALL;
+	INIT_WORK(&mem->reclaim_work, mem_cgroup_reclaim);
 	return &mem->css;
 }
 

Can we potentially avoid direct reclaim and sleep if the charge is exceeded and let the background reclaim do the job and wake us up?

--Vaidy

A message that is not in the archive (sent off-list or not yet received): 471CD987.5030800-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org

#25 lore raw

hi,

Quoted 2 lines

Can we potentially avoid direct reclaim and sleep if the charge is exceeded and let the background reclaim do the job and wake us up?

yes, but i left it for now because direct reclaim has its own benefits.

YAMAMOTO Takashi