On Tue, 18 Dec 2007 22:19:22 +0000 (GMT) Hugh Dickins <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/