[PATCH] ieee1394: sbp2: lower block queue alignment requirement

[PATCH] ieee1394: sbp2: lower block queue alignment requirement

[Stefan Richter]

The old setting seems to be copy & paste from usb-storage.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---

I wonder, do we need this call to blk_queue_dma_alignment() at all?
Does the block layer perhaps provide sufficient alignment guarantees?


Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c	2007-01-14 18:40:08.000000000 +0100
+++ linux/drivers/ieee1394/sbp2.c	2007-01-14 19:30:42.000000000 +0100
@@ -2012,7 +2012,10 @@ static int sbp2scsi_slave_configure(stru
 {
 	struct sbp2_lu *lu = (struct sbp2_lu *)sdev->host->hostdata[0];
 
-	blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
+	/* Page table elements shall be octlet-aligned.
+	 * Everything else can be quadlet-aligned. */
+	blk_queue_dma_alignment(sdev->request_queue, 7);
+
 	sdev->use_10_for_rw = 1;
 
 	if (sdev->type == TYPE_ROM)

-- 
Stefan Richter
-=====-=-=== ---= -===-
http://arcgraph.de/sr/

Re: [PATCH] ieee1394: sbp2: lower block queue alignment requirement

[James Bottomley]

On Sun, 2007-01-14 at 19:45 +0100, Stefan Richter wrote:
> I wonder, do we need this call to blk_queue_dma_alignment() at all?
> Does the block layer perhaps provide sufficient alignment guarantees?

The block layer currently provides sector (512) byte alignment
guarantees.  However, there has been talk in SCSI of reducing that to
word (4) since that's what most intelligent PCI controllers can cope
with.  If you have any alignment constraints, they should be expressed
in the slave configure.

James

Re: [PATCH] ieee1394: sbp2: lower block queue alignment requirement

[Stefan Richter]

On 14 Jan, James Bottomley wrote:
> The block layer currently provides sector (512) byte alignment
> guarantees.  However, there has been talk in SCSI of reducing that to
> word (4) since that's what most intelligent PCI controllers can cope
> with.  If you have any alignment constraints, they should be expressed
> in the slave configure.

OK, thanks. I will keep the explicit setting of the mask then.
I actually think that everything should work just fine with 4 bytes
alignment, but the SBP-2 spec mentiones 8 bytes alignment for the S/G
tables.  So maybe device firmwares will expect it that way even though
IEEE 1394 doesn't impose such a restriction even on transfers with a
payload of a multiple of 8 bytes.
-- 
Stefan Richter
-=====-=-=== ---= -===-
http://arcgraph.de/sr/


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] ieee1394: sbp2: lower block queue alignment requirement

[Kristian Høgsberg]

On 1/14/07, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> On 14 Jan, James Bottomley wrote:
> > The block layer currently provides sector (512) byte alignment
> > guarantees.  However, there has been talk in SCSI of reducing that to
> > word (4) since that's what most intelligent PCI controllers can cope
> > with.  If you have any alignment constraints, they should be expressed
> > in the slave configure.
>
> OK, thanks. I will keep the explicit setting of the mask then.
> I actually think that everything should work just fine with 4 bytes
> alignment, but the SBP-2 spec mentiones 8 bytes alignment for the S/G
> tables.  So maybe device firmwares will expect it that way even though
> IEEE 1394 doesn't impose such a restriction even on transfers with a
> payload of a multiple of 8 bytes.

Do you have a reference to this 8 byte alignment requirement?  I
couldn't find it browsing through the standard.  Also, since the SBP-2
driver has to  convert the scatterlist to an SBP-2 page table, it can
just allocate the page table on a 8-byte boundary if that is required.
 The entries in the scatter table have no alignment restrictions from
the SBP-2 side.

By the way, is there a way to ask the SCSI stack to limit the size of
the entries in the scatterlist to 64k?  This would greatly simplify
the conversion to SBP-2 page tables, since this is the maximum size
these can hold.

cheers,
Kristian

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] ieee1394: sbp2: lower block queue alignment requirement

[Mike Christie]

Kristian Høgsberg wrote:
> On 1/14/07, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>> On 14 Jan, James Bottomley wrote:
>> > The block layer currently provides sector (512) byte alignment
>> > guarantees.  However, there has been talk in SCSI of reducing that to
>> > word (4) since that's what most intelligent PCI controllers can cope
>> > with.  If you have any alignment constraints, they should be expressed
>> > in the slave configure.
>>
>> OK, thanks. I will keep the explicit setting of the mask then.
>> I actually think that everything should work just fine with 4 bytes
>> alignment, but the SBP-2 spec mentiones 8 bytes alignment for the S/G
>> tables.  So maybe device firmwares will expect it that way even though
>> IEEE 1394 doesn't impose such a restriction even on transfers with a
>> payload of a multiple of 8 bytes.
> 
> Do you have a reference to this 8 byte alignment requirement?  I
> couldn't find it browsing through the standard.  Also, since the SBP-2
> driver has to  convert the scatterlist to an SBP-2 page table, it can
> just allocate the page table on a 8-byte boundary if that is required.
> The entries in the scatter table have no alignment restrictions from
> the SBP-2 side.
> 
> By the way, is there a way to ask the SCSI stack to limit the size of
> the entries in the scatterlist to 64k?  This would greatly simplify
> the conversion to SBP-2 page tables, since this is the maximum size
> these can hold.
> 

