Improve pci_alloc_consistent wrapper on preemptive kernels

PATCH In review Andi Kleen 5 participants 11 messages Jul 30, 2004

Subsystems (2)

PCI Generic include/asm header files

lore

Share & tools

lore

Andi Kleen

author

2004-07-30 17:06 UTC #0 lore raw

PATCH 1/1 Improve pci_alloc_consistent wrapper on preemptive kernels

This is a minor optimization for the pci_alloc_consistent wrapper for the generic dma API. When the kernel is compiled preemptive the caller can decide if the allocation needs to be GFP_KERNEL or GFP_ATOMIC.

I had this optimization previously in x86-64 private code, but it needs to move up now that x86-64 uses the generic DMA API.

include/asm-generic/pci-dma-compat.h +9 -1

diff -urpN -X ../KDIFX linux-2.6.8rc2-mm1/include/asm-generic/pci-dma-compat.h linux-2.6.8rc2-mm1-amd64/include/asm-generic/pci-dma-compat.h
@@ -5,6 +5,13 @@
 #define _ASM_GENERIC_PCI_DMA_COMPAT_H
 
 #include <linux/dma-mapping.h>
+#include <linux/config.h>
+
+#ifdef CONFIG_PREEMPT
+#define preempt_atomic() in_atomic()
+#else
+#define preempt_atomic() 1
+#endif
 
 /* note pci_set_dma_mask isn't here, since it's a public function
  * exported from drivers/pci, use dma_supported instead */

@@ -15,11 +22,12 @@ pci_dma_supported(struct pci_dev *hwdev,
 	return dma_supported(hwdev == NULL ? NULL : &hwdev->dev, mask);
 }
 
+/* Would be better to move this out of line. It's already quite big. */
 static inline void *
 pci_alloc_consistent(struct pci_dev *hwdev, size_t size,
 		     dma_addr_t *dma_handle)
 {
-	return dma_alloc_coherent(hwdev == NULL ? NULL : &hwdev->dev, size, dma_handle, GFP_ATOMIC);
+	return dma_alloc_coherent(hwdev == NULL ? NULL : &hwdev->dev, size, dma_handle, preempt_atomic() ? GFP_ATOMIC : GFP_KERNEL);
 }
 
 static inline void

Jeff Garzik

2004-07-30 17:18 UTC #1 lore raw

Quoted 3 lines · Andi Kleen

This is a minor optimization for the pci_alloc_consistent wrapper for the generic dma API. When the kernel is compiled preemptive the caller can decide if the allocation needs to be GFP_KERNEL or GFP_ATOMIC.

[...]

include/asm-generic/pci-dma-compat.h pci_dma_supported(struct pci_dev *hwdev, · line 31

Quoted 8 lines

include/asm-generic/pci-dma-compat.h +2 -1

+/* Would be better to move this out of line. It's already quite big. */
 static inline void *
 pci_alloc_consistent(struct pci_dev *hwdev, size_t size,
 		     dma_addr_t *dma_handle)
 {
-	return dma_alloc_coherent(hwdev == NULL ? NULL : &hwdev->dev, size, dma_handle, GFP_ATOMIC);
+	return dma_alloc_coherent(hwdev == NULL ? NULL : &hwdev->dev, size, dma_handle, preempt_atomic() ? GFP_ATOMIC : GFP_KERNEL);
 }

I do agree with the patch, but I have two worries:

1) Changing from GFP_ATOMIC to <something else> may break code
2) Conversely from #1, I also worry why GFP_ATOMIC would be needed at all. I code all my drivers to require that pci_alloc_consistent() be called from somewhere that is allowed to sleep.

Jeff

Andi Kleen

author

2004-07-30 17:43 UTC #2 lore raw

Quoted 2 lines · On Fri, 30 Jul 2004 13:16:28 -0400 Jeff Garzik <[email protected]> wrote:

1) Changing from GFP_ATOMIC to <something else> may break code

x86-64 did it for a long time and I am not aware of problems with it (however I don't know how widespread CONFIG_PREEMPT use on x86-64 is)

Quoted 3 lines

2) Conversely from #1, I also worry why GFP_ATOMIC would be needed at all. I code all my drivers to require that pci_alloc_consistent() be called from somewhere that is allowed to sleep.

Maybe you do, but others don't.

-Andi

Jeff Garzik

2004-07-30 18:08 UTC #3 lore raw

Quoted 8 lines · Andi Kleen

Quoted 1 line · On Fri, 30 Jul 2004 13:16:28 -0400 Jeff Garzik <[email protected]> wrote:
1) Changing from GFP_ATOMIC to <something else> may break code

Quoted 8 lines

x86-64 did it for a long time and I am not aware of problems with it (however I don't know how widespread CONFIG_PREEMPT use on x86-64 is)
Quoted 1 line
2) Conversely from #1, I also worry why GFP_ATOMIC would be needed at all. I code all my drivers to require that pci_alloc_consistent() be called from somewhere that is allowed to sleep.
Maybe you do, but others don't.

Certainly. Therefore, changing from GFP_ATOMIC will increase likelihood of breakage, no?

Jeff

Takashi Iwai

2004-07-30 18:47 UTC #4 lore raw

At Fri, 30 Jul 2004 14:07:57 -0400,

Quoted 19 lines · Jeff Garzik

