Re: [PATCH] fix spurious OOM kills

Marcelo Tosatti
2004-11-11T17:43:02Z

On Thu, Nov 11, 2004 at 05:50:51PM +0100, Andrea Arcangeli wrote:
> On Thu, Nov 11, 2004 at 10:38:50AM -0200, Marcelo Tosatti wrote:
> >
> > Hi!
> >
> > On Thu, Nov 11, 2004 at 04:42:38PM +0100, Andrea Arcangeli wrote:
> > > On Thu, Nov 11, 2004 at 09:29:22AM -0200, Marcelo Tosatti wrote:
> > > > Hi,
> > > >
> > > > This is an improved version of OOM-kill-from-kswapd patch.
> > > >
> > > > I believe triggering the OOM killer from task reclaim context
> > > > is broken because the chances that it happens increases as the amount
> > > > of tasks inside reclaim increases - and that approach ignores efforts
> > > > being done by kswapd, who is the main entity responsible for
> > > > freeing pages.
> > > >
> > > > There have been a few problems pointed out by others (Andrea, Nick) on the
> > > > last patch - this one solves them.
> > >
> > > I disagree about the design of killing anything from kswapd. kswapd is
> > > an async helper like pdflush and it has no knowledge on the caller (it
> > > cannot know if the caller is ok with the memory currently available in
> > > the freelists, before triggering the oom).
> >
> > If zone_dma / zone_normal are below pages_min no caller is "OK with
> > memory currently available" except GFP_ATOMIC/realtime callers.
>
> If the GFP_DMA zone is filled, and nobody allocates with GFP_DMA,
> nothing should be killed and everything should run fine, how can you
> get this right from kswapd?

It does get it right. It only triggers OOM killer if _both_
GFP_DMA / GFP_KERNEL are full _and_ there is a task failing
to allocate/free memory.

I think you missed the "task_looping_oom" variable in the patch, which is
set as soon as a task is unable to allocate/free memory. This variable
is set where the code used to call the OOM killer.

> > > I'm just about to move the
> > > oom killing away from vmscan.c to page_alloc.c which is basically the
> > > opposite of moving the oom invocation from the task context to kswapd.
> > > page_alloc.c in the task context is the only one who can know if
> > > something has to be killed, vmscan.c cannot know. vmscan.c can only know
> > > if something is still freeable, but if something isn't freeable it
> > > doesn't mean that we've to kill anything
> >
> > Well Andrea, its not about "if something isnt freeable", its about
> > "the VM is unable to make progress reclaiming pages".
>
> "VM is unable to reclaim pages" == "nothing is freeable"

OK, correct, silly me. I noted the gaffe after sending the email.

But still, the main idea is valid here.

I'll say this again just in case: If ZONE_DMA and ZONE_NORMAL reclaiming
efforts are in vain, and there is task which is looping on try_to_free_pages()
unable to succeed, _and_ both DMA/normal are below pages_min, the OOM
killer will be triggered.

(should be pages_min + higher protection).

> > > (for example if a task exited
> > > or some dma or normal-zone or highmem memory was released by another
> > > task while we were paging waiting for I/O).
> >
> > My last patch checks for pages_min before OOM killing, have you read it?
>
> checking pages_min isn't correct anyways, the lowmem_reserve must taken
> into account or you may not kill tasks when you should really kill
> tasks.

Indeed - this can be improved.

> Plus you're checking for all zones, but kswapd cannot know that it
> doesn't matter if the zone dma is under pages_min, as far as there's no
> GFP_DMA.

You mean boxes with no DMA zone?

If the normal zone is below pages_min+protection, then GFP_KERNEL allocations
will fallback and eat from DMA zone.

I dont get you?

> > > Every allocation is different and page_alloc.c is the only one who
> > > knows what has to be done for every single allocation.
> >
> > OK, what do you propose? Its the third time I ask you this and got no
> > concrete answer yet.
>
> I want to move it to page_alloc.c (and up to the caller) and not in
> kswapd, I mention this a few times.
>
> > Sure, allocators should receive -ENOMEM whenever possible, but this
> > is not the issue here.
>
> it is the issue, because only the context of the task can choose if to
> return -ENOMEM or to invoke the oom killer and try again.

If the task chooses to return -ENOMEM it wont set "task_looping_oom" flag.

OK - you are right to say that "only the context of the task can choose
to return -ENOMEM or invoke the oom killer".

This allocator-context-only information is passed to kswapd via
"task_looping_oom".

> > Triggering OOM killer on __alloc_pages() failure ?
>
> yes, ideally I'd put the oom killer _outside_ alloc_pages, but just
> moving it into alloc_pages should make things better than they are right
> now in vmscan.c.
>
> > Show us the code, please :)
>
> I'm supposedly listening to a meeting right now, then I've a bad kernel
> crash to debug with random mem corruption that I just managed to
> reproduce deterministcally inside uml by emulating numa inside uml and
> I'll be busy until next week at the very least. So I doubt I'll be able
> to write any oom-related code until next week, sorry.

Good luck!