On Wed, Apr 15, 2009 at 11:15:28AM +0900, KAMEZAWA Hiroyuki wrote:
> > +/*
> > + * This function is used to make a given page have the bio-cgroup id of
> > + * the owner of this page.
> > + */
> > +void bio_cgroup_set_owner(struct page *page, struct mm_struct *mm)
> > +{
> > + struct bio_cgroup *biog;
> > + struct page_cgroup *pc;
> > +
> > + if (bio_cgroup_disabled())
> > + return;
> > + pc = lookup_page_cgroup(page);
> > + if (unlikely(!pc))
> > + return;
> > +
> > + pc->bio_cgroup_id = 0; /* 0: default bio_cgroup id */
> > + if (!mm)
> > + return;
> > + /*
> > + * Locking "pc" isn't necessary here since the current process is
> > + * the only one that can access the members related to bio_cgroup.
> > + */
> > + rcu_read_lock();
> > + biog = bio_cgroup_from_task(rcu_dereference(mm->owner));
> > + if (unlikely(!biog))
> > + goto out;
> > + /*
> > + * css_get(&bio->css) isn't called to increment the reference
> > + * count of this bio_cgroup "biog" so pc->bio_cgroup_id might turn
> > + * invalid even if this page is still active.
> > + * This approach is chosen to minimize the overhead.
> > + */
> > + pc->bio_cgroup_id = biog->id;
> > +out:
> > + rcu_read_unlock();
> > +}
> > +
> > +/*
> > + * Change the owner of a given page if necessary.
> > + */
> > +void bio_cgroup_reset_owner(struct page *page, struct mm_struct *mm)
> > +{
> > + /*
> > + * A little trick:
> > + * Just call bio_cgroup_set_owner() for pages which are already
> > + * active since the bio_cgroup_id member of page_cgroup can be
> > + * updated without any locks. This is because an integer type of
> > + * variable can be set a new value at once on modern cpus.
> > + */
> > + bio_cgroup_set_owner(page, mm);
> > +}
> Hmm ? I think all operations are under lock_page() and there are no races.
> Isn't it ?
ehm.. no. Adding this in bio_cgroup_set_owner():
WARN_ON_ONCE(!test_bit(PG_locked, &page->flags));
produces the following:
[ 1.641186] WARNING: at mm/biotrack.c:77 bio_cgroup_set_owner+0xe2/0x100()
[ 1.644534] Hardware name:
[ 1.646955] Modules linked in:
[ 1.650526] Pid: 1, comm: swapper Not tainted 2.6.30-rc2 #77
[ 1.653499] Call Trace:
[ 1.656004] [<ffffffff80269370>] warn_slowpath+0xd0/0x120
[ 1.659062] [<ffffffff8023d69a>] ? save_stack_trace+0x2a/0x50
[ 1.662357] [<ffffffff80291f7f>] ? save_trace+0x3f/0xb0
[ 1.670214] [<ffffffff802e3abd>] ? handle_mm_fault+0x40d/0x8b0
[ 1.673321] [<ffffffff8029586b>] ? __lock_acquire+0x63b/0x1de0
[ 1.676446] [<ffffffff802921ba>] ? get_lock_stats+0x2a/0x60
[ 1.679657] [<ffffffff802921fe>] ? put_lock_stats+0xe/0x30
[ 1.682673] [<ffffffff802e3abd>] ? handle_mm_fault+0x40d/0x8b0
[ 1.685706] [<ffffffff80300e72>] bio_cgroup_set_owner+0xe2/0x100
[ 1.688852] [<ffffffff802e3abd>] ? handle_mm_fault+0x40d/0x8b0
[ 1.692280] [<ffffffff802e3ae2>] handle_mm_fault+0x432/0x8b0
[ 1.695261] [<ffffffff802e408f>] __get_user_pages+0x12f/0x430
[ 1.703507] [<ffffffff802e43c2>] get_user_pages+0x32/0x40
[ 1.706947] [<ffffffff80308bab>] get_arg_page+0x4b/0xb0
[ 1.710287] [<ffffffff80308e3d>] copy_strings+0xfd/0x200
[ 1.714028] [<ffffffff80308f69>] copy_strings_kernel+0x29/0x40
[ 1.717058] [<ffffffff8030a651>] do_execve+0x2c1/0x400
[ 1.720291] [<ffffffff8022d739>] sys_execve+0x49/0x80
[ 1.723209] [<ffffffff802300b8>] kernel_execve+0x68/0xd0
[ 1.726309] [<ffffffff8020930b>] ? init_post+0x18b/0x1b0
[ 1.729585] [<ffffffff80af069b>] kernel_init+0x198/0x1b0
[ 1.735754] [<ffffffff8023003a>] child_rip+0xa/0x20
[ 1.738690] [<ffffffff8022fa00>] ? restore_args+0x0/0x30
[ 1.741663] [<ffffffff80af0503>] ? kernel_init+0x0/0x1b0
[ 1.744683] [<ffffffff80230030>] ? child_rip+0x0/0x20
[ 1.747820] ---[ end trace b9f530261e455c85 ]---
In do_anonymous_page(), bio_cgroup_set_owner() seems to be called
without lock_page() held.
-Andrea
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
