lkml.org 
[lkml]   [2010]   [Aug]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] cgroup_freezer: Freezing and task move race fix
    On Wed, Aug 11, 2010 at 12:18:44AM +0200, Tomasz Buchert wrote:
    > Matt Helsley a écrit :
    > > On Tue, Aug 10, 2010 at 09:53:21PM +0200, Tomasz Buchert wrote:
    > >> Writing 'FROZEN' to freezer.state file does not
    > >> forbid the task to be moved away from its cgroup
    > >> (for a very short time). Nevertheless the moved task
    > >> can become frozen OUTSIDE its cgroup which puts
    > >> discussed task in a permanent 'D' state.
    > >>
    > >> This patch forbids migration of either FROZEN
    > >> or FREEZING tasks.
    > >>
    > >> This behavior was observed and easily reproduced on
    > >> a single core laptop. Program and instructions how
    > >> to reproduce the bug can be fetched from:
    > >> http://pentium.hopto.org/~thinred/repos/linux-misc/freezer_bug.c
    > >
    > > Thanks for the report and the test code.
    > >
    > > I'm will try to reproduce this race in the next few hours and analyze
    > > it since I'm not sure the patch really fixes the race -- it may only
    > > make the race trigger less frequently.
    > >
    > > At the very least the patch won't break the current code since it's
    > > essentially a more-strict version of is_task_frozen_enough() -- it lets
    > > fewer tasks attach/detach to/from frozen cgroups.
    > >
    > > Cheers,
    > > -Matt Helsley
    >
    > Hi Matt!
    > I am a novice if it comes to the kernel and I find the cgroup_freezer
    > code especially complicated, so definetely this may be not enough to fix that.
    > Notice also that if you uncomment the line 55 in my testcase this will also
    > trigger the race! This, however, makes sense since process may not be in the cgroup anymore
    > and consequently won't be thawed.

    OK, I triggered it with that. Interesting.

    > I think that this patch fixes these problems because it does the flag checking in a right order:
    > first freezing() is used and then frozen() which assures (see frozen_process()) that
    > the race will not happen. Right? :)

    I see what you mean. It still seems like it wouldn't actually fix the race -- just make it
    harder to trigger. I think you're saying this is what happens without the patch:

    Time "bug" goes through these states cgroup code checks for these states
    -----------------------------------------------------------------------------------
    | freezing
    | is_frozen? Nope.
    | frozen
    | is_freezing? Nope.
    | <move>
    V

    But, without having carefully investigated the details, this could just as easily happen
    with your patch:

    Time "bug" goes through these states cgroup code checks for these states
    -----------------------------------------------------------------------------------
    | is_freezing? Nope.
    | is_frozen? Nope.
    | freezing
    | <move>
    | frozen
    V

    or:

    Time "bug" goes through these states cgroup code checks for these states
    -----------------------------------------------------------------------------------
    | is_freezing? Nope.
    | is_frozen? Nope.
    | freezing
    | frozen
    | <move>
    V

    Time "bug" goes through these states cgroup code checks for these states
    -----------------------------------------------------------------------------------
    | is_freezing? Nope.
    | freezing
    | is_frozen? Nope.
    | <move>
    | frozen
    V

    or:

    Time "bug" goes through these states cgroup code checks for these states
    -----------------------------------------------------------------------------------
    | is_freezing? Nope.
    | freezing
    | is_frozen? Nope.
    | frozen
    | <move>
    V

    (even with 1 cpu/core)

    Your patch only improves things in the sense that it works for the first
    example. We need to prevent the latter cases as well.

    Cheers,
    -Matt
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2010-08-11 06:31    [W:0.028 / U:149.064 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site