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

Please consider the below patch for 2.6.26 (can somebody from the x86 team pick this up please? Thank you)

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.

Quoted 10 lines · On Tue, 27 May 2008 01:49:47 +0200 Miquel van Smoorenburg <[email protected]>…

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?

Quoted 7 lines

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.

Quoted 3 lines

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?

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

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

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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 ?

arch/x86/kernel/pci-dma.c +0 -3

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

arch/x86/kernel/pci-dma.c line 414

Quoted 6 lines

arch/x86/kernel/pci-dma.c +3 -1

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

Okay, so how about this then ?

arch/x86/kernel/pci-dma.c +0 -3

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

Quoted 6 lines · Ingo Molnar

Quoted 1 line · Miquel van Smoorenburg

Okay, so how about this then ?

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

I've thought about it a bit more, and I think the actual patch that really does what everybody wants is this one instead:

[PATCH] x86: pci-dma.c: don't always add __GFP_NORETRY to gfp

Currently arch/x86/kernel/pci-dma.c always adds __GFP_NORETRY to the allocation flags, because it wants to be reasonably sure not to deadlock when calling alloc_pages().

But really that should only be done in two cases:
- when allocating memory in the lower 16 MB DMA zone.
If there's no free memory there, waiting or OOM killing is of no use
- when optimistically trying an allocation in the DMA32 zone when dma_mask < DMA_32BIT_MASK hoping that the allocation happens to fall within the limits of the dma_mask

Also blindly adding __GFP_NORETRY to the the gfp variable might not be a good idea since we then also use it when calling dma_ops->alloc_coherent(). Clearing it might also not be a good idea, dma_alloc_coherent()'s caller might have set it on purpose. The gfp variable should not be clobbered.

Signed-off-by Miquel van Smoorenburg <[email protected]>

arch/x86/kernel/pci-dma.c +10 -4

@@ -378,6 +378,7 @@ dma_alloc_coherent(struct device *dev, s
 	struct page *page;
 	unsigned long dma_mask = 0;
 	dma_addr_t bus;
+	int noretry = 0;
 
 	/* ignore region specifiers */
 	gfp &= ~(__GFP_DMA | __GFP_HIGHMEM | __GFP_DMA32);

@@ -397,20 +398,25 @@ dma_alloc_coherent(struct device *dev, s
 	if (dev->dma_mask == NULL)
 		return NULL;
 
-	/* Don't invoke OOM killer */
-	gfp |= __GFP_NORETRY;
+	/* Don't invoke OOM killer or retry in lower 16MB DMA zone */
+	if (gfp & __GFP_DMA)
+		noretry = 1;
 
 #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
 	   finding fitting memory in the next higher zone first. If
 	   not retry with true GFP_DMA. -AK */
-	if (dma_mask <= DMA_32BIT_MASK && !(gfp & GFP_DMA))
+	if (dma_mask <= DMA_32BIT_MASK && !(gfp & GFP_DMA)) {
 		gfp |= GFP_DMA32;
+		if (dma_mask < DMA_32BIT_MASK)
+			noretry = 1;
+	}
 #endif
 
  again:
-	page = dma_alloc_pages(dev, gfp, get_order(size));
+	page = dma_alloc_pages(dev,
+		noretry ? gfp | __GFP_NORETRY : gfp, get_order(size));
 	if (page == NULL)
 		return NULL;
 

Quoted 8 lines · Miquel van Smoorenburg

Quoted 4 lines · Ingo Molnar

Quoted 1 line · Miquel van Smoorenburg

Okay, so how about this then ?

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

I've thought about it a bit more, and I think the actual patch that really does what everybody wants is this one instead:

applied a delta patch version of the one below to tip/pci-for-jesse.

Quoted 10 lines · Ingo Molnar

Quoted 8 lines · Miquel van Smoorenburg

Quoted 4 lines · Ingo Molnar

Quoted 1 line · Miquel van Smoorenburg

Okay, so how about this then ?

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

I've thought about it a bit more, and I think the actual patch that really does what everybody wants is this one instead:

Quoted 1 line

applied a delta patch version of the one below to tip/pci-for-jesse.

Speaking of that branch, how do you want to handle it? Is it intended for my linux-next branch or are some of the fixes important to get upstream now? Which branch should I diff/log against to see the changes (last time I looked it appeared neither master nor tip were the correct ones...)

Quoted 10 lines · Ingo Molnar

Quoted 8 lines · Miquel van Smoorenburg

Quoted 4 lines · Ingo Molnar

Quoted 1 line · Miquel van Smoorenburg

Okay, so how about this then ?

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

I've thought about it a bit more, and I think the actual patch that really does what everybody wants is this one instead:

Quoted 1 line

applied a delta patch version of the one below to tip/pci-for-jesse.

Speaking of that branch, how do you want to handle it? Is it intended for my linux-next branch or are some of the fixes important to get upstream now? Which branch should I diff/log against to see the changes (last time I looked it appeared neither master nor tip were the correct ones...)

that branch is always based in -git so if stuff goes upstream patches disappear from it. git-log linus/master..tip/pci-for-jesse should tell the currently pending items. Right now it's just two lowprio items:

Yinghai Lu (2):
pci: debug extra pci resources range
pci: debug extra pci bus resources

Quoted 7 lines · Ingo Molnar

that branch is always based in -git so if stuff goes upstream patches disappear from it. git-log linus/master..tip/pci-for-jesse should tell the currently pending items. Right now it's just two lowprio items:

Yinghai Lu (2):
pci: debug extra pci resources range
pci: debug extra pci bus resources

It looks like these add some new, unconditional debug output? I guess I shouldn't worry about printks so much at this point, given how much obsolete/uninteresting info gets dumped these days, but it would be nice if this were only dumped if some debug option were set. Given the variety of PCI stuff we might like to dump at various times, maybe it's time we added a few new pci=debug=foo options? Yinghai?

On Tue, Jul 1, 2008 at 7:39 PM, Jesse Barnes <[email protected]>

Quoted 9 lines · Ingo Molnar

Quoted 7 lines

that branch is always based in -git so if stuff goes upstream patches disappear from it. git-log linus/master..tip/pci-for-jesse should tell the currently pending items. Right now it's just two lowprio items:

Yinghai Lu (2):
pci: debug extra pci resources range
pci: debug extra pci bus resources

Quoted 6 lines

It looks like these add some new, unconditional debug output? I guess I shouldn't worry about printks so much at this point, given how much obsolete/uninteresting info gets dumped these days, but it would be nice if this were only dumped if some debug option were set. Given the variety of PCI stuff we might like to dump at various times, maybe it's time we added a few new pci=debug=foo options? Yinghai?

that is just limited printout. and it is informative.. that is BAR before kernel try to assign new value to it.