[PATCH 0/2] memcgroup: work better with tmpfs

[PATCH 0/2] memcgroup: work better with tmpfs

[Hugh Dickins]

Here's a couple of patches to get memcgroups working better with tmpfs
and shmem, in conjunction with the tmpfs patches I just posted.  There
will be another to come later on, but I shouldn't wait any longer to get
these out to you.

(The missing patch will want to leave a mem_cgroup associated with a tmpfs
file or shm object, so that if its pages get brought back from swap by
swapoff, they can be associated with that mem_cgroup rather than the one
which happens to be running swapoff.)

 mm/memcontrol.c |   81 ++++++++++++++++++++--------------------------
 mm/shmem.c      |   28 +++++++++++++++
 2 files changed, 63 insertions(+), 46 deletions(-)

But on the way I've noticed a number of issues with memcgroups not dealt
with in these patches.

1. Why is spin_lock_irqsave rather than spin_lock needed on mz->lru_lock?
If it is needed, doesn't mem_cgroup_isolate_pages need to use it too?

2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the
former be better called mem_cgroup_charge_mapped? why does the latter
test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't
understand your enums there).  But there's only mem_cgroup_uncharge.
So when, for example, an add_to_page_cache fails, the uncharge may not
balance the charge?

3. mem_cgroup_charge_common has rcu_read_lock/unlock around its
rcu_dereference; mem_cgroup_cache_charge does not: is that right?

4. That page_assign_page_cgroup in free_hot_cold_page, what case is that
handling?  Wouldn't there be a leak if it ever happens?  I've been running
with a BUG_ON(page->page_cgroup) there and not hit it - should it perhaps
be a "Bad page state" case?

Hugh

[PATCH 1/2] memcgroup: tidy up mem_cgroup_charge_common

[Hugh Dickins]

Tidy up mem_cgroup_charge_common before extending it.  Adjust some
comments, but mainly clean up its loop: I've an aversion to loops
full of continues, then a break or a goto at the bottom.  And the
is_atomic test should be on the __GFP_WAIT bit, not GFP_ATOMIC bits.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 mm/memcontrol.c |   44 +++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

--- 2.6.24-rc5-mm1/mm/memcontrol.c	2007-12-05 10:38:45.000000000 +0000
+++ memcgroup1/mm/memcontrol.c	2007-12-06 19:35:52.000000000 +0000
@@ -604,23 +604,22 @@ retry:
 			goto done;
 		}
 	}
-
 	unlock_page_cgroup(page);
 
 	pc = kzalloc(sizeof(struct page_cgroup), gfp_mask);
 	if (pc == NULL)
 		goto err;
 
-	rcu_read_lock();
 	/*
-	 * We always charge the cgroup the mm_struct belongs to
-	 * the mm_struct's mem_cgroup changes on task migration if the
+	 * We always charge the cgroup the mm_struct belongs to.
+	 * The mm_struct's mem_cgroup changes on task migration if the
 	 * thread group leader migrates. It's possible that mm is not
 	 * set, if so charge the init_mm (happens for pagecache usage).
 	 */
 	if (!mm)
 		mm = &init_mm;
 
