lkml.org 
[lkml]   [2008]   [May]   [15]   [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 11:17 PM, Balbir Singh
<balbir@linux.vnet.ibm.com> wrote:
> >
> > 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));
> > }
>
> The charge_as routine is not always called with mmap_sem held, since
> the undo path gets more complicated under the lock. We already have
> our own locking mechanism for the counters.

I'm not worried about the counters themselves being inconsistent - I'm
worried about the case where charge_as() is called in the middle of
the attach operation, and we account the charge X to the new cgroup's
res_counter and update mm->total_vm, and then when we do the move, we
charge the whole of mm->total_mm to the new cgroup even though the
last charge was already accounted to the new res_counter, not the old
one.

That's what I'm hoping to address with the idea of splitting the
attach into one update per subsystem, and letting the subsystems
control their own synchronization.

> We're not really accessing
> any member of the mm here except the owner. Do we need to be called
> with mmap_sem held?
>

Not necessarily mmap_sem, but there needs to be something to ensure
that the update to mm->total_vm and the charge/uncharge against the
res_counter are an atomic pair with respect to the code that shifts an
mm between two cgroups, either due to mm->owner change or due to an
attach_task(). Since mmap_sem is held for write on almost all the fast
path calls to the rlimit_as charge/uncharge functions, using that for
the synchronization avoids the need for any additional synchronization
in the fast path.

Can you say more about the complications of holding a write lock on
mmap_sem in the cleanup calls to uncharge?

> > retry:
> > mm = get_task_mm(p);
> > if (mm == NULL) {
> > task_lock(p);
> > rcu_assign_ptr(p->cgroups, new_css_set);
>
> Will each callback assign p->cgroups to new_css_set?

Yes - but new_css_set will be slightly different for each callback.
Specifically, it will differ from the existing set pointed to by
p->cgroups in the pointer for this particular subsystem. So the task
will move over in a staggered fashion, and each subsystem will get to
choose its own synchronization.

> > task_lock(p);
> > if (p->mm != mm) {
> > /* We raced */
>
> With exit_mmap() or exec_mmap() right?
>

Yes.

> If we agree with the assertion/conclusion above, then a simple lock
> might be able to protect us, assuming that it does not create a
> interwined locking hierarchy.
>

Right - and if we can make that lock be the mmap_sem of the mm in
question, we avoid introducing a new lock into the fast path.

Paul


\
 
 \ /
  Last update: 2008-05-15 08:57    [W:0.056 / U:0.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site