On Wed, 3 Mar 2010 15:01:37 +0900 Daisuke Nishimura <[email protected]> wrote:
> On Wed, 3 Mar 2010 12:29:06 +0900, KAMEZAWA Hiroyuki > <[email protected]> wrote: > > On Wed, 3 Mar 2010 11:12:38 +0900 > > Daisuke Nishimura <[email protected]> wrote: > > > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > > > index fe09e51..f85acae 100644 > > > > --- a/mm/filemap.c > > > > +++ b/mm/filemap.c > > > > @@ -135,6 +135,7 @@ void __remove_from_page_cache(struct page *page) > > > > * having removed the page entirely. > > > > */ > > > > if (PageDirty(page) && mapping_cap_account_dirty(mapping)) { > > > > + mem_cgroup_update_stat(page, > > > > MEM_CGROUP_STAT_FILE_DIRTY, -1); > > > > dec_zone_page_state(page, NR_FILE_DIRTY); > > > > dec_bdi_stat(mapping->backing_dev_info, BDI_DIRTY); > > > > } > > > (snip) > > > > @@ -1096,6 +1113,7 @@ int __set_page_dirty_no_writeback(struct page > > > > *page) > > > > void account_page_dirtied(struct page *page, struct address_space > > > > *mapping) > > > > { > > > > if (mapping_cap_account_dirty(mapping)) { > > > > + mem_cgroup_update_stat(page, > > > > MEM_CGROUP_STAT_FILE_DIRTY, 1); > > > > __inc_zone_page_state(page, NR_FILE_DIRTY); > > > > __inc_bdi_stat(mapping->backing_dev_info, BDI_DIRTY); > > > > task_dirty_inc(current); > > > As long as I can see, those two functions(at least) calls > > > mem_cgroup_update_state(), > > > which acquires page cgroup lock, under mapping->tree_lock. > > > But as I fixed before in commit e767e056, page cgroup lock must not > > > acquired under > > > mapping->tree_lock. > > > hmm, we should call those mem_cgroup_update_state() outside > > > mapping->tree_lock, > > > or add local_irq_save/restore() around lock/unlock_page_cgroup() to avoid > > > dead-lock. > > > > > Ah, good catch! But hmmmmmm... > > This account_page_dirtted() seems to be called under IRQ-disabled. > > About __remove_from_page_cache(), I think page_cgroup should have its own > > DIRTY flag, > > then, mem_cgroup_uncharge_page() can handle it automatically. > > > > But. there are no guarantee that following never happens. > > lock_page_cgroup() > > <=== interrupt. > > -> mapping->tree_lock() > > Even if mapping->tree_lock is held with IRQ-disabled. > > Then, if we add local_irq_save(), we have to add it to all > > lock_page_cgroup(). > > > > Then, hm...some kind of new trick ? as.. > > (Follwoing patch is not tested!!) > > > If we can verify that all callers of mem_cgroup_update_stat() have always > either aquired > or not aquired tree_lock, this direction will work fine. > But if we can't, we have to add local_irq_save() to lock_page_cgroup() like > below. > Agreed. Let's try how we can write a code in clean way. (we have time ;) For now, to me, IRQ disabling while lock_page_cgroup() seems to be a little over killing. What I really want is lockless code...but it seems impossible under current implementation. I wonder the fact "the page is never unchareged under us" can give us some chances ...Hmm. Thanks, -Kame _______________________________________________ Containers mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/containers _______________________________________________ Devel mailing list [email protected] https://openvz.org/mailman/listinfo/devel