+	rcu_read_lock();
 	mem = rcu_dereference(mm->mem_cgroup);
 	/*
 	 * For every charge from the cgroup, increment reference
@@ -634,12 +633,8 @@ retry:
 	 * the cgroup limit.
 	 */
 	while (res_counter_charge(&mem->res, PAGE_SIZE)) {
-		bool is_atomic = gfp_mask & GFP_ATOMIC;
-		/*
-		 * We cannot reclaim under GFP_ATOMIC, fail the charge
-		 */
-		if (is_atomic)
-			goto noreclaim;
+		if (!(gfp_mask & __GFP_WAIT))
+			goto out;
 
 		if (try_to_free_mem_cgroup_pages(mem, gfp_mask))
 			continue;
@@ -653,23 +648,12 @@ retry:
  		 */
 		if (res_counter_check_under_limit(&mem->res))
 			continue;
-			/*
-			 * Since we control both RSS and cache, we end up with a
-			 * very interesting scenario where we end up reclaiming
-			 * memory (essentially RSS), since the memory is pushed
-			 * to swap cache, we eventually end up adding those
-			 * pages back to our list. Hence we give ourselves a
-			 * few chances before we fail
-			 */
-		else if (nr_retries--) {
-			congestion_wait(WRITE, HZ/10);
-			continue;
+
+		if (!nr_retries--) {
+			mem_cgroup_out_of_memory(mem, gfp_mask);
+			goto out;
 		}
-noreclaim:
-		css_put(&mem->css);
-		if (!is_atomic)
-			mem_cgroup_out_of_memory(mem, GFP_KERNEL);
-		goto free_pc;
+		congestion_wait(WRITE, HZ/10);
 	}
 
 	atomic_set(&pc->ref_cnt, 1);
@@ -678,10 +662,11 @@ noreclaim:
 	pc->flags = PAGE_CGROUP_FLAG_ACTIVE;
 	if (ctype == MEM_CGROUP_CHARGE_TYPE_CACHE)
 		pc->flags |= PAGE_CGROUP_FLAG_CACHE;
+
 	if (page_cgroup_assign_new_page_cgroup(page, pc)) {
 		/*
-		 * an another charge is added to this page already.
-		 * we do take lock_page_cgroup(page) again and read
+		 * Another charge has been added to this page already.
+		 * We take lock_page_cgroup(page) again and read
 		 * page->cgroup, increment refcnt.... just retry is OK.
 		 */
 		res_counter_uncharge(&mem->res, PAGE_SIZE);
@@ -698,7 +683,8 @@ noreclaim:
 
 done:
 	return 0;
-free_pc:
+out:
+	css_put(&mem->css);
 	kfree(pc);
 err:
 	return -ENOMEM;

[PATCH 2/2] memcgroup: fix hang with shmem/tmpfs

[Hugh Dickins]

The memcgroup regime relies upon a cgroup reclaiming pages from itself
within add_to_page_cache: which may involve some waiting.  Whereas shmem
and tmpfs rely upon using add_to_page_cache while holding a spinlock:
when it cannot wait.  The consequence is that when a cgroup reaches its
limit, shmem_getpage just hangs - unless there is outside memory pressure
too, neither kswapd nor radix_tree_preload get it out of the retry loop.

In most cases we can mem_cgroup_cache_charge the page waitably first,
to attach the page_cgroup in advance, so add_to_page_cache will do no
more than increment a count; then mem_cgroup_uncharge_page after (in
both success and failure cases) to balance the books again.

And where there used to be a congestion_wait for kswapd (recently made
redundant by radix_tree_preload), use mem_cgroup_cache_charge with NULL
page to go through a cycle of allocation and freeing, without accounting
to any particular page, and without updating the statistics vector.
This brings the cgroup below its limit so the next try usually succeeds.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---
I'm struggling to remember why I added the NULL page thing: why not use
the swappage there, and charge then uncharge it?  That would distort the
statistics, yes, but does that make the NULL handling worthwhile?  But
the patch below is what I've been testing, and I must push these forward
now: overnight I'll try charging and uncharging the swappage instead.

I don't like the ~__GFP_HIGHMEM masking below: I think that would better
be enforced inside mem_cgroup_charge_common (radix_tree_preload likewise).
But I've not made that change here because it affects more files, and it's
not obvious what the correct mask should be - we need to consult Mel on
that - what category of movability should such allocations be given?

 mm/memcontrol.c |   37 +++++++++++++++++++++----------------
 mm/shmem.c      |   28 +++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 17 deletions(-)

--- memcgroup1+tmpfs9/mm/memcontrol.c	2007-12-06 19:35:52.000000000 +0000
+++ memcgroup2/mm/memcontrol.c	2007-12-06 19:49:24.000000000 +0000
@@ -588,23 +588,26 @@ static int mem_cgroup_charge_common(stru
 	 * with it
 	 */
 retry:
-	lock_page_cgroup(page);
-	pc = page_get_page_cgroup(page);
-	/*
-	 * The page_cgroup exists and the page has already been accounted
-	 */
-	if (pc) {
-		if (unlikely(!atomic_inc_not_zero(&pc->ref_cnt))) {
-			/* this page is under being uncharged ? */
-			unlock_page_cgroup(page);
-			cpu_relax();
-			goto retry;
-		} else {
-			unlock_page_cgroup(page);
-			goto done;
+	if (page) {
+		lock_page_cgroup(page);
+		pc = page_get_page_cgroup(page);
+		/*
+		 * The page_cgroup exists and
+		 * the page has already been accounted.
+		 */
+		if (pc) {
+			if (unlikely(!atomic_inc_not_zero(&pc->ref_cnt))) {
+				/* this page is under being uncharged ? */
+				unlock_page_cgroup(page);
+				cpu_relax();
+				goto retry;
+			} else {
+				unlock_page_cgroup(page);
+				goto done;
+			}
 		}
+		unlock_page_cgroup(page);
 	}
-	unlock_page_cgroup(page);
 
 	pc = kzalloc(sizeof(struct page_cgroup), gfp_mask);
 	if (pc == NULL)
@@ -663,7 +666,7 @@ retry:
 	if (ctype == MEM_CGROUP_CHARGE_TYPE_CACHE)
 		pc->flags |= PAGE_CGROUP_FLAG_CACHE;
 
-	if (page_cgroup_assign_new_page_cgroup(page, pc)) {
+	if (!page || page_cgroup_assign_new_page_cgroup(page, pc)) {
 		/*
 		 * Another charge has been added to this page already.
 		 * We take lock_page_cgroup(page) again and read
@@ -672,6 +675,8 @@ retry:
 		res_counter_uncharge(&mem->res, PAGE_SIZE);
 		css_put(&mem->css);
 		kfree(pc);
+		if (!page)
+			goto done;
 		goto retry;
 	}
 
--- memcgroup1+tmpfs9/mm/shmem.c	2007-12-15 19:15:10.000000000 +0000
+++ memcgroup2/mm/shmem.c	2007-12-15 19:21:34.000000000 +0000
@@ -912,9 +912,13 @@ found:
 	error = 1;
 	if (!inode)
 		goto out;
-	error = radix_tree_preload(GFP_KERNEL);
+	/* Precharge page while we can wait, compensate afterwards */
+	error = mem_cgroup_cache_charge(page, current->mm, GFP_KERNEL);
 	if (error)
 		goto out;
+	error = radix_tree_preload(GFP_KERNEL);
+	if (error)
+		goto uncharge;
 	error = 1;
 
 	spin_lock(&info->lock);
@@ -947,6 +951,8 @@ found:
 		shmem_swp_unmap(ptr);
 	spin_unlock(&info->lock);
 	radix_tree_preload_end();
+uncharge:
+	mem_cgroup_uncharge_page(page);
 out:
 	unlock_page(page);
 	page_cache_release(page);
@@ -1308,6 +1314,13 @@ repeat:
 			spin_unlock(&info->lock);
 			unlock_page(swappage);
 			page_cache_release(swappage);
+			if (error == -ENOMEM) {
+				/* allow reclaim from this memory cgroup */
+				error = mem_cgroup_cache_charge(NULL,
+					current->mm, gfp & ~__GFP_HIGHMEM);
+				if (error)
+					goto failed;
+			}
 			goto repeat;
 		}
 	} else if (sgp == SGP_READ && !filepage) {
@@ -1353,6 +1366,17 @@ repeat:
 				goto failed;
 			}
 
+			/* Precharge page while we can wait, compensate after */
+			error = mem_cgroup_cache_charge(filepage, current->mm,
+							gfp & ~__GFP_HIGHMEM);
+			if (error) {
+				page_cache_release(filepage);
+				shmem_unacct_blocks(info->flags, 1);
+				shmem_free_blocks(inode, 1);
+				filepage = NULL;
+				goto failed;
+			}
+
 			spin_lock(&info->lock);
 			entry = shmem_swp_alloc(info, idx, sgp);
 			if (IS_ERR(entry))
@@ -1364,6 +1388,7 @@ repeat:
 			if (error || swap.val || 0 != add_to_page_cache_lru(
 					filepage, mapping, idx, GFP_NOWAIT)) {
 				spin_unlock(&info->lock);
+				mem_cgroup_uncharge_page(filepage);
 				page_cache_release(filepage);
 				shmem_unacct_blocks(info->flags, 1);
 				shmem_free_blocks(inode, 1);
@@ -1372,6 +1397,7 @@ repeat:
 					goto failed;
 				goto repeat;
 			}
+			mem_cgroup_uncharge_page(filepage);
 			info->flags |= SHMEM_PAGEIN;
 		}
 

Re: [PATCH 0/2] memcgroup: work better with tmpfs

[KAMEZAWA Hiroyuki]

On Tue, 18 Dec 2007 22:19:22 +0000 (GMT)
Hugh Dickins <hugh@veritas.com> wrote:
> 1. Why is spin_lock_irqsave rather than spin_lock needed on mz->lru_lock?
> If it is needed, doesn't mem_cgroup_isolate_pages need to use it too?
> 
When I wrote a patch to treat lru_lock (it was not per-zone yet.), I got a
comment to use irqsafe version. So I wonder there is some plan which needs
irq safe locking.
About mem_cgroup_isolate_pages(), zone->lock is acquired with irq-disable 
before it is called. Then, it's not necesary there.

But as you say, it looks we can do it without irq-disable, now.


> 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the
> former be better called mem_cgroup_charge_mapped? why does the latter
> test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't
> understand your enums there).  But there's only mem_cgroup_uncharge.
> So when, for example, an add_to_page_cache fails, the uncharge may not
> balance the charge?
>
Ah...it seems bug. We should add type handling in uncharge. 
Then, changing control_type after start using mem_cgroup seems dangerous.
(Default is ALL now.)

Maybe following will be fix.

- allow changing contorl_type only when there is no task.
- run force_empty when control_type is changed. and drop all charges.

This will change current behavior but I think it's reasonable.
How do you think ?

> 3. mem_cgroup_charge_common has rcu_read_lock/unlock around its
> rcu_dereference; mem_cgroup_cache_charge does not: is that right?
> 
As you say, it seems bug. (sigh, my bug..)
I'd like to fix.

> 4. That page_assign_page_cgroup in free_hot_cold_page, what case is that
> handling?  Wouldn't there be a leak if it ever happens?  I've been running
> with a BUG_ON(page->page_cgroup) there and not hit it - should it perhaps
> be a "Bad page state" case?

I agree with you.

Thank you for review !

Regards
-Kame

Re: [PATCH 0/2] memcgroup: work better with tmpfs

[Hugh Dickins]

On Wed, 19 Dec 2007, KAMEZAWA Hiroyuki wrote:
> On Tue, 18 Dec 2007 22:19:22 +0000 (GMT)
> Hugh Dickins <hugh@veritas.com> wrote:
> > 1. Why is spin_lock_irqsave rather than spin_lock needed on mz->lru_lock?
> > If it is needed, doesn't mem_cgroup_isolate_pages need to use it too?
> > 
> When I wrote a patch to treat lru_lock (it was not per-zone yet.), I got a
> comment to use irqsafe version. So I wonder there is some plan which needs
> irq safe locking.

It might have come about because struct zone's lru_lock really does need
the irq disabled (because of things that happen at I/O completion time).
But I don't think struct mem_cgroup_per_zone's lru_lock needs it.

> About mem_cgroup_isolate_pages(), zone->lock is acquired with irq-disable 
> before it is called. Then, it's not necesary there.

Ah, yes, I completely missed that, thank you.

> 
> But as you say, it looks we can do it without irq-disable, now.
> 
> 
> > 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the
> > former be better called mem_cgroup_charge_mapped? why does the latter
> > test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't
> > understand your enums there).  But there's only mem_cgroup_uncharge.
> > So when, for example, an add_to_page_cache fails, the uncharge may not
> > balance the charge?
> >
> Ah...it seems bug. We should add type handling in uncharge. 
> Then, changing control_type after start using mem_cgroup seems dangerous.
> (Default is ALL now.)
> 
> Maybe following will be fix.
> 
> - allow changing contorl_type only when there is no task.
> - run force_empty when control_type is changed. and drop all charges.
> 
> This will change current behavior but I think it's reasonable.
> How do you think ?

Perhaps... my grasp on this is much too weak to be sure:
I'm pretty sceptical about changing control_type altogether.
Let me reply to Balbir, who still sees no issue there.

Hugh

Re: [PATCH 0/2] memcgroup: work better with tmpfs

[Balbir Singh]

Hugh Dickins wrote:
> Here's a couple of patches to get memcgroups working better with tmpfs
> and shmem, in conjunction with the tmpfs patches I just posted.  There
> will be another to come later on, but I shouldn't wait any longer to get
> these out to you.
> 

Hi, Hugh,

Thank you so much for the review, some comments below

> (The missing patch will want to leave a mem_cgroup associated with a tmpfs
> file or shm object, so that if its pages get brought back from swap by
> swapoff, they can be associated with that mem_cgroup rather than the one
> which happens to be running swapoff.)
> 
>  mm/memcontrol.c |   81 ++++++++++++++++++++--------------------------
>  mm/shmem.c      |   28 +++++++++++++++
>  2 files changed, 63 insertions(+), 46 deletions(-)
> 
> But on the way I've noticed a number of issues with memcgroups not dealt
> with in these patches.
> 
> 1. Why is spin_lock_irqsave rather than spin_lock needed on mz->lru_lock?
> If it is needed, doesn't mem_cgroup_isolate_pages need to use it too?
> 

We always call mem_cgroup_isolate_pages() from shrink_(in)active_pages
under spin_lock_irq of the zone's lru lock. That's the reason that we
don't explicitly use it in the routine.

> 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the
> former be better called mem_cgroup_charge_mapped? why does the latter

Yes, it would be. After we've refactored the code, the new name makes sense.

> test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't
> understand your enums there).

We do that to ensure that we charge page cache pages only when the
accounting type is set to MEM_CGROUP_TYPE_ALL. If the type is anything
else, we ignore cached pages, we did not have MEM_CGROUP_TYPE_CACHED
initially when the patches went in.

 But there's only mem_cgroup_uncharge.
> So when, for example, an add_to_page_cache fails, the uncharge may not
> balance the charge?
> 

We use mem_cgroup_uncharge() everywhere. The reason being, we might
switch control type, we uncharge pages that have a page_cgroup
associated with them, hence once we;ve charged, uncharge does not
distinguish between charge types.

> 3. mem_cgroup_charge_common has rcu_read_lock/unlock around its
> rcu_dereference; mem_cgroup_cache_charge does not: is that right?
> 

Very good catch! Will fix it.

> 4. That page_assign_page_cgroup in free_hot_cold_page, what case is that
> handling?  Wouldn't there be a leak if it ever happens?  I've been running
> with a BUG_ON(page->page_cgroup) there and not hit it - should it perhaps
> be a "Bad page state" case?
> 

Our cleanup in page_cache_uncharge() does take care of cleaning up the
page_cgroup. I think you've got it right, it should be a BUG_ON in
free_hot_cold_page()

> Hugh

Thanks for the detailed review and fixes.

-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

Re: [PATCH 0/2] memcgroup: work better with tmpfs

[Hugh Dickins]

On Wed, 19 Dec 2007, Balbir Singh wrote:
> Hugh Dickins wrote:
> 
> We always call mem_cgroup_isolate_pages() from shrink_(in)active_pages
> under spin_lock_irq of the zone's lru lock. That's the reason that we
> don't explicitly use it in the routine.

Indeed, thanks.

> 
> > 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the
> > former be better called mem_cgroup_charge_mapped? why does the latter
> 
> Yes, it would be. After we've refactored the code, the new name makes sense.
> 
> > test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't
> > understand your enums there).
> 
> We do that to ensure that we charge page cache pages only when the
> accounting type is set to MEM_CGROUP_TYPE_ALL. If the type is anything
> else, we ignore cached pages, we did not have MEM_CGROUP_TYPE_CACHED
> initially when the patches went in.

