Re: [PATCH 2/2] relax scsi dma alignment

[Pete Wyckoff]

[EMAIL PROTECTED] wrote on Mon, 31 Dec 2007 16:43 -0600:
 This patch relaxes the default SCSI DMA alignment from 512 bytes to 4
 bytes.  I remember from previous discussions that usb and firewire have
 sector size alignment requirements, so I upped their alignments in the
 respective slave allocs.
 
 The reason for doing this is so that we don't get such a huge amount of
 copy overhead in bio_copy_user() for udev.  (basically all inquiries it
 issues can now be directly mapped).

Great change.  Here's another patch.  It allows a queue
dma_alignment of 0 bytes, for drivers that can move data
at byte granularity.

Two drivers try to turn off DMA alignment restrictions by
setting the dma_alignment to zero:

drivers/scsi/iscsi_tcp.c: blk_queue_dma_alignment(sdev-request_queue, 0);
drivers/scsi/scsi_tgt_lib.c: blk_queue_dma_alignment(q, 0);

But they end up doing copies due to the way that queue_dma_alignment()
returns 511 in this case.

-- Pete

---

From 90ee6d61ad71a024815eee2b416edb40c6b01256 Mon Sep 17 00:00:00 2001
From: Pete Wyckoff [EMAIL PROTECTED]
Date: Tue, 1 Jan 2008 10:23:02 -0500
Subject: [PATCH] block: allow queue dma_alignment of zero

Let queue_dma_alignment return 0 if it was specifically set to 0.
This permits devices with no particular alignment restrictions to
use arbitrary user space buffers without copying.

Signed-off-by: Pete Wyckoff [EMAIL PROTECTED]
---
 include/linux/blkdev.h |7 +--
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index aa3090c..eea31c2 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -860,12 +860,7 @@ static inline int bdev_hardsect_size(struct block_device 
*bdev)
 
 static inline int queue_dma_alignment(struct request_queue *q)
 {
-   int retval = 511;
-
-   if (q  q-dma_alignment)
-   retval = q-dma_alignment;
-
-   return retval;
+   return q ? q-dma_alignment : 511;
 }
 
 /* assumes size  256 */
-- 
1.5.3.6

-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] relax scsi dma alignment

[James Bottomley]

On Tue, 2008-01-01 at 11:08 -0500, Pete Wyckoff wrote:
 [EMAIL PROTECTED] wrote on Mon, 31 Dec 2007 16:43 -0600:
  This patch relaxes the default SCSI DMA alignment from 512 bytes to 4
  bytes.  I remember from previous discussions that usb and firewire have
  sector size alignment requirements, so I upped their alignments in the
  respective slave allocs.
  
  The reason for doing this is so that we don't get such a huge amount of
  copy overhead in bio_copy_user() for udev.  (basically all inquiries it
  issues can now be directly mapped).
 
 Great change.  Here's another patch.  It allows a queue
 dma_alignment of 0 bytes, for drivers that can move data
 at byte granularity.
 
 Two drivers try to turn off DMA alignment restrictions by
 setting the dma_alignment to zero:
 
 drivers/scsi/iscsi_tcp.c: blk_queue_dma_alignment(sdev-request_queue, 0);
 drivers/scsi/scsi_tgt_lib.c: blk_queue_dma_alignment(q, 0);
 
 But they end up doing copies due to the way that queue_dma_alignment()
 returns 511 in this case.
 
   -- Pete
 
 ---
 
 From 90ee6d61ad71a024815eee2b416edb40c6b01256 Mon Sep 17 00:00:00 2001
 From: Pete Wyckoff [EMAIL PROTECTED]
 Date: Tue, 1 Jan 2008 10:23:02 -0500
 Subject: [PATCH] block: allow queue dma_alignment of zero
 
 Let queue_dma_alignment return 0 if it was specifically set to 0.
 This permits devices with no particular alignment restrictions to
 use arbitrary user space buffers without copying.
 
 Signed-off-by: Pete Wyckoff [EMAIL PROTECTED]
 ---
  include/linux/blkdev.h |7 +--
  1 files changed, 1 insertions(+), 6 deletions(-)
 
 diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
 index aa3090c..eea31c2 100644
 --- a/include/linux/blkdev.h
 +++ b/include/linux/blkdev.h
 @@ -860,12 +860,7 @@ static inline int bdev_hardsect_size(struct block_device 
 *bdev)
  
  static inline int queue_dma_alignment(struct request_queue *q)
  {
 - int retval = 511;
 -
 - if (q  q-dma_alignment)
 - retval = q-dma_alignment;
 -
 - return retval;
 + return q ? q-dma_alignment : 511;
  }
  
  /* assumes size  256 */

