lkml.org 
[lkml]   [2008]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [-mm][PATCH 4/4] Add memrlimit controller accounting and control (v4)
On Wed, May 14, 2008 at 6:25 AM, Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> +
> +int memrlimit_cgroup_charge_as(struct mm_struct *mm, unsigned long nr_pages)
> +{
> + int ret;
> + struct memrlimit_cgroup *memrcg;
> +
> + rcu_read_lock();
> + memrcg = memrlimit_cgroup_from_task(rcu_dereference(mm->owner));
> + css_get(&memrcg->css);
> + rcu_read_unlock();
> +
> + ret = res_counter_charge(&memrcg->as_res, (nr_pages << PAGE_SHIFT));
> + css_put(&memrcg->css);
> + return ret;
> +}

Assuming that we're holding a write lock on mm->mmap_sem here, and we
additionally hold mmap_sem for the whole of mm_update_next_owner(),
then maybe we don't need any extra synchronization here? Something
like simply:

int memrlimit_cgroup_charge_as(struct mm_struct *mm, unsigned long nr_pages)
{
struct memrlimit_cgroup *memrcg = memrlimit_cgroup_from_task(mm->owner);
return res_counter_charge(&memrcg->as_res, (nr_pages << PAGE_SHIFT));
}
Seems good to minimize additional synchronization on the fast path.

The only thing that's still broken is that the task_struct.cgroups
pointer gets updated only under the synchronization of task_lock(), so
we've still got the race of:

A: attach_task() updates B->cgroups

B: memrlimit_cgroup_charge_as() charges the new res counter and
updates mm->total_vm

A: memrlimit_cgroup_move_task() moves mm->total_vm from the old
counter to the new counter

Here's one way I see to fix this:

We change attach_task() so that rather than updating the
task_struct.cgroups pointer once from the original css_set to the
final css_set, it goes through a series of intermediate css_set
structures, one for each subsystem in the hierarchy, transitioning
from the old set to the final set. Then for each subsystem ss, it
would do:

next_css = <old css with pointer for ss updated>
if (ss->attach) {
ss->attach(ss, p, next_css);
} else {
task_lock(p);
rcu_assign_ptr(p->cgroups, next_css);
task_unlock(p);
}
i.e. the subsystem would be free to implement any synchronization it
desired in the attach() code. The attach() method's responsibility
would be to ensure that p->cgroups was updated to point to next_css
before returning. This should make it much simpler for a subsystem to
handle potential races between attach() and accounting. The current
semantics of can_attach()/update/attach() are sufficient for cpusets,
but probably not for systems with more complex accounting. I'd still
need to figure out a nice way to get the kind of transactional
semantics that you want from can_attach().