I think 64k is the limit already, but I think you can call
blk_queue_max_segment_size in your scsi_host_template->slave_configure
or slave_alloc to set the size to whatever you like.
-
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

Re: [PATCH] ieee1394: sbp2: lower block queue alignment requirement

[Stefan Richter]

On 15 Jan, Kristian Høgsberg wrote:
> On 1/14/07, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>> OK, thanks. I will keep the explicit setting of the mask then.
>> I actually think that everything should work just fine with 4 bytes
>> alignment, but the SBP-2 spec mentiones 8 bytes alignment for the S/G
>> tables.  So maybe device firmwares will expect it that way even though
>> IEEE 1394 doesn't impose such a restriction even on transfers with a
>> payload of a multiple of 8 bytes.
> 
> Do you have a reference to this 8 byte alignment requirement?  I
> couldn't find it browsing through the standard.

SBP-2 clause 5.2. It's also in SBP-3 even though there is no obvious
benefit for this rule.

> Also, since the SBP-2
> driver has to  convert the scatterlist to an SBP-2 page table, it can
> just allocate the page table on a 8-byte boundary if that is required.

Yes, after the discussion this occurred to me too but my energy reserves
were already too low to wrap this up.

>  The entries in the scatter table have no alignment restrictions from
> the SBP-2 side.
> 
> By the way, is there a way to ask the SCSI stack to limit the size of
> the entries in the scatterlist to 64k?  This would greatly simplify
> the conversion to SBP-2 page tables, since this is the maximum size
> these can hold.

Douglas has a very informative text on this:
http://sg.torque.net/sg/sg_io.html#mozTocId154063

| [...] since lk 2.6.16 another limit comes into play: the
| MAX_SEGMENT_SIZE define which is set to 64 KB. MAX_SEGMENT_SIZE is a
| default and can be overridden by the LLD calling
| blk_queue_max_segment_size().

Looks like we can yank out some cruft off sbp2.
-- 
Stefan Richter
-=====-=-=== ---= -====
http://arcgraph.de/sr/

-
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

Re: [PATCH] ieee1394: sbp2: lower block queue alignment requirement

[James Bottomley]

On Mon, 2007-01-15 at 21:02 +0100, Stefan Richter wrote:
> > By the way, is there a way to ask the SCSI stack to limit the size of
> > the entries in the scatterlist to 64k?  This would greatly simplify
> > the conversion to SBP-2 page tables, since this is the maximum size
> > these can hold.
> 
> Douglas has a very informative text on this:
> http://sg.torque.net/sg/sg_io.html#mozTocId154063
> 
> | [...] since lk 2.6.16 another limit comes into play: the
> | MAX_SEGMENT_SIZE define which is set to 64 KB. MAX_SEGMENT_SIZE is a
> | default and can be overridden by the LLD calling
> | blk_queue_max_segment_size().
> 
> Looks like we can yank out some cruft off sbp2.

Actually, there's one unfortunate case where Linux won't respect this:
an IOMMU that can do virtual merging.  This parameter is a block queue
parameter, so block will happily make sure the request segments obey it.
However, when you get to dma_map_rq() it doesn't see the segment limits,
so, if the iommu merges, you can end up with SG elements the other side
that violate this.  I've been meaning to do something about this for
ages (IDE is the other subsystem that has an absolute requirement for a
fixed maximum segment size) but never found an excuse to fix it.

James



-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] ieee1394: sbp2: lower block queue alignment requirement

[Stefan Richter]

On 14 Jan, James Bottomley wrote:
> The block layer currently provides sector (512) byte alignment
> guarantees.  However, there has been talk in SCSI of reducing that to
> word (4) since that's what most intelligent PCI controllers can cope
> with.  If you have any alignment constraints, they should be expressed
> in the slave configure.


From: Stefan Richter <stefanr@s5r6.in-berlin.de>
Subject: [PATCH update] ieee1394: sbp2: lower block queue alignment requirement