Quoted 17 lines · Andi Kleen
Quoted 1 line · On Fri, 30 Jul 2004 13:16:28 -0400 Jeff Garzik <[email protected]> wrote:
1) Changing from GFP_ATOMIC to <something else> may break code
x86-64 did it for a long time and I am not aware of problems with it (however I don't know how widespread CONFIG_PREEMPT use on x86-64 is)
Quoted 3 lines
2) Conversely from #1, I also worry why GFP_ATOMIC would be needed at all. I code all my drivers to require that pci_alloc_consistent() be called from somewhere that is allowed to sleep.

Quoted 4 lines

Quoted 1 line
Maybe you do, but others don't.
Certainly. Therefore, changing from GFP_ATOMIC will increase likelihood of breakage, no?

pci_alloc_consistent() was GFP_ATOMIC only on 2.4 anyway, so I don't expect there would be any breakage...

Takashi

Takashi Iwai

2004-07-30 19:28 UTC #5 lore raw

At Fri, 30 Jul 2004 20:47:21 +0200,

Quoted 22 lines · I wrote:

At Fri, 30 Jul 2004 14:07:57 -0400,
Quoted 19 lines · Jeff Garzik
Quoted 17 lines · Andi Kleen
Quoted 1 line · On Fri, 30 Jul 2004 13:16:28 -0400 Jeff Garzik <[email protected]> wrote:
1) Changing from GFP_ATOMIC to <something else> may break code
x86-64 did it for a long time and I am not aware of problems with it (however I don't know how widespread CONFIG_PREEMPT use on x86-64 is)
Quoted 3 lines
2) Conversely from #1, I also worry why GFP_ATOMIC would be needed at all. I code all my drivers to require that pci_alloc_consistent() be called from somewhere that is allowed to sleep.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Maybe you do, but others don't.
Certainly. Therefore, changing from GFP_ATOMIC will increase likelihood of breakage, no?
pci_alloc_consistent() was GFP_ATOMIC only on 2.4 anyway, so I don't expect there would be any breakage...

Sorry got confused. Forget my comment above. The patch won't break, of course (although I don't see much gain by it).

Well, I see now the necessity of this patch - pci_alloc_consistent() is still used in so many drivers... Maybe we can clean up with a script?

Takashi

Andi Kleen

author

2004-07-30 21:42 UTC #6 lore raw

In reply to Jeff Garzik

Quoted 3 lines · On Fri, 30 Jul 2004 14:07:57 -0400 Jeff Garzik <[email protected]> wrote:

Certainly. Therefore, changing from GFP_ATOMIC will increase likelihood of breakage, no?

No, except for places that are already broken on preemptive kernels (and note this change only does something on preemptive kernels)

-Andi

Andrew Morton

2004-07-30 20:04 UTC #7 lore raw

In reply to Andi Kleen

Quoted 4 lines · Andi Kleen

This is a minor optimization for the pci_alloc_consistent wrapper for
the generic dma API. When the kernel is compiled preemptive the caller can decide if the allocation needs to be GFP_KERNEL or GFP_ATOMIC.

We're paying for past sins here. I think it would be better to create a new version of pci_alloc_consistent() which takes gfp_flags, then migrate the drivers you care about to use it. That way the benefit is available on non-preempt kernels too.

The ultimate aim of course would be to deprecate then remove the old function.

James Bottomley

2004-07-30 20:13 UTC #8 lore raw

Quoted 7 lines · Andrew Morton

We're paying for past sins here. I think it would be better to create a new version of pci_alloc_consistent() which takes gfp_flags, then migrate the drivers you care about to use it. That way the benefit is available on non-preempt kernels too.
The ultimate aim of course would be to deprecate then remove the old function.

True, that's why it was added for dma_alloc_coherent().

Is there any need for a new wrapper? Why not just use dma_alloc_coherent() from now on?

James

Andrew Morton

2004-07-30 20:22 UTC #9 lore raw

Quoted 7 lines · James Bottomley

Quoted 5 lines · Andrew Morton
We're paying for past sins here. I think it would be better to create a new version of pci_alloc_consistent() which takes gfp_flags, then migrate the drivers you care about to use it. That way the benefit is available on non-preempt kernels too.

Quoted 8 lines

Quoted 2 lines
The ultimate aim of course would be to deprecate then remove the old function.
True, that's why it was added for dma_alloc_coherent().
Is there any need for a new wrapper? Why not just use dma_alloc_coherent() from now on?

Sounds sane. But the default version in asm-generic/dma-mapping.h needs to be fixed up:

static inline void *
dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
int flag)

{
	BUG_ON(dev->bus != &pci_bus_type);

	return pci_alloc_consistent(to_pci_dev(dev), size, dma_handle);
}

If we stick with this model, we'll still need a new pci_alloc_consistent_gfp().

James Bottomley

2004-07-30 20:33 UTC #10 lore raw

Quoted 6 lines · Andrew Morton

Sounds sane. But the default version in asm-generic/dma-mapping.h needs to be fixed up:
static inline void *
dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
int flag)

Quoted 7 lines

{
	BUG_ON(dev->bus != &pci_bus_type);

	return pci_alloc_consistent(to_pci_dev(dev), size, dma_handle);
}
If we stick with this model, we'll still need a new pci_alloc_consistent_gfp().

Theoretically, that was just to help architectures that didn't want to implement the dma_ functions. If we make dma_alloc_coherent() the preferred implementation, we just remove this because now all arch's will have to implement dma_alloc_coherent anyway.

James