[PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

[PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

[Miquel van Smoorenburg]

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.

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.

Signed-off-by: Miquel van Smoorenburg <miquels@cistron.nl>

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 20:42:10.000000000 +0200
@@ -398,7 +398,7 @@
 		return NULL;
 
 	/* Don't invoke OOM killer */
-	gfp |= __GFP_NORETRY;
+	gfp |= __GFP_NO_OOM;
 
 #ifdef CONFIG_X86_64
 	/* Why <=? Even when the mask is smaller than 4GB it is often
diff -ruN linux-2.6.26-rc3.orig/include/linux/gfp.h linux-2.6.26-rc3/include/linux/gfp.h
--- linux-2.6.26-rc3.orig/include/linux/gfp.h	2008-05-18 23:36:41.000000000 +0200
+++ linux-2.6.26-rc3/include/linux/gfp.h	2008-05-22 21:17:36.000000000 +0200
@@ -43,6 +43,7 @@
 #define __GFP_REPEAT	((__force gfp_t)0x400u)	/* See above */
 #define __GFP_NOFAIL	((__force gfp_t)0x800u)	/* See above */
 #define __GFP_NORETRY	((__force gfp_t)0x1000u)/* See above */
+#define __GFP_NO_OOM	((__force gfp_t)0x2000u)/* Don't invoke oomkiller */
 #define __GFP_COMP	((__force gfp_t)0x4000u)/* Add compound page metadata */
 #define __GFP_ZERO	((__force gfp_t)0x8000u)/* Return zeroed page on success */
 #define __GFP_NOMEMALLOC ((__force gfp_t)0x10000u) /* Don't use emergency reserves */
diff -ruN linux-2.6.26-rc3.orig/mm/page_alloc.c linux-2.6.26-rc3/mm/page_alloc.c
--- linux-2.6.26-rc3.orig/mm/page_alloc.c	2008-05-18 23:36:41.000000000 +0200
+++ linux-2.6.26-rc3/mm/page_alloc.c	2008-05-22 17:39:12.000000000 +0200
@@ -1583,7 +1583,8 @@
 					zonelist, high_zoneidx, alloc_flags);
 		if (page)
 			goto got_pg;
-	} else if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
+	} else if ((gfp_mask & __GFP_FS) &&
+			!(gfp_mask & (__GFP_NORETRY|__GFP_NO_OOM))) {
 		if (!try_set_zone_oom(zonelist, gfp_mask)) {
 			schedule_timeout_uninterruptible(1);
 			goto restart;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

[Ingo Molnar]

* 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)

looks good to me in principle - but it should go via -mm as it touches 
mm/page_alloc.c. Andrew: this fix is for v2.6.26.

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

[Andrew Morton]

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.

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

> 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?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

[Miquel van Smoorenburg]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

[Andi Kleen]

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

When the 16MB zone overflows (which can be common in some workloads)
calling the OOM killer is pretty useless because it has barely any 
real user data [only exception would be the "only 16MB" case Alan
mentioned]. Killing random processes in this case is bad. 

I think for 16MB __GFP_NORETRY is ok because there should be 
nothing freeable in there so looping is useless. Only exception would be the 
"only 16MB total" case again but I'm not sure 2.6 supports that at all
on x86.

On the other hand d_a_c() does more allocations than just 16MB, especially
on 64bit and the other zones need different strategies.


> But this change increases the chances of a caller getting stuck in the
> page allocator for ever, unable to make progress?

At least for much longer, yes I am somewhat worried about this too.

Sometimes a OOM regression test suite would be really nice.

-Andi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

[Miquel van Smoorenburg]

On Wed, 2008-05-28 at 04:47 +0200, Andi Kleen wrote:
> > So...  why not just remove the setting of __GFP_NORETRY?  Why is it
> > wrong to oom-kill things in this case?
> 
> When the 16MB zone overflows (which can be common in some workloads)
> calling the OOM killer is pretty useless because it has barely any 
> real user data [only exception would be the "only 16MB" case Alan
> mentioned]. Killing random processes in this case is bad. 
> 
> I think for 16MB __GFP_NORETRY is ok because there should be 
> nothing freeable in there so looping is useless. Only exception would be the 
> "only 16MB total" case again but I'm not sure 2.6 supports that at all
> on x86.
> 
> On the other hand d_a_c() does more allocations than just 16MB, especially
> on 64bit and the other zones need different strategies.

Okay, so how about this then ?

--- linux-2.6.26-rc4.orig/arch/x86/kernel/pci-dma.c	2008-05-26 20:08:11.000000000 +0200
+++ linux-2.6.26-rc4/arch/x86/kernel/pci-dma.c	2008-05-28 10:27:41.000000000 +0200
@@ -397,9 +397,6 @@
 	if (dev->dma_mask == NULL)
 		return NULL;
 
-	/* Don't invoke OOM killer */
-	gfp |= __GFP_NORETRY;
-
 #ifdef CONFIG_X86_64
 	/* Why <=? Even when the mask is smaller than 4GB it is often
 	   larger than 16MB and in this case we have a chance of
@@ -410,7 +407,9 @@
 #endif
 
  again:
-	page = dma_alloc_pages(dev, gfp, get_order(size));
+	/* Don't invoke OOM killer or retry in lower 16MB DMA zone */
+	page = dma_alloc_pages(dev,
+		(gfp & GFP_DMA) ? gfp | __GFP_NORETRY : gfp, get_order(size));
 	if (page == NULL)
 		return NULL;
 

Mike.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

[Andrew Morton]

On Wed, 28 May 2008 10:31:25 +0200 Miquel van Smoorenburg <mikevs@xs4all.net> wrote:

> > When the 16MB zone overflows (which can be common in some workloads)
> > calling the OOM killer is pretty useless because it has barely any 
> > real user data [only exception would be the "only 16MB" case Alan
> > mentioned]. Killing random processes in this case is bad. 
> > 
> > I think for 16MB __GFP_NORETRY is ok because there should be 
> > nothing freeable in there so looping is useless. Only exception would be the 
> > "only 16MB total" case again but I'm not sure 2.6 supports that at all
> > on x86.
> > 
> > On the other hand d_a_c() does more allocations than just 16MB, especially
> > on 64bit and the other zones need different strategies.
> 
> Okay, so how about this then ?
> 
> --- linux-2.6.26-rc4.orig/arch/x86/kernel/pci-dma.c	2008-05-26 20:08:11.000000000 +0200
> +++ linux-2.6.26-rc4/arch/x86/kernel/pci-dma.c	2008-05-28 10:27:41.000000000 +0200
> @@ -397,9 +397,6 @@
>  	if (dev->dma_mask == NULL)
>  		return NULL;
>  
> -	/* Don't invoke OOM killer */
> -	gfp |= __GFP_NORETRY;
> -
>  #ifdef CONFIG_X86_64
>  	/* Why <=? Even when the mask is smaller than 4GB it is often
>  	   larger than 16MB and in this case we have a chance of
> @@ -410,7 +407,9 @@
>  #endif
>  
>   again:
> -	page = dma_alloc_pages(dev, gfp, get_order(size));
> +	/* Don't invoke OOM killer or retry in lower 16MB DMA zone */
> +	page = dma_alloc_pages(dev,
> +		(gfp & GFP_DMA) ? gfp | __GFP_NORETRY : gfp, get_order(size));
>  	if (page == NULL)
>  		return NULL;

I guess that's more specifally solving that-which-we-wish-to-solve.

Formally we should be testing __GFP_DMA here, not GFP_DMA - just the
zone selector field.  They're presently equal, but someone could
legitimately come along and do

#define GFP_DMA (__GFP_DMA|__GFP_HIGH)

or similar.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

[Andi Kleen]

> > -	page = dma_alloc_pages(dev, gfp, get_order(size));
> > +	/* Don't invoke OOM killer or retry in lower 16MB DMA zone */
> > +	page = dma_alloc_pages(dev,
> > +		(gfp & GFP_DMA) ? gfp | __GFP_NORETRY : gfp, get_order(size));
> >  	if (page == NULL)
> >  		return NULL;
> 
> I guess that's more specifally solving that-which-we-wish-to-solve.

Then the allocator could still be stuck in ZONE_DMA32 on 64bit.

Also d_a_c() does one "speculative" allocation, as in an allocation
where it knows the zone is too large for the mask but it tries anyways
because it often works. In that case too much trying is also not good.

-Andi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

[Ingo Molnar]

* Miquel van Smoorenburg <mikevs@xs4all.net> wrote:

> Okay, so how about this then ?
> 
> --- linux-2.6.26-rc4.orig/arch/x86/kernel/pci-dma.c	2008-05-26 20:08:11.000000000 +0200
> +++ linux-2.6.26-rc4/arch/x86/kernel/pci-dma.c	2008-05-28 10:27:41.000000000 +0200
> @@ -397,9 +397,6 @@
>  	if (dev->dma_mask == NULL)
>  		return NULL;
>  
> -	/* Don't invoke OOM killer */
> -	gfp |= __GFP_NORETRY;
> -
>  #ifdef CONFIG_X86_64
>  	/* Why <=? Even when the mask is smaller than 4GB it is often
>  	   larger than 16MB and in this case we have a chance of
> @@ -410,7 +407,9 @@
>  #endif
>  
>   again:
> -	page = dma_alloc_pages(dev, gfp, get_order(size));
> +	/* Don't invoke OOM killer or retry in lower 16MB DMA zone */
> +	page = dma_alloc_pages(dev,
> +		(gfp & GFP_DMA) ? gfp | __GFP_NORETRY : gfp, get_order(size));
>  	if (page == NULL)
>  		return NULL;

applied to tip/pci-for-jesse for more testing. Thanks,

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>