relax scsi dma alignment

PATCH In review James Bottomley 3 participants 7 messages Dec 31, 2007 – Jan 1, 2008

Subsystems (6)

SCSI USB Block layer libata +2 more subsystems

lore

Share & tools

lore

James Bottomley

author maintainer

2007-12-31 22:43 UTC #0 lore raw

PATCH 2/2 relax scsi dma alignment

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

Signed-off-by James Bottomley <[email protected]>

---

This patch depends on the prior patch:

block: Introduce new blk_queue_update_dma_alignment interface

drivers/ata/libata-scsi.c +4 -1

index 571fba5..db627a4 100644
@@ -824,6 +824,9 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
 	 * requests.
 	 */
 	sdev->max_device_blocked = 1;
+
+	/* set the min alignment */
+	blk_queue_update_dma_alignment(sdev->request_queue, ATA_DMA_PAD_MASK);
 }
 
 static void ata_scsi_dev_config(struct scsi_device *sdev,

@@ -868,7 +871,7 @@ int ata_scsi_slave_config(struct scsi_device *sdev)
 	if (dev)
 		ata_scsi_dev_config(sdev, dev);
 
-	return 0;	/* scsi layer doesn't check return value, sigh */
+	return 0;
 }
 
 /**

drivers/firewire/fw-sbp2.c +6 -0

index 624ff3e..c2169d2 100644
@@ -1238,6 +1238,12 @@ static int sbp2_scsi_slave_alloc(struct scsi_device *sdev)
 
 	sdev->allow_restart = 1;
 
+	/*
+	 * Update the dma alignment (minimum alignment requirements for
+	 * start and end of DMA transfers) to be a sector
+	 */
+	blk_queue_update_dma_alignment(sdev->request_queue, 511);
+
 	if (lu->tgt->workarounds & SBP2_WORKAROUND_INQUIRY_36)
 		sdev->inquiry_len = 36;

drivers/ieee1394/sbp2.c +6 -0

index b83d254..1eda11a 100644
@@ -1963,6 +1963,12 @@ static int sbp2scsi_slave_alloc(struct scsi_device *sdev)
 	lu->sdev = sdev;
 	sdev->allow_restart = 1;
 
+	/*
+	 * Update the dma alignment (minimum alignment requirements for
+	 * start and end of DMA transfers) to be a sector
+	 */
+	blk_queue_update_dma_alignment(sdev->request_queue, 511);
+
 	if (lu->workarounds & SBP2_WORKAROUND_INQUIRY_36)
 		sdev->inquiry_len = 36;
 	return 0;

drivers/scsi/scsi_lib.c +8 -0

index d1a4671..0aa5296 100644
@@ -1667,6 +1667,14 @@ struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
 
 	if (!shost->use_clustering)
 		clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
+
+	/*
+	 * set a reasonable default alignment on word boundaries: the
+	 * host and device may alter it using
+	 * blk_queue_update_dma_alignment() later.
+	 */
+	blk_queue_dma_alignment(q, 0x03);
+
 	return q;
 }
 EXPORT_SYMBOL(__scsi_alloc_queue);

drivers/usb/storage/scsiglue.c +6 -0