The old setting is copy & waste from usb-storage and doesn't apply to
sbp2.  There is only 4-byte alignment required for everything, except
for S/G table elements which have to be 8-byte aligned according to the
SBP-2 spec.  (They happen to be ____cacheline_aligned in our
implementation.  Whether that's good is another question.)

We now simply don't tune block queue alignment at all.  The default
alignment would surely never become anything else than a multiple of 4,
else tons of calls to blk_queue_dma_alignment would have to be added
everywhere in drivers/...

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Index: linux-2.6.20-rc5/drivers/ieee1394/sbp2.c
===================================================================
--- linux-2.6.20-rc5.orig/drivers/ieee1394/sbp2.c
+++ linux-2.6.20-rc5/drivers/ieee1394/sbp2.c
@@ -51,7 +51,6 @@
  * Grep for inline FIXME comments below.
  */
 
-#include <linux/blkdev.h>
 #include <linux/compiler.h>
 #include <linux/delay.h>
 #include <linux/device.h>
@@ -2012,7 +2011,6 @@ static int sbp2scsi_slave_configure(stru
 {
 	struct sbp2_lu *lu = (struct sbp2_lu *)sdev->host->hostdata[0];
 
-	blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
 	sdev->use_10_for_rw = 1;
 
 	if (sdev->type == TYPE_ROM)



-- 
Stefan Richter
-=====-=-=== ---= ==--=
http://arcgraph.de/sr/


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[PATCH] ieee1394: sbp2: remove unnecessary alignments of struct members

[Stefan Richter]

The members "dma_addr_t command_orb_dma" and "dma_addr_t sge_dma" of
sbp2.h::sbp2_command_info do not have to be aligned themselves --- only
the memory which they point to has to be.

The member "struct sbp2_command_orb command_orb" has to be aligned on
4 bytes boundary which is guaranteed because it contains u32 members.

The member "struct sbp2_unrestricted_page_table scatter_gather_element",
i.e. the SBP-2 s/g table, has to be aligned on 8 bytes boundary
according to the SBP-2 spec.  This is not a requirement for FireWire
controllers but could be expected by SBP-2 targets.

I see no need to align the members command_orb and
scatter_gather_element on CPU cacheline boundaries.  It could have
performance benefits, but on the other hand sbp2 has a somewhat wasteful
allocation scheme which should be optimized first before further tweaks
like cacheline alignments.  (E.g. don't always allocate SG_ALL s/g table
elements.)

Note, before as well as after the patch, the code relies on the
assumption that memory alignment in the virtual address space is
preserved in the physical address space after DMA mapping.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/sbp2.h |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-2.6.20-rc5/drivers/ieee1394/sbp2.h
===================================================================
--- linux-2.6.20-rc5.orig/drivers/ieee1394/sbp2.h
+++ linux-2.6.20-rc5/drivers/ieee1394/sbp2.h
@@ -250,15 +250,15 @@ enum sbp2_dma_types {
 /* Per SCSI command */
 struct sbp2_command_info {
 	struct list_head list;
-	struct sbp2_command_orb command_orb ____cacheline_aligned;
-	dma_addr_t command_orb_dma ____cacheline_aligned;
+	struct sbp2_command_orb command_orb;
+	dma_addr_t command_orb_dma;
 	struct scsi_cmnd *Current_SCpnt;
 	void (*Current_done)(struct scsi_cmnd *);
 
 	/* Also need s/g structure for each sbp2 command */
 	struct sbp2_unrestricted_page_table
-			scatter_gather_element[SG_ALL] ____cacheline_aligned;
-	dma_addr_t sge_dma ____cacheline_aligned;
+		scatter_gather_element[SG_ALL] __attribute__((aligned(8)));
+	dma_addr_t sge_dma;
 	void *sge_buffer;
 	dma_addr_t cmd_dma;
 	enum sbp2_dma_types dma_type;


-- 
Stefan Richter
-=====-=-=== --=- --=--
http://arcgraph.de/sr/

[PATCH] ieee1394: sbp2: enforce 32bit DMA mapping

[Stefan Richter]

In order to use OHCI physical DMA, all s/g elements, s/g tables, ORBs,
and response buffers have to reside within the first 4 GB of the
FireWire controller's physical address space.  Set the correct mask for
DMA mappings.

Also ensure that the memory allocated for command ORBs and s/g tables is
suitable for DMA.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Should I use GFP_DMA32 instead of GFP_DMA?


 drivers/ieee1394/sbp2.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Index: linux-2.6.20-rc5/drivers/ieee1394/sbp2.c
===================================================================
--- linux-2.6.20-rc5.orig/drivers/ieee1394/sbp2.c
+++ linux-2.6.20-rc5/drivers/ieee1394/sbp2.c
@@ -477,7 +477,7 @@ static int sbp2util_create_command_orb_p
 
 	spin_lock_irqsave(&lu->cmd_orb_lock, flags);
 	for (i = 0; i < orbs; i++) {
-		cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC);
+		cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC | GFP_DMA);
 		if (!cmd) {
 			spin_unlock_irqrestore(&lu->cmd_orb_lock, flags);
 			return -ENOMEM;
@@ -757,6 +757,11 @@ static struct sbp2_lu *sbp2_alloc_device
 			SBP2_ERR("failed to register lower 4GB address range");
 			goto failed_alloc;
 		}
+#else
+		if (dma_set_mask(hi->host->device.parent, DMA_32BIT_MASK)) {
+			SBP2_ERR("failed to set 4GB DMA mask");
+			goto failed_alloc;
+		}
 #endif
 	}
 


-- 
Stefan Richter
-=====-=-=== --=- --=--
http://arcgraph.de/sr/

Re: [PATCH] ieee1394: sbp2: enforce 32bit DMA mapping

[David Moore]

Yes, I think you should use GFP_DMA32 instead of GFP_DMA.  GFP_DMA
refers to just the first 16MB of memory.

I'd like to make a similar change in ieee1394/dma.c.  Right now, it
allocates large DMA regions with vmalloc().  One reason why that is bad
is because it often returns physical addresses larger than 32-bits,
causing the hardware to use bounce buffers (and is thus slow, and also
happened to expose a bug in swiotlb).

-David

On Sun, 2007-02-04 at 13:05 +0100, Stefan Richter wrote:
> In order to use OHCI physical DMA, all s/g elements, s/g tables, ORBs,
> and response buffers have to reside within the first 4 GB of the
> FireWire controller's physical address space.  Set the correct mask for
> DMA mappings.
> 
> Also ensure that the memory allocated for command ORBs and s/g tables is
> suitable for DMA.
> 
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> ---
> Should I use GFP_DMA32 instead of GFP_DMA?
> 
> 
>  drivers/ieee1394/sbp2.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.20-rc5/drivers/ieee1394/sbp2.c
> ===================================================================
> --- linux-2.6.20-rc5.orig/drivers/ieee1394/sbp2.c
> +++ linux-2.6.20-rc5/drivers/ieee1394/sbp2.c
> @@ -477,7 +477,7 @@ static int sbp2util_create_command_orb_p
>  
>  	spin_lock_irqsave(&lu->cmd_orb_lock, flags);
>  	for (i = 0; i < orbs; i++) {
> -		cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC);
> +		cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC | GFP_DMA);
>  		if (!cmd) {
>  			spin_unlock_irqrestore(&lu->cmd_orb_lock, flags);
>  			return -ENOMEM;
> @@ -757,6 +757,11 @@ static struct sbp2_lu *sbp2_alloc_device
>  			SBP2_ERR("failed to register lower 4GB address range");
>  			goto failed_alloc;
>  		}
> +#else
> +		if (dma_set_mask(hi->host->device.parent, DMA_32BIT_MASK)) {
> +			SBP2_ERR("failed to set 4GB DMA mask");
> +			goto failed_alloc;
> +		}
>  #endif
>  	}
>  
> 
> 


-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] ieee1394: sbp2: enforce 32bit DMA mapping

