lkml.org 
[lkml]   [2008]   [May]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY
From
Date
On Tue, 2008-05-27 at 01:47 -0700, Andrew Morton wrote:
> On Tue, 27 May 2008 01:49:47 +0200 Miquel van Smoorenburg <mikevs@xs4all.net> wrote:
>
> > Please consider the below patch for 2.6.26 (can somebody from the
> > x86 team pick this up please? Thank you)
> >
> >
> >
> > [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY
> >
> > arch/x86/kernel/pci-dma.c::dma_alloc_coherent() adds __GFP_NORETRY to
> > the gfp flags before calling alloc_pages() to prevent the oom killer
> > from running.
>
> Now, why does dma_alloc_coherent() do that?
>
> If __GFP_FS is cleared (most cases) then we won't be calling
> out_of_memory() anyway.
>
> If __GFP_FS _is_ set then setting __GFP_NORETRY will do much more than
> avoiding oom-killings. It will prevent the page allocator from
> retrying and will cause the problems which one assumes (without
> evidence :() you have observed.

Ah right, this was discussed in a different thread on linux-kernel /
linux-mm. Message id <20080521113028.GA24632@xs4all.net> or see
http://lkml.org/lkml/2008/5/21/131

> So... why not just remove the setting of __GFP_NORETRY? Why is it
> wrong to oom-kill things in this case?

This was the first thing I proposed, since it was already that way in
pci-dma_32.c, the __GFP_NORETRY was only added in pci-dma_64.c . Hence I
found out about this when moving boxes to a 64 bit kernel. But in
2.6.26, those two were merged.

>
> > This has the expected side effect that that alloc_pages() doesn't
> > retry anymore. Not really a problem for dma_alloc_coherent(.. GFP_ATOMIC)
> > which is the way most drivers use it (through pci_alloc_consistent())
> > but drivers that call dma_alloc_coherent(.. GFP_KERNEL) directly can get
> > unexpected failures.
> >
> > Until we have the mask allocator, use a new flag __GFP_NO_OOM
> > instead of __GFP_NORETRY.
> >
>
> But this change increases the chances of a caller getting stuck in the
> page allocator for ever, unable to make progress?

Another bandaid fix I proposed was this:

diff -ruN linux-2.6.26-rc3.orig/arch/x86/kernel/pci-dma.c linux-2.6.26-rc3/arch/x86/kernel/pci-dma.c
--- linux-2.6.26-rc3.orig/arch/x86/kernel/pci-dma.c 2008-05-18 23:36:41.000000000 +0200
+++ linux-2.6.26-rc3/arch/x86/kernel/pci-dma.c 2008-05-22 21:21:37.000000000 +0200
@@ -398,7 +398,8 @@
return NULL;

/* Don't invoke OOM killer */
- gfp |= __GFP_NORETRY;
+ if (!(gfp & __GFP_WAIT))
+ gfp |= __GFP_NORETRY;

#ifdef CONFIG_X86_64
/* Why <=? Even when the mask is smaller than 4GB it is often
This will at least make sure that when called with GFP_KERNEL, __GFP_NORETRY
is not set, while it will be set with GFP_ATOMIC.

But I concluded from the earlier discussion that there was consensus about
__GFP_NO_OOM , so I sent this patch. Now I'm most definitely not an
expert, in fact pretty ignorant really, so if there are serious objections
or a better solution, please drop this patch.

I do think the issue should still be fixed.

The minimum would be to surround the gfp |= __GFP_NORETRY with
#ifdef CONFIG_X86_64 so that at least 32 bit doesn't regress
in 2.6.26
However as dpt_i2o in 2.6.26 works on 64 bit systems now and
it calls dma_alloc_coherent(.. GFP_KERNEL) I'm afraid it might
cause instability with that driver on x86_64 (that's my main
worry. tw_cli crashing is merely inconvenient).

Mike.



\
 
 \ /
  Last update: 2008-05-27 11:39    [W:0.095 / U:3.916 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site