index 7c9593b..39c03f9 100644
@@ -82,6 +82,12 @@ static int slave_alloc (struct scsi_device *sdev)
 	sdev->inquiry_len = 36;
 
 	/*
+	 * Update the dma alignment (minimum alignment requirements for
+	 * start and end of DMA transfers) to be a sector
+	 */
+	blk_queue_update_dma_alignment(sdev->request_queue, 511);
+
+	/*
 	 * The UFI spec treates the Peripheral Qualifier bits in an
 	 * INQUIRY result as reserved and requires devices to set them
 	 * to 0.  However the SCSI spec requires these bits to be set

Alan Stern

maintainer

2007-12-31 22:57 UTC #1 lore raw

Quoted 8 lines · James Bottomley

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

...

Quoted 7 lines

drivers/usb/storage/scsiglue.c +0 -0

index 7c9593b..39c03f9 100644
@@ -82,6 +82,12 @@ static int slave_alloc (struct scsi_device *sdev)
 	sdev->inquiry_len = 36;

drivers/usb/storage/scsiglue.c static int slave_alloc (struct scsi_device *sdev) · line 93

Quoted 10 lines

drivers/usb/storage/scsiglue.c +6 -0

 	/*
+	 * Update the dma alignment (minimum alignment requirements for
+	 * start and end of DMA transfers) to be a sector
+	 */
+	blk_queue_update_dma_alignment(sdev->request_queue, 511);
+
+	/*
 	 * The UFI spec treates the Peripheral Qualifier bits in an
 	 * INQUIRY result as reserved and requires devices to set them
 	 * to 0.  However the SCSI spec requires these bits to be set

It would be better to move the existing code from the start of slave_configure() up into slave_alloc(). Otherwise it's fine.

Alan Stern

James Bottomley

author maintainer

2008-01-01 16:00 UTC #2 lore raw

Quoted 6 lines · Alan Stern

Quoted 5 lines · James Bottomley
drivers/usb/storage/scsiglue.c +0 -0
@@ -82,6 +82,12 @@ static int slave_alloc (struct scsi_device *sdev)
 	sdev->inquiry_len = 36;
 

Quoted 13 lines

drivers/usb/storage/scsiglue.c static int slave_alloc (struct scsi_device *sdev) · line 93
Quoted 10 lines
drivers/usb/storage/scsiglue.c +6 -0
 	/*
+	 * Update the dma alignment (minimum alignment requirements for
+	 * start and end of DMA transfers) to be a sector
+	 */
+	blk_queue_update_dma_alignment(sdev->request_queue, 511);
+
+	/*
 	 * The UFI spec treates the Peripheral Qualifier bits in an
 	 * INQUIRY result as reserved and requires devices to set them
 	 * to 0.  However the SCSI spec requires these bits to be set
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

drivers/ata/libata-scsi.c +4 -1

index fed5308..25c9470 100644
@@ -824,6 +824,9 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
 	 * requests.
 	 */
 	sdev->max_device_blocked = 1;
+
+	/* set the min alignment */
+	blk_queue_update_dma_alignment(sdev->request_queue, ATA_DMA_PAD_MASK);
 }
 
 static void ata_scsi_dev_config(struct scsi_device *sdev,

@@ -868,7 +871,7 @@ int ata_scsi_slave_config(struct scsi_device *sdev)
 	if (dev)
 		ata_scsi_dev_config(sdev, dev);
 
-	return 0;	/* scsi layer doesn't check return value, sigh */
+	return 0;
 }
 
 /**

drivers/firewire/fw-sbp2.c +6 -0

index 624ff3e..c2169d2 100644
@@ -1238,6 +1238,12 @@ static int sbp2_scsi_slave_alloc(struct scsi_device *sdev)
 
 	sdev->allow_restart = 1;
 
+	/*
+	 * Update the dma alignment (minimum alignment requirements for
+	 * start and end of DMA transfers) to be a sector
+	 */
+	blk_queue_update_dma_alignment(sdev->request_queue, 511);
+
 	if (lu->tgt->workarounds & SBP2_WORKAROUND_INQUIRY_36)
 		sdev->inquiry_len = 36;

drivers/ieee1394/sbp2.c +6 -0

index b83d254..1eda11a 100644
@@ -1963,6 +1963,12 @@ static int sbp2scsi_slave_alloc(struct scsi_device *sdev)
 	lu->sdev = sdev;
 	sdev->allow_restart = 1;
 
+	/*
+	 * Update the dma alignment (minimum alignment requirements for
+	 * start and end of DMA transfers) to be a sector
+	 */
+	blk_queue_update_dma_alignment(sdev->request_queue, 511);
+
 	if (lu->workarounds & SBP2_WORKAROUND_INQUIRY_36)
 		sdev->inquiry_len = 36;
 	return 0;

drivers/scsi/scsi_lib.c +8 -0

index d1a4671..0aa5296 100644
@@ -1667,6 +1667,14 @@ struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
 
 	if (!shost->use_clustering)
 		clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