This is certainly a possible patch.  What assurance do you have that it
doesn't upset block devices who set zero assuming they get the default
alignment?

James


-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] relax scsi dma alignment

[Alan Stern]

On Tue, 1 Jan 2008, James Bottomley wrote:

 On Mon, 2007-12-31 at 17:57 -0500, Alan Stern wrote:

  It would be better to move the existing code from the start of
  slave_configure() up into slave_alloc().  Otherwise it's fine.
 
 Um, yes, you're right, I didn't notice there was an existing one ... I
 just assumed you were relying on the SCSI default.
 
 Here's an updated patch.
 
 James

If I had put the alignment call in the correct place to begin with, 
this change wouldn't be needed.  Ah well...

Acked-by: Alan Stern [EMAIL PROTECTED]

-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] relax scsi dma alignment

[Pete Wyckoff]

[EMAIL PROTECTED] wrote on Tue, 01 Jan 2008 10:27 -0600:
 On Tue, 2008-01-01 at 11:08 -0500, Pete Wyckoff wrote:
  [EMAIL PROTECTED] wrote on Mon, 31 Dec 2007 16:43 -0600:
   This patch relaxes the default SCSI DMA alignment from 512 bytes to 4
   bytes.  I remember from previous discussions that usb and firewire have
   sector size alignment requirements, so I upped their alignments in the
   respective slave allocs.
   
   The reason for doing this is so that we don't get such a huge amount of
   copy overhead in bio_copy_user() for udev.  (basically all inquiries it
   issues can now be directly mapped).
  
  Great change.  Here's another patch.  It allows a queue
  dma_alignment of 0 bytes, for drivers that can move data
  at byte granularity.
  
  Two drivers try to turn off DMA alignment restrictions by
  setting the dma_alignment to zero:
  
  drivers/scsi/iscsi_tcp.c: blk_queue_dma_alignment(sdev-request_queue, 
  0);
  drivers/scsi/scsi_tgt_lib.c: blk_queue_dma_alignment(q, 0);
  
  But they end up doing copies due to the way that queue_dma_alignment()
  returns 511 in this case.
[..]
  From 90ee6d61ad71a024815eee2b416edb40c6b01256 Mon Sep 17 00:00:00 2001
  From: Pete Wyckoff [EMAIL PROTECTED]
  Date: Tue, 1 Jan 2008 10:23:02 -0500
  Subject: [PATCH] block: allow queue dma_alignment of zero
  
  Let queue_dma_alignment return 0 if it was specifically set to 0.
  This permits devices with no particular alignment restrictions to
  use arbitrary user space buffers without copying.
  
  Signed-off-by: Pete Wyckoff [EMAIL PROTECTED]
  ---
   include/linux/blkdev.h |7 +--
   1 files changed, 1 insertions(+), 6 deletions(-)
  
  diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
  index aa3090c..eea31c2 100644
  --- a/include/linux/blkdev.h
  +++ b/include/linux/blkdev.h
  @@ -860,12 +860,7 @@ static inline int bdev_hardsect_size(struct 
  block_device *bdev)
   
   static inline int queue_dma_alignment(struct request_queue *q)
   {
  -   int retval = 511;
  -
  -   if (q  q-dma_alignment)
  -   retval = q-dma_alignment;
  -
  -   return retval;
  +   return q ? q-dma_alignment : 511;
   }
   
   /* assumes size  256 */
 
 This is certainly a possible patch.  What assurance do you have that it
 doesn't upset block devices who set zero assuming they get the default
 alignment?

Code inspection of the initialization and use of this field.  I hope
anybody who sees a mistake here will point it out.

1. Initialization

Most users call blk_init_queue{,_node} to alloc and initialize a new
request_queue.  This leads to initialization of dma_alignment to 511
in blk_queue_make_request().

The rest of the callers use blk_alloc_queue{,_node}.  Most of those
call blk_queue_make_request() with a custom make_request_fn a few
lines later, similarly causing dma_alignment to be initialized to
non-zero.  The loop and pktcdvd drivers require a bit of reading to
convince oneself, but also can be seen to call
blk_queue_make_request() before using the queue.

2. Use of blk_queue_dma_alignment() to set, queue_dma_alignment() and
   -dma_alignment to get

Inspection of the 20-odd spots that match dma_alignment shows that
none of them set zero to expect the default.


Definitely a valid concern, but it seems to be a safe change, at
least for in-tree users.  Do you think a new request_queue flag for
zero-aware drivers and a WARN_ON that checks for zero and !zero_aware
would be worthwhile?

Without this change, we can go as low as two-byte alignment on
buffer start and length by setting dma_alignment to 1, but will do a
full copy if (buffer1) or (length1).

-- Pete

-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html