Re: [PATCH 2/4] remove boost_dying_task_prio()

Re: [PATCH 2/4] remove boost_dying_task_prio()

[Andrew Morton]

On Mon, 11 Apr 2011 14:31:18 +0900 (JST)
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> This is a almost revert commit 93b43fa (oom: give the dying
> task a higher priority).
> 
> 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.

hm.  How did that happen?  I thought that sched_setscheduler() modifies
only a single thread, and that thread is in the process of exiting?

> Eventually, kernel may hang up when oom kill occur.
> I and Luis who original author agreed to disable this logic at
> once.
> 
> ...
>
> index 6a819d1..83fb72c1 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);
> -}

I'm rather glad to see that code go away though - SCHED_FIFO is
dangerous...

Re: [PATCH 2/4] remove boost_dying_task_prio()

[KOSAKI Motohiro]

Hi

> On Mon, 11 Apr 2011 14:31:18 +0900 (JST)
> KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
> 
> > This is a almost revert commit 93b43fa (oom: give the dying
> > task a higher priority).
> > 
> > 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.
> 
> hm.  How did that happen?  I thought that sched_setscheduler() modifies
> only a single thread, and that thread is in the process of exiting?

If admin insert !RT process into a cpu cgroup of setting rtruntime=0,
usually it run perfectly because !RT task isn't affected from rtruntime
knob, but If it promote RT task, by explicit setscheduler() syscall or
OOM, the task can't run at all.

In short, now oom killer don't work at all if admin are using cpu
cgroup and don't touch rtruntime knob.

Re: [PATCH 2/4] remove boost_dying_task_prio()

[David Rientjes]

On Mon, 11 Apr 2011, KOSAKI Motohiro wrote:

> This is a almost revert commit 93b43fa (oom: give the dying
> task a higher priority).
> 
> 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.
> I and Luis who original author agreed to disable this logic at
> once.
> 
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Acked-by: Luis Claudio R. Goncalves <lclaudio@uudg.org>
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH 1/4] vmscan: all_unreclaimable() use zone->all_unreclaimable as a name

[Andrew Morton]

On Mon, 11 Apr 2011 14:30:31 +0900 (JST)
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> all_unreclaimable check in direct reclaim has been introduced at 2.6.19
> by following commit.
> 
> 	2006 Sep 25; commit 408d8544; oom: use unreclaimable info
> 
> And it went through strange history. firstly, following commit broke
> the logic unintentionally.
> 
> 	2008 Apr 29; commit a41f24ea; page allocator: smarter retry of
> 				      costly-order allocations
> 
> Two years later, I've found obvious meaningless code fragment and
> restored original intention by following commit.
> 
> 	2010 Jun 04; commit bb21c7ce; vmscan: fix do_try_to_free_pages()
> 				      return value when priority==0
> 
> But, the logic didn't works when 32bit highmem system goes hibernation
> and Minchan slightly changed the algorithm and fixed it .
> 
> 	2010 Sep 22: commit d1908362: vmscan: check all_unreclaimable
> 				      in direct reclaim path
> 
> But, recently, Andrey Vagin found the new corner case. Look,
> 
> 	struct zone {
> 	  ..
> 	        int                     all_unreclaimable;
> 	  ..
> 	        unsigned long           pages_scanned;
> 	  ..
> 	}
> 
> zone->all_unreclaimable and zone->pages_scanned are neigher atomic
> variables nor protected by lock. Therefore zones can become a state
> of zone->page_scanned=0 and zone->all_unreclaimable=1. In this case,
> current all_unreclaimable() return false even though
> zone->all_unreclaimabe=1.
> 
> Is this ignorable minor issue? No. Unfortunatelly, x86 has very
> small dma zone and it become zone->all_unreclamble=1 easily. and
> if it become all_unreclaimable=1, it never restore all_unreclaimable=0.
> Why? if all_unreclaimable=1, vmscan only try DEF_PRIORITY reclaim and
> a-few-lru-pages>>DEF_PRIORITY always makes 0. that mean no page scan
> at all!
> 
> Eventually, oom-killer never works on such systems. That said, we
> can't use zone->pages_scanned for this purpose. This patch restore
> all_unreclaimable() use zone->all_unreclaimable as old. and in addition,
> to add oom_killer_disabled check to avoid reintroduce the issue of
> commit d1908362.

