GFP_DMA use in SCSI midlayer

GFP_DMA use in SCSI midlayer

[Andi Kleen]

>From a quick grep the SCSI midlayer still uses a lot of GFP_DMA
for various things:

% gid GFP_DMA | grep scsi
drivers/scsi/aha1542.c:704:             SCpnt->host_scribble = (unsigned char *) kmalloc(512, GFP_KERNEL | GFP_DMA);
drivers/scsi/ch.c:266:  buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
drivers/scsi/ch.c:323:  buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
drivers/scsi/ch.c:773:          buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
drivers/scsi/eata.c:1358:               gfp_t gfp_mask = (shost->unchecked_isa_dma ? GFP_DMA : 0) | GFP_ATOMIC;
drivers/scsi/initio.c:2830:             if ((tul_scb = (SCB *) kmalloc(i, GFP_ATOMIC | GFP_DMA)) != NULL)
drivers/scsi/osst.c:5250:               priority |= GFP_DMA;
drivers/scsi/pluto.c:120:       fcs = (struct ctrl_inquiry *) kmalloc (sizeof (struct ctrl_inquiry) * fcscount, GFP_DMA);
drivers/scsi/sg.c:1660:  * XXX(hch): we shouldn't need GFP_DMA for the actual S/G list.
drivers/scsi/sg.c:1663:          gfp_flags |= GFP_DMA;
drivers/scsi/sg.c:2453:         page_mask = GFP_ATOMIC | GFP_DMA | __GFP_COMP | __GFP_NOWARN;
drivers/scsi/sr.c:628:  buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
drivers/scsi/sr.c:728:  buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
drivers/scsi/sr_ioctl.c:34:/* primitive to determine whether we need to have GFP_DMA set based on
drivers/scsi/sr_ioctl.c:36:#define SR_GFP_DMA(cd) (((cd)->device->host->unchecked_isa_dma) ? GFP_DMA : 0)
drivers/scsi/sr_vendor.c:120:   buffer = (unsigned char *) kmalloc(512, GFP_KERNEL | GFP_DMA);
drivers/scsi/sr_vendor.c:167:   buffer = (unsigned char *) kmalloc(512, GFP_KERNEL | GFP_DMA);
drivers/scsi/st.c:3627:         priority |= GFP_DMA;
drivers/scsi/u14-34f.c:983:            (sh[j]->unchecked_isa_dma ? GFP_DMA : 0) | GFP_ATOMIC))) {

GFP_DMA in general is deprecated and should be replaced by appropiate dma_alloc_coherent()
or similar.  And I can't imagine any modern systems still need them, and for 
the few still non CONFIG_BROKEN ISA drivers maybe some other way can be found?
And do they really require the mid layer data structures to be GFP_DMA too?

Is it possible to get rid of those GFP_DMAs in the mid layer and the 
higher level drivers like sd,sr,ch etc.?

Thanks,

-Andi

Re: GFP_DMA use in SCSI midlayer

[James Bottomley]

On Sun, 2006-10-01 at 22:13 +0200, Andi Kleen wrote:
> GFP_DMA in general is deprecated and should be replaced by appropiate
> dma_alloc_coherent()

Um, no it shouldn't.

All of the places where we use GFP_DMA are because we might be
addressing an ISA card or other strange mask limited card (gated usually
by unchecked_isa_dma).  However, what is wanted in every case is
ordinary memory, not coherent memory.  They can't simply be replaced
with dma_alloc_coherent because

a) it will waste memory for platforms that only do it in page size
multiples
b) It will fail on platforms that can't do it at all.

Coherent memory is really only for device drivers to use in mailboxes
and shared ring buffers.

> or similar.  And I can't imagine any modern systems still need them, and for 
> the few still non CONFIG_BROKEN ISA drivers maybe some other way can be found?
> And do they really require the mid layer data structures to be GFP_DMA too?

Well, it's the "or similar" that's still the problem.  All they need is
ordinary memory which respects the device mask (then I can get rid of
the unchecked_isa_dma flag as well).

One could argue, I suppose, that when we get every SCSI path to go via
sg, then the block layer will bounce this for us, and we don't need to
worry at all.

> Is it possible to get rid of those GFP_DMAs in the mid layer and the 
> higher level drivers like sd,sr,ch etc.?

James

Re: GFP_DMA use in SCSI midlayer

[Andi Kleen]

On Sunday 01 October 2006 22:40, James Bottomley wrote:
> On Sun, 2006-10-01 at 22:13 +0200, Andi Kleen wrote:
> > GFP_DMA in general is deprecated and should be replaced by appropiate
> > dma_alloc_coherent()
> 
> Um, no it shouldn't
> 
> All of the places where we use GFP_DMA are because we might be
> addressing an ISA card or other strange mask limited card (gated usually
> by unchecked_isa_dma).  

This means the majority of drivers will never use it? I don't 
claim to be a SCSI expert, but a lot of uses looked unchecked
to me from a quick look. That is very worrying because it's
only 16MB on x86 and 16MB run out quickly.

If it's never used I don't care that much, but it certainly
doesn't look like that.

Or do you only need it because pci_map_single() on i386 doesn't
do bouncing?  If yes we can probably fix that.

> However, what is wanted in every case is 
> ordinary memory, not coherent memory.  They can't simply be replaced
> with dma_alloc_coherent because
> 
> a) it will waste memory for platforms that only do it in page size
> multiples
> b) It will fail on platforms that can't do it at all.

GFP_DMA will also fail on a lot of platforms, so it's the same.

> Coherent memory is really only for device drivers to use in mailboxes
> and shared ring buffers.

Maybe we need a new interface then with a mask. The plan is anyways
get rid of GFP_DMA completely because it has lots of problems. And 
any user should declare the mask it really needs.

-Andi

Re: GFP_DMA use in SCSI midlayer

[James Bottomley]

On Sun, 2006-10-01 at 22:55 +0200, Andi Kleen wrote:
> This means the majority of drivers will never use it? I don't 
> claim to be a SCSI expert, but a lot of uses looked unchecked
> to me from a quick look. That is very worrying because it's
> only 16MB on x86 and 16MB run out quickly.

Yes, some cases it's used because the code can't be bothered to check to
see if it's required ... these cases can be audited and cleaned up.

> If it's never used I don't care that much, but it certainly
> doesn't look like that.
> 
> Or do you only need it because pci_map_single() on i386 doesn't
> do bouncing?  If yes we can probably fix that.

Heh, that would be the worst place to fix it. We usually have use
context for the allocation, so can sleep in the worst case.  We don't
have such context where we do pci_map_single().

> > However, what is wanted in every case is 
> > ordinary memory, not coherent memory.  They can't simply be replaced
> > with dma_alloc_coherent because
> > 
> > a) it will waste memory for platforms that only do it in page size
> > multiples
> > b) It will fail on platforms that can't do it at all.
> 
> GFP_DMA will also fail on a lot of platforms, so it's the same.

No, at the moment it succeeds on all of them (although at odd locations,
like on parisc GFP_DMA goes up to 4GB).


> > Coherent memory is really only for device drivers to use in
> mailboxes
> > and shared ring buffers.
> 
> Maybe we need a new interface then with a mask. The plan is anyways
> get rid of GFP_DMA completely because it has lots of problems. And 
> any user should declare the mask it really needs.

Well ... possibly; either that or just let the SG only paths take care
of it.

James

Re: GFP_DMA use in SCSI midlayer

[Andi Kleen]

On Monday 02 October 2006 16:52, James Bottomley wrote:
> On Sun, 2006-10-01 at 22:55 +0200, Andi Kleen wrote:
> > This means the majority of drivers will never use it? I don't 
> > claim to be a SCSI expert, but a lot of uses looked unchecked
> > to me from a quick look. That is very worrying because it's
> > only 16MB on x86 and 16MB run out quickly.
> 
> Yes, some cases it's used because the code can't be bothered to check to
> see if it's required ... these cases can be audited and cleaned up.

That would be useful.

> > If it's never used I don't care that much, but it certainly
> > doesn't look like that.
> > 
> > Or do you only need it because pci_map_single() on i386 doesn't
> > do bouncing?  If yes we can probably fix that.
> 
> Heh, that would be the worst place to fix it. We usually have use
> context for the allocation, so can sleep in the worst case.  We don't
> have such context where we do pci_map_single().

Ok then you'll likely want a new function that does this.

Do you always have a device or is a mask enough? 



> > > However, what is wanted in every case is 
> > > ordinary memory, not coherent memory.  They can't simply be replaced
> > > with dma_alloc_coherent because
> > > 
> > > a) it will waste memory for platforms that only do it in page size
> > > multiples
> > > b) It will fail on platforms that can't do it at all.
> > 
> > GFP_DMA will also fail on a lot of platforms, so it's the same.
> 
> No, at the moment it succeeds on all of them (although at odd locations,
> like on parisc GFP_DMA goes up to 4GB).

Not after Christoph's patchkit went in.

-Andi

Re: GFP_DMA use in SCSI midlayer

[Christoph Lameter]

On Mon, 2 Oct 2006, Andi Kleen wrote:

> > > GFP_DMA will also fail on a lot of platforms, so it's the same.
> > 
> > No, at the moment it succeeds on all of them (although at odd locations,
> > like on parisc GFP_DMA goes up to 4GB).
> 
> Not after Christoph's patchkit went in.

Well we can make it fail if you want to do that. Currently if you do not 
have CONFIG_ZONE_DMA set it simply ignores GFP_DMA because of the weird 
stuff in places like the SCSI layer. If you would fix it up then I could 
warn on GFP_DMA.

Re: GFP_DMA use in SCSI midlayer

[Mike Christie]

On Sun, 2006-10-01 at 15:40 -0500, James Bottomley wrote:
> On Sun, 2006-10-01 at 22:13 +0200, Andi Kleen wrote:
> > GFP_DMA in general is deprecated and should be replaced by appropiate
> > dma_alloc_coherent()
> 
> Um, no it shouldn't.
> 
> All of the places where we use GFP_DMA are because we might be
> addressing an ISA card or other strange mask limited card (gated usually
> by unchecked_isa_dma).  However, what is wanted in every case is
> ordinary memory, not coherent memory.  They can't simply be replaced
> with dma_alloc_coherent because
> 
> a) it will waste memory for platforms that only do it in page size
> multiples
> b) It will fail on platforms that can't do it at all.
> 
> Coherent memory is really only for device drivers to use in mailboxes
> and shared ring buffers.
> 
> > or similar.  And I can't imagine any modern systems still need them, and for 
> > the few still non CONFIG_BROKEN ISA drivers maybe some other way can be found?
> > And do they really require the mid layer data structures to be GFP_DMA too?
> 
> Well, it's the "or similar" that's still the problem.  All they need is
> ordinary memory which respects the device mask (then I can get rid of
> the unchecked_isa_dma flag as well).
> 
> One could argue, I suppose, that when we get every SCSI path to go via
> sg, then the block layer will bounce this for us, and we don't need to
> worry at all.


I think we might just need the blk_map_kern users now. For the async
execute I added the bounce code already and the block SG_IO has it
atleady. I think the blk_map_kern bounce code got dropped because we
thought the correct gfp_t would be passed in. But I think all we need is
the patch below and all the paths are take care of. The patch is not
tested. Patch was made against scsi-misc.

The last place that is sending non sg commands may just be md/dm-emc.c
but that is is just waiting on alasdair to take some patches that fix
that and a bunch of junk in there including adding bounce support. If
the patch below is ok though and dm-emc finally gets converted then it
will have sg and bonce buffer support.



Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index 9c3a06b..e58b43a 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -2533,6 +2533,7 @@ int blk_rq_map_kern(request_queue_t *q, 
 	rq->bio = rq->biotail = bio;
 	blk_rq_bio_prep(q, rq, bio);
 
+	blk_queue_bounce(q, &rq->bio);
 	rq->buffer = rq->data = NULL;
 	rq->data_len = len;
 	return 0;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
diff --git a/fs/bio.c b/fs/bio.c

Re: GFP_DMA use in SCSI midlayer

[Mike Christie]

Mike Christie wrote:
> On Sun, 2006-10-01 at 15:40 -0500, James Bottomley wrote:
>> On Sun, 2006-10-01 at 22:13 +0200, Andi Kleen wrote:
>>> GFP_DMA in general is deprecated and should be replaced by appropiate
>>> dma_alloc_coherent()
>> Um, no it shouldn't.
>>
>> All of the places where we use GFP_DMA are because we might be
>> addressing an ISA card or other strange mask limited card (gated usually
>> by unchecked_isa_dma).  However, what is wanted in every case is
>> ordinary memory, not coherent memory.  They can't simply be replaced
>> with dma_alloc_coherent because
>>
>> a) it will waste memory for platforms that only do it in page size
>> multiples
>> b) It will fail on platforms that can't do it at all.
>>
>> Coherent memory is really only for device drivers to use in mailboxes
>> and shared ring buffers.
>>
>>> or similar.  And I can't imagine any modern systems still need them, and for 
>>> the few still non CONFIG_BROKEN ISA drivers maybe some other way can be found?
>>> And do they really require the mid layer data structures to be GFP_DMA too?
>> Well, it's the "or similar" that's still the problem.  All they need is
>> ordinary memory which respects the device mask (then I can get rid of
>> the unchecked_isa_dma flag as well).
>>
>> One could argue, I suppose, that when we get every SCSI path to go via
>> sg, then the block layer will bounce this for us, and we don't need to
>> worry at all.
> 
> 
> I think we might just need the blk_map_kern users now. For the async

Oops, I just meant for the ULDs, scsi-ml, and block layer drivers. I did
not look at if/when the LLDs inject commands directly to their queuecommand.

> execute I added the bounce code already and the block SG_IO has it
> atleady. I think the blk_map_kern bounce code got dropped because we
> thought the correct gfp_t would be passed in. But I think all we need is
> the patch below and all the paths are take care of. The patch is not
> tested. Patch was made against scsi-misc.
> 
> The last place that is sending non sg commands may just be md/dm-emc.c
> but that is is just waiting on alasdair to take some patches that fix
> that and a bunch of junk in there including adding bounce support. If
> the patch below is ok though and dm-emc finally gets converted then it
> will have sg and bonce buffer support.
> 
> 
> 
> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
> 
> diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
> index 9c3a06b..e58b43a 100644
> --- a/block/ll_rw_blk.c
> +++ b/block/ll_rw_blk.c
> @@ -2533,6 +2533,7 @@ int blk_rq_map_kern(request_queue_t *q, 
>  	rq->bio = rq->biotail = bio;
>  	blk_rq_bio_prep(q, rq, bio);
>  
> +	blk_queue_bounce(q, &rq->bio);
>  	rq->buffer = rq->data = NULL;
>  	rq->data_len = len;
>  	return 0;
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> diff --git a/fs/bio.c b/fs/bio.c
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html