I think you've given yourself far too many degrees of freedom here.

Please explain to me how the system is supposed to behave when I
echo -n 2 >/cg/0/memory.control_type

>From the name in the enum (MEM_CGROUP_TYPE_CACHED, doesn't even have
an "= 2", yet this is a part of the API?), I think it's supposed to
account pages into and out of the page cache, but not as they're
mapped into and out of userspace.  But from the code, no references
whatever to MEM_CGROUP_TYPE_CACHED or MEM_CGROUP_TYPE_MAPPED,
it looks as if it'll behave just like MEM_CGROUP_TYPE_MAPPED
(which we hope = 1).

As you say, you didn't have MEM_CGROUP_TYPE_CACHED originally:
it looks like it got added to the straightforward case of MAPPED,
in an apparently flexible but not fully thought out way.

> 
>  But there's only mem_cgroup_uncharge.
> > So when, for example, an add_to_page_cache fails, the uncharge may not
> > balance the charge?
> > 
> 
> We use mem_cgroup_uncharge() everywhere. The reason being, we might
> switch control type, we uncharge pages that have a page_cgroup
> associated with them, hence once we;ve charged, uncharge does not
> distinguish between charge types.

Ah, so this is the meaning of that 
	/*
	 * This can handle cases when a page is not charged at all and we
	 * are switching between handling the control_type.
	 */
	if (!pc)
		return;

	if (atomic_dec_and_test(&pc->ref_cnt)) {
at the beginning of mem_cgroup_uncharge.

Sorry, that doesn't fly.  You've no locking between acquiring pc from
the page->page_cgroup, testing pc here, and decrementing its ref_cnt.
When that ref_cnt goes down to zero, clear_page_cgroup may NULLify
page->page_cgroup and kfree the pc.

So if you cannot really keep track of the ref_cnt (because of
changing control_type), that atomic_dec_and_test is in danger
of corrupting someone else's memory - isn't it?

I can just about imagine that some admins will want control_type
MAPPED and others CACHED and others MAPPED+CACHED.  But is there
actually a need for one cgroup to be controlling MAPPED while
another on the same machine is controlling MAPPED+CACHED?  And
does that make sense - there'll be weirdnesses, won't there?
And is there actually a need to change a cgroup's control_type
on the fly while it's alive?

Of course it's nice to be flexible and allow for such possibilities;
but not if that just opens windows for corruption.  My own view is
that at present you should just account mapped and cached for all,
and strip out these extra degrees of freedom: add them back in some
future when the locking is worked out.

Hugh

Re: [PATCH 0/2] memcgroup: work better with tmpfs

[Balbir Singh]

Hugh Dickins wrote:
> On Wed, 19 Dec 2007, Balbir Singh wrote:
>> Hugh Dickins wrote:
>>
>> We always call mem_cgroup_isolate_pages() from shrink_(in)active_pages
>> under spin_lock_irq of the zone's lru lock. That's the reason that we
>> don't explicitly use it in the routine.
> 
> Indeed, thanks.
> 
>>> 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the
>>> former be better called mem_cgroup_charge_mapped? why does the latter
>> Yes, it would be. After we've refactored the code, the new name makes sense.
>>
>>> test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't
>>> understand your enums there).
>> We do that to ensure that we charge page cache pages only when the
>> accounting type is set to MEM_CGROUP_TYPE_ALL. If the type is anything
>> else, we ignore cached pages, we did not have MEM_CGROUP_TYPE_CACHED
>> initially when the patches went in.
> 
> I think you've given yourself far too many degrees of freedom here.
> 
> Please explain to me how the system is supposed to behave when I
> echo -n 2 >/cg/0/memory.control_type
> 
> From the name in the enum (MEM_CGROUP_TYPE_CACHED, doesn't even have
> an "= 2", yet this is a part of the API?), I think it's supposed to
> account pages into and out of the page cache, but not as they're
> mapped into and out of userspace.  But from the code, no references
> whatever to MEM_CGROUP_TYPE_CACHED or MEM_CGROUP_TYPE_MAPPED,
> it looks as if it'll behave just like MEM_CGROUP_TYPE_MAPPED
> (which we hope = 1).
> 
> As you say, you didn't have MEM_CGROUP_TYPE_CACHED originally:
> it looks like it got added to the straightforward case of MAPPED,
> in an apparently flexible but not fully thought out way.
> 

