lkml.org 
[lkml]   [2011]   [Mar]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 2/5] Revert "oom: give the dying task a higher priority"
    On Tue, Mar 22, 2011 at 08:06:48PM +0900, KOSAKI Motohiro wrote:
    > This reverts commit 93b43fa55088fe977503a156d1097cc2055449a2.
    >
    > The commit dramatically improve oom killer logic when fork-bomb
    > occur. But, I've found it has nasty corner case. Now cpu cgroup
    > has strange default RT runtime. It's 0! That said, if a process
    > under cpu cgroup promote RT scheduling class, the process never
    > run at all.
    >
    > Eventually, kernel may hang up when oom kill occur.
    >
    > The author need to resubmit it as adding knob and disabled
    > by default if he really need this feature.
    >
    > Cc: Luis Claudio R. Goncalves <lclaudio@uudg.org>
    > Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
    Reviewed-by: Minchan Kim <minchan.kim@gmail.com>

    Just a comment below.

    > ---
    > mm/oom_kill.c | 27 ---------------------------
    > 1 files changed, 0 insertions(+), 27 deletions(-)
    >
    > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
    > index 3100bc5..739dee4 100644
    > --- a/mm/oom_kill.c
    > +++ b/mm/oom_kill.c
    > @@ -84,24 +84,6 @@ static bool has_intersects_mems_allowed(struct task_struct *tsk,
    > #endif /* CONFIG_NUMA */
    >
    > /*
    > - * If this is a system OOM (not a memcg OOM) and the task selected to be
    > - * killed is not already running at high (RT) priorities, speed up the
    > - * recovery by boosting the dying task to the lowest FIFO priority.
    > - * That helps with the recovery and avoids interfering with RT tasks.
    > - */
    > -static void boost_dying_task_prio(struct task_struct *p,
    > - struct mem_cgroup *mem)
    > -{
    > - struct sched_param param = { .sched_priority = 1 };
    > -
    > - if (mem)
    > - return;
    > -
    > - if (!rt_task(p))
    > - sched_setscheduler_nocheck(p, SCHED_FIFO, &param);
    > -}
    > -
    > -/*
    > * The process p may have detached its own ->mm while exiting or through
    > * use_mm(), but one or more of its subthreads may still have a valid
    > * pointer. Return p, or any of its subthreads with a valid ->mm, with
    > @@ -452,13 +434,6 @@ static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
    > set_tsk_thread_flag(p, TIF_MEMDIE);
    > force_sig(SIGKILL, p);
    >
    > - /*
    > - * We give our sacrificial lamb high priority and access to
    > - * all the memory it needs. That way it should be able to
    > - * exit() and clear out its resources quickly...
    > - */
    > - boost_dying_task_prio(p, mem);
    > -

    Before merging 93b43fa5508, we had a following routine.

    +static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
    {
    p = find_lock_task_mm(p);
    if (!p) {
    @@ -434,9 +452,17 @@ static int oom_kill_task(struct task_struct *p)
    K(get_mm_counter(p->mm, MM_FILEPAGES)));
    task_unlock(p);

    - p->rt.time_slice = HZ; <<---- THIS
    +
    set_tsk_thread_flag(p, TIF_MEMDIE);
    force_sig(SIGKILL, p);
    +
    + /*
    + * We give our sacrificial lamb high priority and access to
    + * all the memory it needs. That way it should be able to
    + * exit() and clear out its resources quickly...
    + */
    + boost_dying_task_prio(p, mem);
    +
    return 0;
    }

    At that time, I thought that routine is meaningless in non-RT scheduler.
    So I Cced Peter but don't get the answer.
    I just want to confirm it.
    Do you still think it's meaningless?
    so you remove it when you revert 93b43fa5508?
    Then, this isn't just revert patch but revert + killing meaningless code patch.


    -
    Kind regards,
    Minchan Kim


    \
     
     \ /
      Last update: 2011-03-24 16:31    [W:0.029 / U:0.248 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site