[PATCH] Improve pci_alloc_consistent wrapper on preemptive kernels

[PATCH] Improve pci_alloc_consistent wrapper on preemptive kernels

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

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.


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
--- linux-2.6.8rc2-mm1/include/asm-generic/pci-dma-compat.h	2004-04-06 13:12:19.000000000 +0200
+++ linux-2.6.8rc2-mm1-amd64/include/asm-generic/pci-dma-compat.h	2004-07-30 17:02:49.000000000 +0200
@@ -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

Re: [PATCH] Improve pci_alloc_consistent wrapper on preemptive kernels

[Jeff Garzik]

Andi Kleen wrote:
> 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.
[...]
> +/* 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

Re: [PATCH] Improve pci_alloc_consistent wrapper on preemptive kernels

[Andi Kleen]

On Fri, 30 Jul 2004 13:16:28 -0400
Jeff Garzik <jgarzik@pobox.com> 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) 

> 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

Re: [PATCH] Improve pci_alloc_consistent wrapper on preemptive kernels

[Jeff Garzik]

Andi Kleen wrote:
> On Fri, 30 Jul 2004 13:16:28 -0400
> Jeff Garzik <jgarzik@pobox.com> 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) 
> 
> 
>>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

Re: [PATCH] Improve pci_alloc_consistent wrapper on preemptive kernels

[Andi Kleen]

Jeff Garzik <jgarzik@pobox.com> writes:
>
> Certainly.  Therefore, changing from GFP_ATOMIC will increase
> likelihood of breakage, no?

No, the atomic checks prevents that. When the code wasn't broken
before on preemptive kernels it won't be broken now.

-Andi

Re: [PATCH] Improve pci_alloc_consistent wrapper on preemptive kernels

[Takashi Iwai]

At Fri, 30 Jul 2004 14:07:57 -0400,
Jeff Garzik wrote:
> 
> Andi Kleen wrote:
> > On Fri, 30 Jul 2004 13:16:28 -0400
> > Jeff Garzik <jgarzik@pobox.com> 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) 
> > 
> > 
> >>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?

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


Takashi

Re: [PATCH] Improve pci_alloc_consistent wrapper on preemptive kernels

[Takashi Iwai]

At Fri, 30 Jul 2004 20:47:21 +0200,
I wrote:
> 
> At Fri, 30 Jul 2004 14:07:57 -0400,
> Jeff Garzik wrote:
> > 
> > Andi Kleen wrote:
> > > On Fri, 30 Jul 2004 13:16:28 -0400
> > > Jeff Garzik <jgarzik@pobox.com> 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) 
> > > 
> > > 
> > >>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?
> 
> 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

Re: [PATCH] Improve pci_alloc_consistent wrapper on preemptive kernels

[Andrew Morton]

Andi Kleen <ak@suse.de> wrote:
>
> 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.

Re: [PATCH] Improve pci_alloc_consistent wrapper on preemptive kernels

[James Bottomley]

On Fri, 2004-07-30 at 16:02, Andrew Morton wrote:
> 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

Re: [PATCH] Improve pci_alloc_consistent wrapper on preemptive kernels

[Andrew Morton]

James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
>
> On Fri, 2004-07-30 at 16:02, Andrew Morton wrote:
> > 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?
> 

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

Re: [PATCH] Improve pci_alloc_consistent wrapper on preemptive kernels

[James Bottomley]

On Fri, 2004-07-30 at 16:20, Andrew Morton wrote:
> 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().

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

Re: [PATCH] Improve pci_alloc_consistent wrapper on preemptive kernels

[Andi Kleen]

On Fri, 30 Jul 2004 14:07:57 -0400
Jeff Garzik <jgarzik@pobox.com> 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