+
+	/*
+	 * set a reasonable default alignment on word boundaries: the
+	 * host and device may alter it using
+	 * blk_queue_update_dma_alignment() later.
+	 */
+	blk_queue_dma_alignment(q, 0x03);
+
 	return q;
 }
 EXPORT_SYMBOL(__scsi_alloc_queue);

drivers/usb/storage/scsiglue.c +10 ~~-10~~

index 7c9593b..dd8b13e 100644
@@ -81,6 +81,16 @@ static int slave_alloc (struct scsi_device *sdev)
 	 */
 	sdev->inquiry_len = 36;
 
+	/* Scatter-gather buffers (all but the last) must have a length
+	 * divisible by the bulk maxpacket size.  Otherwise a data packet
+	 * would end up being short, causing a premature end to the data
+	 * transfer.  Since high-speed bulk pipes have a maxpacket size
+	 * of 512, we'll use that as the scsi device queue's DMA alignment
+	 * mask.  Guaranteeing proper alignment of the first buffer will
+	 * have the desired effect because, except at the beginning and
+	 * the end, scatter-gather buffers follow page boundaries. */
+	blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
+
 	/*
 	 * The UFI spec treates the Peripheral Qualifier bits in an
 	 * INQUIRY result as reserved and requires devices to set them

@@ -100,16 +110,6 @@ static int slave_configure(struct scsi_device *sdev)
 {
 	struct us_data *us = host_to_us(sdev->host);
 
-	/* Scatter-gather buffers (all but the last) must have a length
-	 * divisible by the bulk maxpacket size.  Otherwise a data packet
-	 * would end up being short, causing a premature end to the data
-	 * transfer.  Since high-speed bulk pipes have a maxpacket size
-	 * of 512, we'll use that as the scsi device queue's DMA alignment
-	 * mask.  Guaranteeing proper alignment of the first buffer will
-	 * have the desired effect because, except at the beginning and
-	 * the end, scatter-gather buffers follow page boundaries. */
-	blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
-
 	/* Many devices have trouble transfering more than 32KB at a time,
 	 * while others have trouble with more than 64K. At this time we
 	 * are limiting both to 32K (64 sectores).

Alan Stern

maintainer Acked-by

2008-01-01 16:35 UTC #3 lore raw

Quoted 11 lines · James Bottomley

Quoted 2 lines · Alan Stern
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]>

Pete Wyckoff

2008-01-01 16:20 UTC #4 lore raw

In reply to James Bottomley

[email protected] wrote on Mon, 31 Dec 2007 16:43 -0600:

Quoted 8 lines

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

---

Quoted 1 line

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

include/linux/blkdev.h +1 -6

index aa3090c..eea31c2 100644
@@ -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 */

Signature · 1 line

1.5.3.6

James Bottomley

author maintainer

2008-01-01 16:27 UTC #5 lore raw

Quoted 49 lines · Pete Wyckoff

[email protected] wrote on Mon, 31 Dec 2007 16:43 -0600:
Quoted 8 lines
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
---
Quoted 1 line
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(-)
include/linux/blkdev.h +0 -0
index aa3090c..eea31c2 100644
@@ -860,12 +860,7 @@ static inline int bdev_hardsect_size(struct block_device *bdev)
 
 static inline int queue_dma_alignment(struct request_queue *q)
 {

include/linux/blkdev.h static inline int bdev_hardsect_size(struct block_device *bdev) · line 866

Quoted 10 lines

include/linux/blkdev.h +1 -6

-	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

Pete Wyckoff

2008-01-01 17:15 UTC #6 lore raw

[email protected] wrote on Tue, 01 Jan 2008 10:27 -0600:

Quoted 23 lines · Pete Wyckoff

Quoted 22 lines
[email protected] wrote on Mon, 31 Dec 2007 16:43 -0600:
Quoted 8 lines
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.

[..]

Quoted 31 lines

Quoted 31 lines
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(-)
include/linux/blkdev.h +1 -6
index aa3090c..eea31c2 100644
@@ -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;
 }
 

Quoted 5 lines

include/linux/blkdev.h static inline int bdev_hardsect_size(struct block_device *bdev) · line 866
Quoted 1 line
include/linux/blkdev.h +0 -0
 /* 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 (buffer&1) or (length&1).

		-- Pete