> +
> +void memrlimit_cgroup_uncharge_as(struct mm_struct *mm, unsigned long nr_pages)
> +{
> + struct memrlimit_cgroup *memrcg;
> +
> + rcu_read_lock();
> + memrcg = memrlimit_cgroup_from_task(rcu_dereference(mm->owner));
> + css_get(&memrcg->css);
> + rcu_read_unlock();
> +
> + res_counter_uncharge(&memrcg->as_res, (nr_pages << PAGE_SHIFT));
> + css_put(&memrcg->css);
> +}
> +
> static struct cgroup_subsys_state *
> memrlimit_cgroup_create(struct cgroup_subsys *ss, struct cgroup *cgrp)
> {
> @@ -134,11 +169,70 @@ static int memrlimit_cgroup_populate(str
> ARRAY_SIZE(memrlimit_cgroup_files));
> }
>
> +static void memrlimit_cgroup_move_task(struct cgroup_subsys *ss,
> + struct cgroup *cgrp,
> + struct cgroup *old_cgrp,
> + struct task_struct *p)
> +{
> + struct mm_struct *mm;
> + struct memrlimit_cgroup *memrcg, *old_memrcg;
> +
> + mm = get_task_mm(p);
> + if (mm == NULL)
> + return;
> +
> + rcu_read_lock();
> + if (p != rcu_dereference(mm->owner))
> + goto out;

out: does up_read() on mmap_sem, which you don't currently hold.

Can you add more comments about why you're using RCU here?

You have a refcounted pointer on mm, so mm can't go away, and you
never dereference the result of the rcu_dereference(mm->owner), so
you're not protecting the validity of that pointer. The only way this
could help would be if anything that changes mm->owner calls
synchronize_rcu() before, say, doing accounting changes, and I don't
believe that's the case.

Would it be simpler to use task_lock(p) here to ensure that it doesn't
lose its owner status (by exiting or execing) while we're moving it?

i.e., something like: [ this assumes the new semantics of attach that
I proposed above ]


static void memrlimit_cgroup_move_task(struct cgroup_subsys *ss,
struct cgroup *cgrp,
struct cgroup *old_cgrp,
struct task_struct *p,
struct css_set new_css_set)
{
struct mm_struct *mm;
struct memrlimit_cgroup *memrcg, *old_memrcg;
retry:
mm = get_task_mm(p);
if (mm == NULL) {
task_lock(p);
rcu_assign_ptr(p->cgroups, new_css_set);
task_unlock(p);
return;
}
/* Take mmap_sem to prevent address space changes */
down_read(&mm->mmap_sem);
/* task_lock(p) to prevent mm ownership changes */
task_lock(p);
if (p->mm != mm) {
/* We raced */
task_unlock(p);
up_read(&mm->mmap_sem);
mmput(mm);
goto retry;
}
if (p != mm->owner)
goto out_assign;
memrcg = memrlimit_cgroup_from_cgrp(cgrp);
old_memrcg = memrlimit_cgroup_from_cgrp(old_cgrp);

if (memrcg == old_memrcg)
goto out_assign;
if (res_counter_charge(&memrcg->as_res, (mm->total_vm << PAGE_SHIFT)))
goto out_assign;
res_counter_uncharge(&old_memrcg->as_res, (mm->total_vm << PAGE_SHIFT));
out_assign:
rcu_assign_ptr(p->cgroups, new_css_set);
task_unlock(p);
up_read(&mm->mmap_sem);
mmput(mm);
}



> +
> + memrcg = memrlimit_cgroup_from_cgrp(cgrp);
> + old_memrcg = memrlimit_cgroup_from_cgrp(old_cgrp);
> +
> + if (memrcg == old_memrcg)
> + goto out;

mmap_sem is also not held here.

> +
> + /*
> + * Hold mmap_sem, so that total_vm does not change underneath us
> + */
> + down_read(&mm->mmap_sem);

You can't block inside rcu_read_lock().

> + if (res_counter_charge(&memrcg->as_res, (mm->total_vm << PAGE_SHIFT)))
> + goto out;
> + res_counter_uncharge(&old_memrcg->as_res, (mm->total_vm << PAGE_SHIFT));
> +out:
> + up_read(&mm->mmap_sem);
> + rcu_read_unlock();
> + mmput(mm);
> +}
> +
> +static void memrlimit_cgroup_mm_owner_changed(struct cgroup_subsys *ss,
> + struct cgroup *cgrp,
> + struct cgroup *old_cgrp,
> + struct task_struct *p)
> +{
> + struct memrlimit_cgroup *memrcg, *old_memrcg;
> + struct mm_struct *mm = get_task_mm(p);
> +
> + BUG_ON(!mm);
> + memrcg = memrlimit_cgroup_from_cgrp(cgrp);
> + old_memrcg = memrlimit_cgroup_from_cgrp(old_cgrp);
> +
> + down_read(&mm->mmap_sem);

At this point we're holding p->alloc_lock, so we can't do a blocking
down_read().

How about if we down_read(&mm->mmap_sem) in mm_update_next_owner()
prior to taking tasklist_lock? That will ensure that mm ownership
changes are synchronized against mmap/munmap operations on that mm,
and since the cgroups-based owners are likely to be wanting to track
the ownership changes for accounting purposes, this seems like an
appropriate lock to hold.

> + if (res_counter_charge(&memrcg->as_res, (mm->total_vm << PAGE_SHIFT)))
> + goto out;
> + res_counter_uncharge(&old_memrcg->as_res, (mm->total_vm << PAGE_SHIFT));
> +out:
> + up_read(&mm->mmap_sem);
> +
> + mmput(mm);
> +}
> +


\
 
 \ /
  Last update: 2008-05-15 04:27    [from the cache]
©2003-2011 Jasper Spaans