Yes, I think your argument makes sense. I had initially thought of
making the enums a mask

MEM_CGROUP_TYPE_MAPPED = 0x1,
MEM_CGROUP_TYPE_CACHED = 0x2,
MEM_CGROUP_TYPE_ALL = 0x3

and check for control_type & MEM_CGROUP_TYPE_CACHED


>>  But there's only mem_cgroup_uncharge.
>>> So when, for example, an add_to_page_cache fails, the uncharge may not
>>> balance the charge?
>>>
>> We use mem_cgroup_uncharge() everywhere. The reason being, we might
>> switch control type, we uncharge pages that have a page_cgroup
>> associated with them, hence once we;ve charged, uncharge does not
>> distinguish between charge types.
> 
> Ah, so this is the meaning of that 
> 	/*
> 	 * This can handle cases when a page is not charged at all and we
> 	 * are switching between handling the control_type.
> 	 */
> 	if (!pc)
> 		return;
> 
> 	if (atomic_dec_and_test(&pc->ref_cnt)) {
> at the beginning of mem_cgroup_uncharge.
> 
> Sorry, that doesn't fly.  You've no locking between acquiring pc from
> the page->page_cgroup, testing pc here, and decrementing its ref_cnt.
> When that ref_cnt goes down to zero, clear_page_cgroup may NULLify
> page->page_cgroup and kfree the pc.
> 

Yes, we need to lock the page group.


> So if you cannot really keep track of the ref_cnt (because of
> changing control_type), that atomic_dec_and_test is in danger
> of corrupting someone else's memory - isn't it?
>

Yes, my bad :(

> I can just about imagine that some admins will want control_type
> MAPPED and others CACHED and others MAPPED+CACHED.  But is there
> actually a need for one cgroup to be controlling MAPPED while
> another on the same machine is controlling MAPPED+CACHED?  And
> does that make sense - there'll be weirdnesses, won't there?
> And is there actually a need to change a cgroup's control_type
> on the fly while it's alive?
> 
> Of course it's nice to be flexible and allow for such possibilities;
> but not if that just opens windows for corruption.  My own view is
> that at present you should just account mapped and cached for all,
> and strip out these extra degrees of freedom: add them back in some
> future when the locking is worked out.
> 


I am going to rip out the control_type interface for now. The locking
needs fixing irrespective of control_type. I am sending out a patch to
rip out control_type.

> Hugh

Thanks for your detailed comments,

-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL