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

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

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

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