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

[Balbir Singh <balbir@linux.vnet.ibm.com>]

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