Remove __GFP_DMA

PATCH In review Bernhard Walle 7 participants 16 messages May 21, 2007 – May 27, 2007

Subsystems (2)

SCSI Aacraid SCSI RAID driver

lore

Share & tools

lore

Bernhard Walle

author

2007-05-21 15:15 UTC #0 lore raw

PATCH 1/1 Remove __GFP_DMA

[PATCH] [scsi] Remove __GFP_DMA

After 821de3a27bf33f11ec878562577c586cd5f83c64, it's not necessary to alloate a DMA buffer any more in sd.c.

Signed-off-by Bernhard Walle <[email protected]>

---
 drivers/scsi/sd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/scsi/sd.c +1 -1

@@ -1515,7 +1515,7 @@ static int sd_revalidate_disk(struct gen
 	if (!scsi_device_online(sdp))
 		goto out;
 
-	buffer = kmalloc(SD_BUF_SIZE, GFP_KERNEL | __GFP_DMA);
+	buffer = kmalloc(SD_BUF_SIZE, GFP_KERNEL);
 	if (!buffer) {
 		sd_printk(KERN_WARNING, sdkp, "sd_revalidate_disk: Memory "
 			  "allocation failure.\n");

Christoph Lameter

Acked-by on v1 1/1

2007-05-22 21:44 UTC #1 lore raw

Quoted 6 lines · Bernhard Walle

[PATCH] [scsi] Remove __GFP_DMA
After 821de3a27bf33f11ec878562577c586cd5f83c64, it's not necessary to alloate a DMA buffer any more in sd.c.
Signed-off-by Bernhard Walle <[email protected]>

Great that avoids a DMA kmalloc slab. Any other GFP_DMAs left in the scsi layer?

Acked-by Christoph Lameter <[email protected]>

Aubrey Li

2007-05-23 02:41 UTC #2 lore raw

Quoted 7 lines · Christoph Lameter

Quoted 5 lines · Bernhard Walle
[PATCH] [scsi] Remove __GFP_DMA
After 821de3a27bf33f11ec878562577c586cd5f83c64, it's not necessary to alloate a DMA buffer any more in sd.c.

Quoted 6 lines

Quoted 1 line
Signed-off-by Bernhard Walle <[email protected]>
Great that avoids a DMA kmalloc slab. Any other GFP_DMAs left in the scsi layer?
Acked-by Christoph Lameter <[email protected]>

Yes, here is another patch

Signed-off-by Aubrey.Li <[email protected]>

---
drivers/scsi/aacraid/commctrl.c | 12 ++++++------

1 files changed, 6 insertions(+), 6 deletions(-)

drivers/scsi/aacraid/commctrl.c +0 -0

index 72b0393..405722d 100644
@@ -580,8 +580,8 @@ static int aac_send_raw_srb(struct aac_dev* dev,

void __user * arg)

			for (i = 0; i < upsg->count; i++) {
				u64 addr;
				void* p;
-				/* Does this really need to be GFP_DMA? */
-				p = kmalloc(upsg->sg[i].count,GFP_KERNEL|__GFP_DMA);
+
+				p = kmalloc(upsg->sg[i].count,GFP_KERNEL);
				if(p == 0) {
					dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size
= %d buffer number %d of %d\n",
					  upsg->sg[i].count,i,upsg->count));
@@ -624,8 +624,8 @@ static int aac_send_raw_srb(struct aac_dev* dev,
void __user * arg)
			for (i = 0; i < usg->count; i++) {
				u64 addr;
				void* p;
-				/* Does this really need to be GFP_DMA? */
-				p = kmalloc(usg->sg[i].count,GFP_KERNEL|__GFP_DMA);
+
+				p = kmalloc(usg->sg[i].count,GFP_KERNEL);
				if(p == 0) {
					kfree (usg);
					dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size
= %d buffer number %d of %d\n",
@@ -666,8 +666,8 @@ static int aac_send_raw_srb(struct aac_dev* dev,
void __user * arg)
			for (i = 0; i < upsg->count; i++) {
				u64 addr;
				void* p;
-				/* Does this really need to be GFP_DMA? */
-				p = kmalloc(usg->sg[i].count,GFP_KERNEL|__GFP_DMA);
+
+				p = kmalloc(usg->sg[i].count,GFP_KERNEL);
				if(p == 0) {
					dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size
= %d buffer number %d of %d\n",
					  usg->sg[i].count,i,usg->count));

Signature · 1 line

1.5.1.1

James Bottomley

2007-05-23 15:18 UTC #3 lore raw

Quoted 8 lines · Aubrey Li

Quoted 7 lines · Christoph Lameter
Quoted 5 lines · Bernhard Walle
[PATCH] [scsi] Remove __GFP_DMA
After 821de3a27bf33f11ec878562577c586cd5f83c64, it's not necessary to alloate a DMA buffer any more in sd.c.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
Signed-off-by Bernhard Walle <[email protected]>
Great that avoids a DMA kmalloc slab. Any other GFP_DMAs left in the scsi layer?
Acked-by Christoph Lameter <[email protected]>
Yes, here is another patch

I'll defer to Mark on this one. However, please remember that you can't just blindly remove GFP_DMA ... there are some cards which require it.

Aacraid is one example ... it has a set of cards that can only DMA to 31 bits. For them, the GFP_DMA is necessary: The allocation in question is a scatterlist, which must be within the device DMA mask.

James

Robert P. J. Day

2007-05-23 15:56 UTC #4 lore raw

Quoted 7 lines · James Bottomley

I'll defer to Mark on this one. However, please remember that you can't just blindly remove GFP_DMA ... there are some cards which require it.
Aacraid is one example ... it has a set of cards that can only DMA to 31 bits. For them, the GFP_DMA is necessary: The allocation in question is a scatterlist, which must be within the device DMA mask.

a question i asked a while back, and still haven't seen an answer for -- given this in include/linux/gfp.h:

  #define GFP_DMA         __GFP_DMA

is there a qualitative difference between these two macros? is there *supposed* to be? if there isn't, one would think that just one variation would be sufficient.

rday

Signature · 7 lines

========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================

James Bottomley

2007-05-23 16:17 UTC #5 lore raw

Quoted 10 lines · Robert P. J. Day

Quoted 7 lines · James Bottomley
I'll defer to Mark on this one. However, please remember that you can't just blindly remove GFP_DMA ... there are some cards which require it.
Aacraid is one example ... it has a set of cards that can only DMA to 31 bits. For them, the GFP_DMA is necessary: The allocation in question is a scatterlist, which must be within the device DMA mask.

Quoted 8 lines

a question i asked a while back, and still haven't seen an answer for -- given this in include/linux/gfp.h:
  #define GFP_DMA         __GFP_DMA
is there a qualitative difference between these two macros? is there *supposed* to be? if there isn't, one would think that just one variation would be sufficient.

__GFP_ are the raw GFP flags ... the GFP_ are combinations of the flags with predefined meanings. There's no convention that you have to use one form or the other when making combinations of the allocation flags. Historically that's lead to things like

GFP_ATOMIC | __GFP_DMA (indicating additional DMA zone to the usual atomic flags)

and

GFP_ATOMIC | GFP_DMA (indicating same thing, but with the defined flags)

You can argue that GFP_DMA has a pretty bogus GFP meaning and should never appear on its own, which, to my mind makes the former usage preferable. However, GFP_DMA has been in linux since pretty much the dawn of ISA drivers, so I think it's conventionally well understood.

James

Salyzyn, Mark

2007-05-23 19:17 UTC #6 lore raw

In reply to James Bottomley

The 31 bit limit for some of these cards is a problem, we currently only do __GFP_DMA for bounce buffer sg elements allocated for user supplied references in ioctls.

I figure we should be using pci_alloc_consistent calls for these allocations to more accurately acquire memory within the 31 bit limit if necessary, we could switch to these to remove the need for the __GFP_DMA flag in the aacraid driver?

-- Mark

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of James Bottomley
Sent: Wednesday, May 23, 2007 11:18 AM
To: Aubrey Li
Cc: Christoph Lameter; Bernhard Walle; [email protected]; Andrew Morton; [email protected]
Subject: Re: [PATCH] [scsi] Remove __GFP_DMA

Quoted 6 lines · Aubrey Li

Quoted 5 lines · Christoph Lameter
Quoted 3 lines · Bernhard Walle
[PATCH] [scsi] Remove __GFP_DMA
After 821de3a27bf33f11ec878562577c586cd5f83c64, it's not necessary

to alloate a

Quoted 5 lines

Quoted 5 lines
Quoted 3 lines
DMA buffer any more in sd.c.
Signed-off-by Bernhard Walle <[email protected]>
Great that avoids a DMA kmalloc slab. Any other GFP_DMAs left in the

scsi

Quoted 5 lines

Quoted 3 lines
layer?
Acked-by Christoph Lameter <[email protected]>
Yes, here is another patch

I'll defer to Mark on this one. However, please remember that you can't just blindly remove GFP_DMA ... there are some cards which require it.

James

Alan Cox

2007-05-23 19:35 UTC #7 lore raw

Quoted 8 lines · Mark

The 31 bit limit for some of these cards is a problem, we currently only do __GFP_DMA for bounce buffer sg elements allocated for user supplied references in ioctls.
I figure we should be using pci_alloc_consistent calls for these allocations to more accurately acquire memory within the 31 bit limit if necessary, we could switch to these to remove the need for the __GFP_DMA flag in the aacraid driver?

That didn't used to work right on the AMD boards when I tried it last as we ended up with a buffer that was mapped by the IOMMU for some reason and that was not below 2GB.

James Bottomley

2007-05-27 19:46 UTC #8 lore raw

Quoted 7 lines · Alan Cox

Quoted 4 lines · Mark
The 31 bit limit for some of these cards is a problem, we currently only do __GFP_DMA for bounce buffer sg elements allocated for user supplied references in ioctls.

Quoted 8 lines

Quoted 4 lines
I figure we should be using pci_alloc_consistent calls for these allocations to more accurately acquire memory within the 31 bit limit if necessary, we could switch to these to remove the need for the __GFP_DMA flag in the aacraid driver?
That didn't used to work right on the AMD boards when I tried it last as we ended up with a buffer that was mapped by the IOMMU for some reason and that was not below 2GB.

Is that a bug in the x86_64 subsystem ... or just a bug in the HW?

I'm assuming what happened is that the GART aperture was placed above the 2GB limit making the GART unable to act as an IOMMU for any devices with a dma_mask < 32bit?

This does show up a problem in our dma_ functions: namely that if the system says it has an IOMMU then we assume it's fully functional (i.e. able to reach all addresses). There was a move afoot a while ago to replace the single PCI_DMA_BUS_IS_PHYS macro which is used to tell the system globally if we have an IOMMU with something like dma_dev_uses_iommu(struct device *) which would do the same thing but on a per-device basis. Does this need to be resurrected?

James

Salyzyn, Mark

2007-05-24 13:24 UTC #9 lore raw

In reply to Aubrey Li

So, is the sequence:

	p = kmalloc(upsg->sg[i].count,GFP_KERNEL);
	. . .
	addr = pci_map_single(dev->pdev, p, upsg->sg[i].count,

data_dir);

Going to ensure that we have a 31 bit (not 32 bit) physical address?

If not, then I reject this patch. We can not consider replacement with pci_alloc_consistent until it works on AMD respecting the DMA masks.

Sincerely -- Mark Salyzyn

Quoted 101 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Aubrey Li
Sent: Tuesday, May 22, 2007 10:41 PM
To: Christoph Lameter
Cc: Bernhard Walle; [email protected]; Andrew Morton; [email protected]; James Bottomley
Subject: Re: [PATCH] [scsi] Remove __GFP_DMA
Quoted 5 lines · Christoph Lameter
Quoted 3 lines · Bernhard Walle
[PATCH] [scsi] Remove __GFP_DMA
After 821de3a27bf33f11ec878562577c586cd5f83c64, it's not
necessary to alloate a
Quoted 5 lines
Quoted 3 lines
DMA buffer any more in sd.c.
Signed-off-by Bernhard Walle <[email protected]>
Great that avoids a DMA kmalloc slab. Any other GFP_DMAs
left in the scsi
Quoted 3 lines
layer?
Acked-by Christoph Lameter <[email protected]>
Yes, here is another patch
Signed-off-by Aubrey.Li <[email protected]>
---
 drivers/scsi/aacraid/commctrl.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
drivers/scsi/aacraid/commctrl.c +0 -1
index 72b0393..405722d 100644
@@ -580,8 +580,8 @@ static int aac_send_raw_srb(struct aac_dev* dev,
 void __user * arg)
 			for (i = 0; i < upsg->count; i++) {
 				u64 addr;
 				void* p;
-				/* Does this really need to be 
GFP_DMA? */
drivers/scsi/aacraid/commctrl.c +0 -1
-				p = 
kmalloc(upsg->sg[i].count,GFP_KERNEL|__GFP_DMA);
drivers/scsi/aacraid/commctrl.c +2 -0
+
+				p = 
kmalloc(upsg->sg[i].count,GFP_KERNEL);
if(p == 0) {
dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
upsg->sg[i].count,i,upsg->count));
drivers/scsi/aacraid/commctrl.c +0 -1
@@ -624,8 +624,8 @@ static int aac_send_raw_srb(struct aac_dev* dev,
 void __user * arg)
 			for (i = 0; i < usg->count; i++) {
 				u64 addr;
 				void* p;
-				/* Does this really need to be 
GFP_DMA? */
drivers/scsi/aacraid/commctrl.c +0 -1
-				p = 
kmalloc(usg->sg[i].count,GFP_KERNEL|__GFP_DMA);
drivers/scsi/aacraid/commctrl.c +2 -0
+
+				p = 
kmalloc(usg->sg[i].count,GFP_KERNEL);
drivers/scsi/aacraid/commctrl.c +0 -0
 				if(p == 0) {
 					kfree (usg);
dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
drivers/scsi/aacraid/commctrl.c +0 -1
@@ -666,8 +666,8 @@ static int aac_send_raw_srb(struct aac_dev* dev,
 void __user * arg)
 			for (i = 0; i < upsg->count; i++) {
 				u64 addr;
 				void* p;
-				/* Does this really need to be 
GFP_DMA? */
drivers/scsi/aacraid/commctrl.c +0 -1
-				p = 
kmalloc(usg->sg[i].count,GFP_KERNEL|__GFP_DMA);
drivers/scsi/aacraid/commctrl.c +2 -0
+
+				p = 
kmalloc(usg->sg[i].count,GFP_KERNEL);
if(p == 0) {
dprintk((KERN_DEBUG"aacraid: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
usg->sg[i].count,i,usg->count));
Signature · 7 lines
1.5.1.1
-
To unsubscribe from this list: send the line "unsubscribe 
linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

James Bottomley

2007-05-24 14:59 UTC #10 lore raw

Quoted 8 lines · Mark

So, is the sequence:
	p = kmalloc(upsg->sg[i].count,GFP_KERNEL);
	. . .
	addr = pci_map_single(dev->pdev, p, upsg->sg[i].count,
data_dir);
Going to ensure that we have a 31 bit (not 32 bit) physical address?

No, unfortunately. Implementing kmalloc_mask() and kmalloc_dev() was something I said I'd do ... about two years ago.

Quoted 2 lines

If not, then I reject this patch. We can not consider replacement with pci_alloc_consistent until it works on AMD respecting the DMA masks.

It should, I believe ... x86_64 has a complex allocation scheme where for masks < 32 bit it first tries in GFP_DMA32 and sees if it gets lucky before falling back to GFP_DMA. i386 just goes straight to GFP_DMA.

James

Christoph Lameter

2007-05-24 17:00 UTC #11 lore raw

Quoted 4 lines · James Bottomley

Quoted 1 line
Going to ensure that we have a 31 bit (not 32 bit) physical address?
No, unfortunately. Implementing kmalloc_mask() and kmalloc_dev() was something I said I'd do ... about two years ago.

Tell me more about these ideas.

James Bottomley

2007-05-24 17:15 UTC #12 lore raw

Quoted 8 lines · Christoph Lameter

Quoted 4 lines · James Bottomley
Quoted 1 line
Going to ensure that we have a 31 bit (not 32 bit) physical address?
No, unfortunately. Implementing kmalloc_mask() and kmalloc_dev() was something I said I'd do ... about two years ago.
Tell me more about these ideas.

Oh, it was this Kernel Summit presentation which discussed it

http://licensing.steeleye.com/support/papers/kernel_summit_iommu.pdf

The writeup of the session is here:

http://lwn.net/Articles/144100/

The idea was basically to match an allocation to a device mask. I was going to do a generic implementation (which would probably kmalloc, check the physaddr and fall back to GFP_DMA if we were unlucky) but allow the architectures to override.

However, the majority of need (except for the aacraid like devices) was solved by GFP_DMA32

James

Christoph Lameter

2007-05-24 17:22 UTC #13 lore raw

Quoted 4 lines · James Bottomley

The idea was basically to match an allocation to a device mask. I was going to do a generic implementation (which would probably kmalloc, check the physaddr and fall back to GFP_DMA if we were unlucky) but allow the architectures to override.

Hmmmm... We could actually implement something like it in the slab allocators. The mask parameter would lead the allocator to check if the objects are in a satisfactory range. If not it could examine its partial lists for slabs that satisfy the range. If that does not work then it would eventually go to the page allocator to ask for a page in a fitting range.

That wont be fast though. How performance sensitive are the allocations?

James Bottomley

2007-05-24 17:26 UTC #14 lore raw

Quoted 7 lines · Christoph Lameter

Quoted 4 lines · James Bottomley
The idea was basically to match an allocation to a device mask. I was going to do a generic implementation (which would probably kmalloc, check the physaddr and fall back to GFP_DMA if we were unlucky) but allow the architectures to override.

Quoted 8 lines

Hmmmm... We could actually implement something like it in the slab allocators. The mask parameter would lead the allocator to check if the objects are in a satisfactory range. If not it could examine its partial lists for slabs that satisfy the range. If that does not work then it would eventually go to the page allocator to ask for a page in a fitting range.
That wont be fast though. How performance sensitive are the allocations?

Most of them aren't very. They're usually things that are set at driver intialisation time (shared mailbox memory etc).

If the allocation is performance sensitive and has to be done at command or ioctl submission time, we tend to feed the consistent or mapped memory into a dma pool which pre allocates and solves the performance issue.

In the aacraid case, the mbox is done at ioctl time, but isn't incredibly performance sensitive.

James

Christoph Lameter

2007-05-24 16:59 UTC #15 lore raw

In reply to Salyzyn, Mark

Quoted 8 lines · Mark

So, is the sequence:
	p = kmalloc(upsg->sg[i].count,GFP_KERNEL);
	. . .
	addr = pci_map_single(dev->pdev, p, upsg->sg[i].count,
data_dir);
Going to ensure that we have a 31 bit (not 32 bit) physical address?

Only if you have less than 2G of memory. So no.