The above is a nice analysis of the bug and how it came to be
introduced.  But we don't actually have a bug description!  What was
the observeable problem which got fixed?

Such a description will help people understand the importance of the
patch and will help people (eg, distros) who are looking at a user's
bug report and wondering whether your patch will fix it.

Re: [PATCH 1/4] vmscan: all_unreclaimable() use zone->all_unreclaimable as a name

[KOSAKI Motohiro]

Hi

> > zone->all_unreclaimable and zone->pages_scanned are neigher atomic
> > variables nor protected by lock. Therefore zones can become a state
> > of zone->page_scanned=0 and zone->all_unreclaimable=1. In this case,
> > current all_unreclaimable() return false even though
> > zone->all_unreclaimabe=1.
> > 
> > Is this ignorable minor issue? No. Unfortunatelly, x86 has very
> > small dma zone and it become zone->all_unreclamble=1 easily. and
> > if it become all_unreclaimable=1, it never restore all_unreclaimable=0.
> > Why? if all_unreclaimable=1, vmscan only try DEF_PRIORITY reclaim and
> > a-few-lru-pages>>DEF_PRIORITY always makes 0. that mean no page scan
> > at all!
> > 
> > Eventually, oom-killer never works on such systems. That said, we
> > can't use zone->pages_scanned for this purpose. This patch restore
> > all_unreclaimable() use zone->all_unreclaimable as old. and in addition,
> > to add oom_killer_disabled check to avoid reintroduce the issue of
> > commit d1908362.
> 
> The above is a nice analysis of the bug and how it came to be
> introduced.  But we don't actually have a bug description!  What was
> the observeable problem which got fixed?

The above says "Eventually, oom-killer never works". Is this no enough?
The above says
  1) current logic have a race
  2) x86 increase a chance of the race by dma zone
  3) if race is happen, oom killer don't work

> 
> Such a description will help people understand the importance of the
> patch and will help people (eg, distros) who are looking at a user's
> bug report and wondering whether your patch will fix it.
> 

Re: [PATCH 1/4] vmscan: all_unreclaimable() use zone->all_unreclaimable as a name

[Andrew Morton]

On Tue, 12 Apr 2011 10:04:15 +0900 (JST) KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> Hi
> 
> > > zone->all_unreclaimable and zone->pages_scanned are neigher atomic
> > > variables nor protected by lock. Therefore zones can become a state
> > > of zone->page_scanned=0 and zone->all_unreclaimable=1. In this case,
> > > current all_unreclaimable() return false even though
> > > zone->all_unreclaimabe=1.
> > > 
> > > Is this ignorable minor issue? No. Unfortunatelly, x86 has very
> > > small dma zone and it become zone->all_unreclamble=1 easily. and
> > > if it become all_unreclaimable=1, it never restore all_unreclaimable=0.
> > > Why? if all_unreclaimable=1, vmscan only try DEF_PRIORITY reclaim and
> > > a-few-lru-pages>>DEF_PRIORITY always makes 0. that mean no page scan
> > > at all!
> > > 
> > > Eventually, oom-killer never works on such systems. That said, we
> > > can't use zone->pages_scanned for this purpose. This patch restore
> > > all_unreclaimable() use zone->all_unreclaimable as old. and in addition,
> > > to add oom_killer_disabled check to avoid reintroduce the issue of
> > > commit d1908362.
> > 
> > The above is a nice analysis of the bug and how it came to be
> > introduced.  But we don't actually have a bug description!  What was
> > the observeable problem which got fixed?
> 
> The above says "Eventually, oom-killer never works". Is this no enough?
> The above says
>   1) current logic have a race
>   2) x86 increase a chance of the race by dma zone
>   3) if race is happen, oom killer don't work

And the system hangs up, so it's a local DoS and I guess we should
backport the fix into -stable.  I added this:

: This resulted in the kernel hanging up when executing a loop of the form
: 
: 1. fork
: 2. mmap
: 3. touch memory
: 4. read memory
: 5. munmmap
: 
: as described in
: http://www.gossamer-threads.com/lists/linux/kernel/1348725#1348725

And the problems which the other patches in this series address are
pretty deadly as well.  Should we backport everything?

Re: [PATCH 1/4] vmscan: all_unreclaimable() use zone->all_unreclaimable as a name

[KOSAKI Motohiro]

Hi

> > The above says "Eventually, oom-killer never works". Is this no enough?
> > The above says
> >   1) current logic have a race
> >   2) x86 increase a chance of the race by dma zone
> >   3) if race is happen, oom killer don't work
> 
> And the system hangs up, so it's a local DoS and I guess we should
> backport the fix into -stable.  I added this:
> 
> : This resulted in the kernel hanging up when executing a loop of the form
> : 
> : 1. fork
> : 2. mmap
> : 3. touch memory
> : 4. read memory
> : 5. munmmap
> : 
> : as described in
> : http://www.gossamer-threads.com/lists/linux/kernel/1348725#1348725
> 
> And the problems which the other patches in this series address are
> pretty deadly as well.  Should we backport everything?

patch [1/4] and [2/4] should be backported because they are regression fix.
But [3/4] and [4/4] are on borderline to me. they improve a recovery time 
from oom. some times it is very important, some times not. And it is not
regression fix. Our oom-killer is very weak from forkbomb attack since
very old days.

Thanks.

Re: [PATCH 1/4] vmscan: all_unreclaimable() use zone->all_unreclaimable as a name

[David Rientjes]

On Mon, 11 Apr 2011, KOSAKI Motohiro wrote:

> all_unreclaimable check in direct reclaim has been introduced at 2.6.19
> by following commit.
> 
> 	2006 Sep 25; commit 408d8544; oom: use unreclaimable info
> 
> And it went through strange history. firstly, following commit broke
> the logic unintentionally.
> 
> 	2008 Apr 29; commit a41f24ea; page allocator: smarter retry of
> 				      costly-order allocations
> 
> Two years later, I've found obvious meaningless code fragment and
> restored original intention by following commit.
> 
> 	2010 Jun 04; commit bb21c7ce; vmscan: fix do_try_to_free_pages()
> 				      return value when priority==0
> 
> But, the logic didn't works when 32bit highmem system goes hibernation
> and Minchan slightly changed the algorithm and fixed it .
> 
> 	2010 Sep 22: commit d1908362: vmscan: check all_unreclaimable
> 				      in direct reclaim path
> 
> But, recently, Andrey Vagin found the new corner case. Look,
> 
> 	struct zone {
> 	  ..
> 	        int                     all_unreclaimable;
> 	  ..
> 	        unsigned long           pages_scanned;
> 	  ..
> 	}
> 
> zone->all_unreclaimable and zone->pages_scanned are neigher atomic
> variables nor protected by lock. Therefore zones can become a state
> of zone->page_scanned=0 and zone->all_unreclaimable=1. In this case,
> current all_unreclaimable() return false even though
> zone->all_unreclaimabe=1.
> 
> Is this ignorable minor issue? No. Unfortunatelly, x86 has very
> small dma zone and it become zone->all_unreclamble=1 easily. and
> if it become all_unreclaimable=1, it never restore all_unreclaimable=0.
> Why? if all_unreclaimable=1, vmscan only try DEF_PRIORITY reclaim and
> a-few-lru-pages>>DEF_PRIORITY always makes 0. that mean no page scan
> at all!
> 
> Eventually, oom-killer never works on such systems. That said, we
> can't use zone->pages_scanned for this purpose. This patch restore
> all_unreclaimable() use zone->all_unreclaimable as old. and in addition,
> to add oom_killer_disabled check to avoid reintroduce the issue of
> commit d1908362.
> 
> Reported-by: Andrey Vagin <avagin@openvz.org>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Nick Piggin <npiggin@kernel.dk>
> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Acked-by: David Rientjes <rientjes@google.com>

Seems like it should be a candidate for stable inclusion as well, nice 
catch.