[Stefan Richter]

David Moore wrote:
> Yes, I think you should use GFP_DMA32 instead of GFP_DMA.  GFP_DMA
> refers to just the first 16MB of memory.

Yes, I forgot that GFP_DMA is for ISA DMA. However since GFP_DMA32 is
nowhere enforced except in allocations for coherent PCI DMA and in some
AGP code I think now I'll just drop that hunk.


From: Stefan Richter <stefanr@s5r6.in-berlin.de>
Subject: ieee1394: sbp2: enforce 32bit DMA mapping

In order to use OHCI-1394 physical DMA, all s/g elements, s/g tables,
ORBs, and response buffers have to reside within the first 4 GB of the
FireWire controller's physical address space.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Index: linux-2.6.20-rc5/drivers/ieee1394/sbp2.c
===================================================================
--- linux-2.6.20-rc5.orig/drivers/ieee1394/sbp2.c
+++ linux-2.6.20-rc5/drivers/ieee1394/sbp2.c
@@ -757,6 +757,11 @@ static struct sbp2_lu *sbp2_alloc_device
 			SBP2_ERR("failed to register lower 4GB address range");
 			goto failed_alloc;
 		}
+#else
+		if (dma_set_mask(hi->host->device.parent, DMA_32BIT_MASK)) {
+			SBP2_ERR("failed to set 4GB DMA mask");
+			goto failed_alloc;
+		}
 #endif
 	}
 

-- 
Stefan Richter
-=====-=-=== --=- --=--
http://arcgraph.de/sr/


-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642