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)
    * Paul Menage <menage@google.com> [2008-05-15 00:39:45]:

    > On Thu, May 15, 2008 at 12:03 AM, Balbir Singh
    > <balbir@linux.vnet.ibm.com> wrote:
    > >
    > > I want to focus on this conclusion/assertion, since it takes care of
    > > most of the locking related discussion above, unless I missed
    > > something.
    > >
    > > My concern with using mmap_sem, is that
    > >
    > > 1. It's highly contended (every page fault, vma change, etc)
    >
    > But the only *new* cases of taking the mmap_sem that this would
    > introduce would be:
    >
    > - on a failed vm limit charge

    Why a failed charge? Aren't we talking of moving all charge/uncharge
    under mmap_sem?

    > - when a task exit/exec causes an mm ownership change

    Yes, in the mm_owner_changed callbacks

    > - when a task moves between two cgroups in the memrlimit hierarchy.
    >

    Yes, this would nest cgroup_mutex and mmap_sem. Not sure if that would
    be a bad side-effect.

    > All of these should be rare events, so I don't think the additional
    > contention is a worry.

    We do make several of all charge calls under the mmap_sem, but not
    all of them. So the additional contention might not be all that bad.

    >
    > > 2. It's going to make the locking hierarchy deeper and complex
    >
    > Yes, potentially. But if the upside of that is that we eliminate a
    > lock/unlock on a shared lock on every mmap/munmap call, it might well
    > be worth it.
    >
    > > 3. It's not appropriate to call all the accounting callbacks with
    > > the mmap_sem() held, since the undo operations _can get_ complicated
    > > at the caller.
    > >
    >
    > Can you give an example?

    Some paths of the uncharge are not under mmap_sem. Undoing the
    operation there seemed complex.

    >
    > > I would prefer introducing a new lock, so that other subsystems are
    > > not affected.
    > >
    >
    > For getting the first cut of the memrlimit controller working this may
    > well make sense. But it would be nice to avoid it longer-term.

    OK, so here's what I am going to try and do

    Refactor the code to try and use mmap_sem and see what I come up
    with. Basically use mmap_sem for all charge/uncharge operations as
    well use mmap_sem in read_mode in the move_task() and
    mm_owner_changed() callbacks. That should take care of the race
    conditions discussed, unless I missed something.
    Try and instrument insert_vm_struct() for charge/uncharge

    --
    Warm Regards,
    Balbir Singh
    Linux Technology Center
    IBM, ISTL


    \
     
     \ /
      Last update: 2008-05-15 10:29    [from the cache]
    ©2003-2014 Jasper Spaans. hosted at Digital Ocean