iscsi iser limits

PATCH In review Pete Wyckoff 5 participants 26 messages Feb 12, 2008 – May 12, 2008

Subsystems (3)

SCSI InfiniBand iSCSI extensions for RDMA (iSER) initiator

lore

Share & tools

lore

Pete Wyckoff

author

2008-02-12 20:52 UTC #0 lore raw

cover iscsi iser limits

These three patches remove various block device limits to the iSER iSCSI transport. I guess in theory they should go in via the IB tree, although ACKs from the iSCSI side would be welcome.

The first two look quite similar to some TCP iSCSI patches that Mike applied a while back:

d1d81c01f4bdd50577d9f89aa4a8e6344f63aa70
[SCSI] iscsi_tcp: remove DMA alignment restriction

8231f0eddbe425cc3b54f2d723bb03531925272e
[SCSI] iscsi_tcp: increase max_sectors

The third fixes a problem with mapping an unaligned 512 kB buffer and takes the opportunity to increase the FMR mapping limit to 1 MB, as is done in SRP.

		-- Pete

Pete Wyckoff

author

2008-02-12 20:54 UTC #1 lore raw

PATCH 1/3 iscsi iser: remove DMA restrictions

iscsi_iser does not have any hardware DMA restrictions. Add a slave_configure function to remove any DMA alignment restriction, allowing the use of direct IO from arbitrary offsets within a page. Also disable page bouncing; iser has no restrictions on which pages it can address.

Signed-off-by Pete Wyckoff <[email protected]>

---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

drivers/infiniband/ulp/iser/iscsi_iser.c +8 -0

index be1b9fb..1b272a6 100644
@@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
 	iser_conn_terminate(ib_conn);
 }
 
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)
+{
+	blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_ANY);
+	blk_queue_dma_alignment(sdev->request_queue, 0);
+	return 0;
+}
+
 static struct scsi_host_template iscsi_iser_sht = {
 	.module                 = THIS_MODULE,
 	.name                   = "iSCSI Initiator over iSER, v." DRV_VER,

@@ -556,6 +563,7 @@ static struct scsi_host_template iscsi_iser_sht = {
 	.eh_device_reset_handler= iscsi_eh_device_reset,
 	.eh_host_reset_handler	= iscsi_eh_host_reset,
 	.use_clustering         = DISABLE_CLUSTERING,
+	.slave_configure	= iscsi_iser_slave_configure,
 	.proc_name              = "iscsi_iser",
 	.this_id                = -1,
 };

Signature · 1 line

1.5.3.8

James Bottomley

maintainer

2008-02-12 21:10 UTC #2 lore raw

Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

Quoted 16 lines · Pete Wyckoff

iscsi_iser does not have any hardware DMA restrictions. Add a slave_configure function to remove any DMA alignment restriction, allowing the use of direct IO from arbitrary offsets within a page. Also disable page bouncing; iser has no restrictions on which pages it can address.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
index be1b9fb..1b272a6 100644

drivers/infiniband/ulp/iser/iscsi_iser.c iscsi_iser_ep_disconnect(__u64 ep_handle) · line 548

Quoted 6 lines

drivers/infiniband/ulp/iser/iscsi_iser.c +3 -0

@@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
 	iser_conn_terminate(ib_conn);
 }
 
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)
+{
+	blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_ANY);

You really don't want to do this. That signals to the block layer that we have an iommu, although it's practically the same thing as a 64 bit DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be mismatched.

James

Pete Wyckoff

author

2008-02-12 21:46 UTC #3 lore raw

Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

[email protected] wrote on Tue, 12 Feb 2008 15:10 -0600:

Quoted 20 lines · Pete Wyckoff

Quoted 19 lines
iscsi_iser does not have any hardware DMA restrictions. Add a slave_configure function to remove any DMA alignment restriction, allowing the use of direct IO from arbitrary offsets within a page. Also disable page bouncing; iser has no restrictions on which pages it can address.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
index be1b9fb..1b272a6 100644
@@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
 	iser_conn_terminate(ib_conn);
 }
 

Quoted 9 lines

drivers/infiniband/ulp/iser/iscsi_iser.c iscsi_iser_ep_disconnect(__u64 ep_handle) · line 548
Quoted 3 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +3 -0
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)
+{
+	blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_ANY);
You really don't want to do this. That signals to the block layer that we have an iommu, although it's practically the same thing as a 64 bit DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be mismatched.

Oh. I decided to add that line for symmetry with TCP, and was convinced by the arguments here:

commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56
Author: Mike Christie <[email protected]>
Date: Thu Jul 26 12:46:47 2007 -0500

[SCSI] iscsi_tcp: Turn off bounce buffers

It was found by LSI that on setups with large amounts of memory we were bouncing buffers when we did not need to. If the iscsi tcp code touches the data buffer (or a helper does),
it will kmap the buffer. iscsi_tcp also does not interact with hardware, so it does not have any hw dma restrictions. This patch sets the bounce buffer settings for our device queue so buffers should not be bounced because of a driver limit.

I don't see a convenient place to callback into particular iscsi devices to set the DMA mask per-host. It has to go on the shost_gendev, right?, but only for TCP and iSER, not qla4xxx, which handles its DMA mask during device probe.

		-- Pete

James Bottomley

maintainer

2008-02-12 21:57 UTC #4 lore raw

Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

Quoted 48 lines · Pete Wyckoff

[email protected] wrote on Tue, 12 Feb 2008 15:10 -0600:
Quoted 20 lines · Pete Wyckoff
Quoted 19 lines
iscsi_iser does not have any hardware DMA restrictions. Add a slave_configure function to remove any DMA alignment restriction, allowing the use of direct IO from arbitrary offsets within a page. Also disable page bouncing; iser has no restrictions on which pages it can address.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
index be1b9fb..1b272a6 100644
@@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
 	iser_conn_terminate(ib_conn);
 }
 
Quoted 9 lines
drivers/infiniband/ulp/iser/iscsi_iser.c iscsi_iser_ep_disconnect(__u64 ep_handle) · line 548
Quoted 3 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +3 -0
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)
+{
+	blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_ANY);
You really don't want to do this. That signals to the block layer that we have an iommu, although it's practically the same thing as a 64 bit DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be mismatched.
Oh. I decided to add that line for symmetry with TCP, and was convinced by the arguments here:
commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56
Author: Mike Christie <[email protected]>
Date: Thu Jul 26 12:46:47 2007 -0500
[SCSI] iscsi_tcp: Turn off bounce buffers
It was found by LSI that on setups with large amounts of memory we were bouncing buffers when we did not need to. If the iscsi tcp code touches the data buffer (or a helper does),
it will kmap the buffer. iscsi_tcp also does not interact with hardware, so it does not have any hw dma restrictions. This patch sets the bounce buffer settings for our device queue so buffers should not be bounced because of a driver limit.

Quoted 4 lines

I don't see a convenient place to callback into particular iscsi devices to set the DMA mask per-host. It has to go on the shost_gendev, right?, but only for TCP and iSER, not qla4xxx, which handles its DMA mask during device probe.

You should be taking your mask from the underlying infiniband device as part of the setup, shouldn't you?

James

Pete Wyckoff

author

2008-02-13 19:59 UTC #5 lore raw

Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

[email protected] wrote on Tue, 12 Feb 2008 15:57 -0600:

Quoted 49 lines · Pete Wyckoff

Quoted 48 lines
[email protected] wrote on Tue, 12 Feb 2008 15:10 -0600:
Quoted 20 lines · Pete Wyckoff
Quoted 19 lines
iscsi_iser does not have any hardware DMA restrictions. Add a slave_configure function to remove any DMA alignment restriction, allowing the use of direct IO from arbitrary offsets within a page. Also disable page bouncing; iser has no restrictions on which pages it can address.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
index be1b9fb..1b272a6 100644
@@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
 	iser_conn_terminate(ib_conn);
 }
 
Quoted 9 lines
drivers/infiniband/ulp/iser/iscsi_iser.c iscsi_iser_ep_disconnect(__u64 ep_handle) · line 548
Quoted 3 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +3 -0
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)
+{
+	blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_ANY);
You really don't want to do this. That signals to the block layer that we have an iommu, although it's practically the same thing as a 64 bit DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be mismatched.
Oh. I decided to add that line for symmetry with TCP, and was convinced by the arguments here:
commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56
Author: Mike Christie <[email protected]>
Date: Thu Jul 26 12:46:47 2007 -0500
[SCSI] iscsi_tcp: Turn off bounce buffers
It was found by LSI that on setups with large amounts of memory we were bouncing buffers when we did not need to. If the iscsi tcp code touches the data buffer (or a helper does),
it will kmap the buffer. iscsi_tcp also does not interact with hardware, so it does not have any hw dma restrictions. This patch sets the bounce buffer settings for our device queue so buffers should not be bounced because of a driver limit.

Quoted 7 lines

Quoted 4 lines
I don't see a convenient place to callback into particular iscsi devices to set the DMA mask per-host. It has to go on the shost_gendev, right?, but only for TCP and iSER, not qla4xxx, which handles its DMA mask during device probe.
You should be taking your mask from the underlying infiniband device as part of the setup, shouldn't you?

I think you're right about this. All the existing IB HW tries to set a 64-bit dma mask, but that's no reason to disable the mechanism entirely in iser. I'll remove that line that disables bouncing in my patch. Perhaps Mike will know if the iscsi_tcp usage is still appropriate.

		-- Pete

Pete Wyckoff

author

2008-02-14 21:10 UTC #6 lore raw

PATCH v2 1/3 iscsi iser: remove DMA alignment restriction

Thanks to James pointing out the problems with the BLK_BOUNCE_ANY for IB devices, this revised patch contains only the DMA alignment fix for iSER.

Mike, can you take care of this and the other two patches in the series:

[PATCH 2/3] iscsi iser: increase max_sectors
[PATCH 3/3] iscsi iser: increase sg_tablesize

-- Pete

Quoted 1 line

From 255e73b67ec1458af18395981fddebdc958e8fe9 Mon Sep 17 00:00:00 2001

From: Pete Wyckoff <[email protected]>
Date: Thu, 14 Feb 2008 16:09:27 -0500
Subject: [PATCH] iscsi iser: remove DMA alignment restriction

iscsi_iser does not require any particular DMA aligement requirement. Add a slave_configure function to set the alignment to zero, allowing the use of direct IO from arbitrary offsets within a page.

Signed-off-by Pete Wyckoff <[email protected]>

---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

drivers/infiniband/ulp/iser/iscsi_iser.c +7 -0

index be1b9fb..313f102 100644
@@ -543,6 +543,12 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
 	iser_conn_terminate(ib_conn);
 }
 
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)
+{
+	blk_queue_dma_alignment(sdev->request_queue, 0);
+	return 0;
+}
+
 static struct scsi_host_template iscsi_iser_sht = {
 	.module                 = THIS_MODULE,
 	.name                   = "iSCSI Initiator over iSER, v." DRV_VER,

@@ -556,6 +562,7 @@ static struct scsi_host_template iscsi_iser_sht = {
 	.eh_device_reset_handler= iscsi_eh_device_reset,
 	.eh_host_reset_handler	= iscsi_eh_host_reset,
 	.use_clustering         = DISABLE_CLUSTERING,
+	.slave_configure	= iscsi_iser_slave_configure,
 	.proc_name              = "iscsi_iser",
 	.this_id                = -1,
 };

Signature · 1 line

1.5.3.8

Erez Zilber

2008-05-05 13:18 UTC #7 lore raw

Re: [PATCH 1/3 v2] iscsi iser: remove DMA alignment restriction

Quoted 18 lines · Pete Wyckoff

Thanks to James pointing out the problems with the BLK_BOUNCE_ANY for IB devices, this revised patch contains only the DMA alignment fix for iSER.
Mike, can you take care of this and the other two patches in the series:
[PATCH 2/3] iscsi iser: increase max_sectors
[PATCH 3/3] iscsi iser: increase sg_tablesize
-- Pete
From 255e73b67ec1458af18395981fddebdc958e8fe9 Mon Sep 17 00:00:00 2001
From: Pete Wyckoff <[email protected]>
Date: Thu, 14 Feb 2008 16:09:27 -0500
Subject: [PATCH] iscsi iser: remove DMA alignment restriction

Quoted 5 lines

iscsi_iser does not require any particular DMA aligement requirement. Add a slave_configure function to set the alignment to zero, allowing the use of direct IO from arbitrary offsets within a page.

We had a long discussion about it, and I'm ok with this patch. However, I would change the description:

iSER has DMA alignment requirements that are too complex to express with q->dma_alignment. Therefore, if necessary, bouncing should be done by the iSER driver. Add a slave_configure function to set the alignment to zero, allowing the use of direct IO from arbitrary offsets within a page.

Erez

Erez Zilber

2008-04-21 13:51 UTC #8 lore raw

Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

In reply to Pete Wyckoff

Quoted 63 lines · Pete Wyckoff

[email protected] wrote on Tue, 12 Feb 2008 15:57 -0600:
Quoted 52 lines · Pete Wyckoff
Quoted 51 lines
[email protected] wrote on Tue, 12 Feb 2008 15:10 -0600:
Quoted 21 lines · Pete Wyckoff
Quoted 20 lines
iscsi_iser does not have any hardware DMA restrictions. Add a slave_configure function to remove any DMA alignment restriction, allowing the use of direct IO from arbitrary offsets within a page. Also disable page bouncing; iser has no restrictions on which pages it can address.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
index be1b9fb..1b272a6 100644
@@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
 	iser_conn_terminate(ib_conn);
 }
 
Quoted 10 lines
drivers/infiniband/ulp/iser/iscsi_iser.c iscsi_iser_ep_disconnect(__u64 ep_handle)
Quoted 4 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +3 -0
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)
+{
+	blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_ANY);
 
You really don't want to do this. That signals to the block layer that we have an iommu, although it's practically the same thing as a 64 bit DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be mismatched.
Oh. I decided to add that line for symmetry with TCP, and was convinced by the arguments here:
commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56
Author: Mike Christie <[email protected]>
Date: Thu Jul 26 12:46:47 2007 -0500
[SCSI] iscsi_tcp: Turn off bounce buffers
It was found by LSI that on setups with large amounts of memory we were bouncing buffers when we did not need to. If the iscsi tcp code touches the data buffer (or a helper does),
it will kmap the buffer. iscsi_tcp also does not interact with hardware, so it does not have any hw dma restrictions. This patch sets the bounce buffer settings for our device queue so buffers should not be bounced because of a driver limit.
Quoted 8 lines
Quoted 5 lines
I don't see a convenient place to callback into particular iscsi devices to set the DMA mask per-host. It has to go on the shost_gendev, right?, but only for TCP and iSER, not qla4xxx, which handles its DMA mask during device probe.
You should be taking your mask from the underlying infiniband device as part of the setup, shouldn't you?

Quoted 7 lines

I think you're right about this. All the existing IB HW tries to set a 64-bit dma mask, but that's no reason to disable the mechanism entirely in iser. I'll remove that line that disables bouncing in my patch. Perhaps Mike will know if the iscsi_tcp usage is still appropriate.

Let me make sure that I understand: you say that the IB HW driver (e.g. ib_mthca) tries to set a 64-bit dma mask:

    err = pci_set_dma_mask(pdev, DMA_64BIT_MASK);
    if (err) {
        dev_warn(&pdev->dev, "Warning: couldn't set 64-bit PCI DMA
mask.\n");
        err = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
        if (err) {
            dev_err(&pdev->dev, "Can't set PCI DMA mask, aborting.\n");
            goto err_free_res;
        }
    }

So, in the example above, the driver will use a 64-bit mask or a 32-bit mask (or fail). According to that, iSER (and SRP) needs to call blk_queue_bounce_limit with the appropriate parameter, right?

Thanks,
Erez

Erez Zilber

2008-04-23 13:41 UTC #9 lore raw

Re: [ofa-general] Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

Quoted 97 lines · Erez Zilber

Quoted 1 line · Pete Wyckoff
[email protected] wrote on Tue, 12 Feb 2008
15:57 -0600:
Quoted 4 lines
Quoted 4 lines · Pete Wyckoff
Quoted 2 lines
[email protected] wrote on Tue, 12 Feb 2008
15:10 -0600:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines · Pete Wyckoff
Quoted 5 lines
iscsi_iser does not have any hardware DMA restrictions. Add a slave_configure function to remove any DMA alignment restriction, allowing the use of direct IO from arbitrary offsets within a page. Also disable page bouncing; iser has no restrictions on which
pages it
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review
Quoted 8 lines
can address.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
(quoted diff) +0 -0
b/drivers/infiniband/ulp/iser/iscsi_iser.c
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
index be1b9fb..1b272a6 100644
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
@@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
  iser_conn_terminate(ib_conn);
 }
 
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/infiniband/ulp/iser/iscsi_iser.c iscsi_iser_ep_disconnect(__u64 ep_handle)
Quoted 4 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +3 -0
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)
+{
+ blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_ANY);
 
You really don't want to do this. That signals to the block
layer that
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
we have an iommu, although it's practically the same thing as a
64 bit
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be
mismatched.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Oh. I decided to add that line for symmetry with TCP, and was convinced by the arguments here:
    commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56
    Author: Mike Christie <[email protected]>
    Date:   Thu Jul 26 12:46:47 2007 -0500
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
[SCSI] iscsi_tcp: Turn off bounce buffers
It was found by LSI that on setups with large amounts of memory we were bouncing buffers when we did not need to. If the iscsi tcp code touches the data buffer (or a helper does),
it will kmap the buffer. iscsi_tcp also does not interact with
hardware,
Quoted 1 line
Quoted 1 line
Quoted 1 line
so it does not have any hw dma restrictions. This patch sets
the bounce
Quoted 1 line
Quoted 1 line
Quoted 1 line
buffer settings for our device queue so buffers should not be
bounced
Quoted 11 lines
Quoted 10 lines
Quoted 7 lines
because of a driver limit.
I don't see a convenient place to callback into particular iscsi devices to set the DMA mask per-host. It has to go on the shost_gendev, right?, but only for TCP and iSER, not qla4xxx, which handles its DMA mask during device probe.
You should be taking your mask from the underlying infiniband device as part of the setup, shouldn't you?
Quoted 7 lines
I think you're right about this. All the existing IB HW tries to set a 64-bit dma mask, but that's no reason to disable the mechanism entirely in iser. I'll remove that line that disables bouncing in my patch. Perhaps Mike will know if the iscsi_tcp usage is still appropriate.
Let me make sure that I understand: you say that the IB HW driver (e.g. ib_mthca) tries to set a 64-bit dma mask:
    err = pci_set_dma_mask(pdev, DMA_64BIT_MASK);
    if (err) {
        dev_warn(&pdev->dev, "Warning: couldn't set 64-bit PCI DMA
mask.\n");
        err = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
        if (err) {
            dev_err(&pdev->dev, "Can't set PCI DMA mask, aborting.\n");
            goto err_free_res;
        }
    }

Quoted 4 lines

So, in the example above, the driver will use a 64-bit mask or a 32-bit mask (or fail). According to that, iSER (and SRP) needs to call blk_queue_bounce_limit with the appropriate parameter, right?

Roland, James,

I'm trying to fix this potential problem in iSER, and I have some questions about that. How can I get the DMA mask that the HCA driver is using (DMA_64BIT_MASK or DMA_32BIT_MASK)? Can I get it somehow from struct ib_device? Is it in ib_device->device?

Another question is - after I get the DMA mask data from the HCA driver, I guess that I need to call blk_queue_bounce_limit with the appropriate parameter (BLK_BOUNCE_HIGH, BLK_BOUNCE_ANY or BLK_BOUNCE_ISA). Which value should iSER use according to the DMA mask info? For example, if the HCA driver sets DMA_64BIT_MASK, should iSER use BLK_BOUNCE_HIGH/BLK_BOUNCE_ANY/BLK_BOUNCE_ISA ?

Thanks,
Erez

Mike Christie

2008-04-23 16:33 UTC #10 lore raw

Re: [ofa-general] Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

Quoted 100 lines · Erez Zilber

Quoted 94 lines · Erez Zilber
Quoted 1 line · Pete Wyckoff
[email protected] wrote on Tue, 12 Feb 2008
15:57 -0600:
Quoted 4 lines
Quoted 4 lines · Pete Wyckoff
Quoted 2 lines
[email protected] wrote on Tue, 12 Feb 2008
15:10 -0600:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines · Pete Wyckoff
Quoted 5 lines
iscsi_iser does not have any hardware DMA restrictions. Add a slave_configure function to remove any DMA alignment restriction, allowing the use of direct IO from arbitrary offsets within a page. Also disable page bouncing; iser has no restrictions on which
pages it
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review
Quoted 8 lines
can address.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
(quoted diff) +0 -0
b/drivers/infiniband/ulp/iser/iscsi_iser.c
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
index be1b9fb..1b272a6 100644
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
@@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
  iser_conn_terminate(ib_conn);
 }
 
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/infiniband/ulp/iser/iscsi_iser.c iscsi_iser_ep_disconnect(__u64 ep_handle) · line 549
Quoted 4 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +3 -0
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)
+{
+ blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_ANY);
 
You really don't want to do this. That signals to the block
layer that
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
we have an iommu, although it's practically the same thing as a
64 bit
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be
mismatched.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Oh. I decided to add that line for symmetry with TCP, and was convinced by the arguments here:
    commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56
    Author: Mike Christie <[email protected]>
    Date:   Thu Jul 26 12:46:47 2007 -0500
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
[SCSI] iscsi_tcp: Turn off bounce buffers
It was found by LSI that on setups with large amounts of memory we were bouncing buffers when we did not need to. If the iscsi tcp code touches the data buffer (or a helper does),
it will kmap the buffer. iscsi_tcp also does not interact with
hardware,
Quoted 1 line
Quoted 1 line
Quoted 1 line
so it does not have any hw dma restrictions. This patch sets
the bounce
Quoted 1 line
Quoted 1 line
Quoted 1 line
buffer settings for our device queue so buffers should not be
bounced
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
because of a driver limit.
I don't see a convenient place to callback into particular iscsi devices to set the DMA mask per-host. It has to go on the shost_gendev, right?, but only for TCP and iSER, not qla4xxx, which handles its DMA mask during device probe.
Quoted 10 lines
Quoted 3 lines
You should be taking your mask from the underlying infiniband device as part of the setup, shouldn't you?
I think you're right about this. All the existing IB HW tries to set a 64-bit dma mask, but that's no reason to disable the mechanism entirely in iser. I'll remove that line that disables bouncing in my patch. Perhaps Mike will know if the iscsi_tcp usage is still appropriate.
Let me make sure that I understand: you say that the IB HW driver (e.g. ib_mthca) tries to set a 64-bit dma mask:
    err = pci_set_dma_mask(pdev, DMA_64BIT_MASK);
    if (err) {
        dev_warn(&pdev->dev, "Warning: couldn't set 64-bit PCI DMA
mask.\n");
        err = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
        if (err) {
            dev_err(&pdev->dev, "Can't set PCI DMA mask, aborting.\n");
            goto err_free_res;
        }
    }
Quoted 4 lines
So, in the example above, the driver will use a 64-bit mask or a 32-bit mask (or fail). According to that, iSER (and SRP) needs to call blk_queue_bounce_limit with the appropriate parameter, right?

Quoted 6 lines

Roland, James,
I'm trying to fix this potential problem in iSER, and I have some questions about that. How can I get the DMA mask that the HCA driver is using (DMA_64BIT_MASK or DMA_32BIT_MASK)? Can I get it somehow from struct ib_device? Is it in ib_device->device?

I think what Erez is asking, or maybe it is something I was wondering is, that scsi drivers like lpfc or qla2xxx will do something like:

if (dma_set_mask(&scsi_host->pdev->dev, DMA_64BIT_MASK))

	dma_set_mask(&scsi_host->pdev->dev, DMA_32BIT_MASK)

And when __scsi_alloc_queue calls scsi_calculate_bounce_limit it checks the host's parent dma_mask and sets the bounce_limit for the driver.

Does srp/iser need to call the dma_set_mask functions or does the ib_device's device already have the dma info set up?

Quoted 7 lines

Another question is - after I get the DMA mask data from the HCA driver, I guess that I need to call blk_queue_bounce_limit with the appropriate parameter (BLK_BOUNCE_HIGH, BLK_BOUNCE_ANY or BLK_BOUNCE_ISA). Which value should iSER use according to the DMA mask info? For example, if the HCA driver sets DMA_64BIT_MASK, should iSER use BLK_BOUNCE_HIGH/BLK_BOUNCE_ANY/BLK_BOUNCE_ISA ?

Have you seen how the scsi layer calls blk_queue_bounce_limit when you have a parent device that is setup?

In the bnx2i branch I modified iser to be more like srp and traditional drivers, because it accesses the ib_device similar to how other drivers like lpfc or qla2xxx access their parent device for dma funtions, and when the underlying device is removed we now remove the sessions like with other hotplug drivers (we remove sessions from the ib_client remove callout like srp). In the branch, iser allocates a scsi_host per ib_device, and the scsi_host's parent is the ib_device ( ..../ib_device/scsi_host/iscsi_session/scsi_target/scsi_device), so if the dma_mask is set right then the bounce_limit will be set by scsi_calculate_bounce_limit?

An alternative could be to keep allocating a host per session, but just call blk_queue_bounce_limit in the scsi_host_template->slave_alloc function?

Mike Christie

2008-04-23 17:16 UTC #11 lore raw

Re: [ofa-general] Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

Quoted 109 lines · Mike Christie

Quoted 101 lines · Erez Zilber
Quoted 95 lines · Erez Zilber
Quoted 1 line · Pete Wyckoff
[email protected] wrote on Tue, 12 Feb 2008
15:57 -0600:
Quoted 4 lines
Quoted 4 lines · Pete Wyckoff
Quoted 2 lines
[email protected] wrote on Tue, 12 Feb 2008
15:10 -0600:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines · Pete Wyckoff
Quoted 5 lines
iscsi_iser does not have any hardware DMA restrictions. Add a slave_configure function to remove any DMA alignment restriction, allowing the use of direct IO from arbitrary offsets within a page. Also disable page bouncing; iser has no restrictions on which
pages it
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review
Quoted 8 lines
can address.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
(quoted diff) +0 -0
b/drivers/infiniband/ulp/iser/iscsi_iser.c
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
index be1b9fb..1b272a6 100644
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
@@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
  iser_conn_terminate(ib_conn);
 }
 
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/infiniband/ulp/iser/iscsi_iser.c iscsi_iser_ep_disconnect(__u64 ep_handle) · line 549
Quoted 4 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +3 -0
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)
+{
+ blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_ANY);
 
You really don't want to do this. That signals to the block
layer that
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
we have an iommu, although it's practically the same thing as a
64 bit
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be
mismatched.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Oh. I decided to add that line for symmetry with TCP, and was convinced by the arguments here:
    commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56
    Author: Mike Christie <[email protected]>
    Date:   Thu Jul 26 12:46:47 2007 -0500
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
[SCSI] iscsi_tcp: Turn off bounce buffers
It was found by LSI that on setups with large amounts of memory we were bouncing buffers when we did not need to. If the iscsi
tcp
code touches the data buffer (or a helper does),
it will kmap the buffer. iscsi_tcp also does not interact with
hardware,
Quoted 1 line
Quoted 1 line
Quoted 1 line
so it does not have any hw dma restrictions. This patch sets
the bounce
Quoted 1 line
Quoted 1 line
Quoted 1 line
buffer settings for our device queue so buffers should not be
bounced
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
because of a driver limit.
I don't see a convenient place to callback into particular iscsi devices to set the DMA mask per-host. It has to go on the shost_gendev, right?, but only for TCP and iSER, not qla4xxx, which handles its DMA mask during device probe.
Quoted 10 lines
Quoted 3 lines
You should be taking your mask from the underlying infiniband device as part of the setup, shouldn't you?
I think you're right about this. All the existing IB HW tries to set a 64-bit dma mask, but that's no reason to disable the mechanism entirely in iser. I'll remove that line that disables bouncing in my patch. Perhaps Mike will know if the iscsi_tcp usage is still appropriate.
Let me make sure that I understand: you say that the IB HW driver (e.g. ib_mthca) tries to set a 64-bit dma mask:
    err = pci_set_dma_mask(pdev, DMA_64BIT_MASK);
    if (err) {
        dev_warn(&pdev->dev, "Warning: couldn't set 64-bit PCI DMA
mask.\n");
        err = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
        if (err) {
            dev_err(&pdev->dev, "Can't set PCI DMA mask, aborting.\n");
            goto err_free_res;
        }
    }
Quoted 4 lines
So, in the example above, the driver will use a 64-bit mask or a 32-bit mask (or fail). According to that, iSER (and SRP) needs to call blk_queue_bounce_limit with the appropriate parameter, right?
Quoted 6 lines
Roland, James,
I'm trying to fix this potential problem in iSER, and I have some questions about that. How can I get the DMA mask that the HCA driver is using (DMA_64BIT_MASK or DMA_32BIT_MASK)? Can I get it somehow from struct ib_device? Is it in ib_device->device?

Quoted 8 lines

I think what Erez is asking, or maybe it is something I was wondering is, that scsi drivers like lpfc or qla2xxx will do something like:
if (dma_set_mask(&scsi_host->pdev->dev, DMA_64BIT_MASK))
    dma_set_mask(&scsi_host->pdev->dev, DMA_32BIT_MASK)
And when __scsi_alloc_queue calls scsi_calculate_bounce_limit it checks the host's parent dma_mask and sets the bounce_limit for the driver.
Does srp/iser need to call the dma_set_mask functions or does the ib_device's device already have the dma info set up?

Nevermind. I misread the mail. We know the ib hw driver sets the mask. I
guess what we are debating is if we should set the scsi_host's parent
to the ib_device so the dma mask is picked up, or if should just set them in our slave_configure by calling blk_queue_bounce_limit. And if we use the blk_queue_bounce_limit path, what function do we call to get the dma_mask.

I also modified iser to allocate a host per ib_deivce so it works like other scsi drivers since we know the parent. Is this preferred over the host per session style. Does it matter?

bnx2i works similar to iser where we use a libiscsi, and dma against a real device. Should it do a host per session or host per netdev?

And if we do not allocate a host per ib_device/netdevice what should we allocate per those structs? Should we create our own?

Mike Christie

2008-04-23 17:43 UTC #12 lore raw

Re: [ofa-general] Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

Quoted 115 lines · Mike Christie

Quoted 114 lines · Mike Christie
Quoted 101 lines · Erez Zilber
Quoted 95 lines · Erez Zilber
Quoted 1 line · Pete Wyckoff
[email protected] wrote on Tue, 12 Feb 2008
15:57 -0600:
Quoted 4 lines
Quoted 4 lines · Pete Wyckoff
Quoted 2 lines
[email protected] wrote on Tue, 12 Feb 2008
15:10 -0600:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines · Pete Wyckoff
Quoted 5 lines
iscsi_iser does not have any hardware DMA restrictions. Add a slave_configure function to remove any DMA alignment restriction, allowing the use of direct IO from arbitrary offsets within a page. Also disable page bouncing; iser has no restrictions on which
pages it
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review
Quoted 8 lines
can address.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
(quoted diff) +0 -0
b/drivers/infiniband/ulp/iser/iscsi_iser.c
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
index be1b9fb..1b272a6 100644
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
@@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
  iser_conn_terminate(ib_conn);
 }
 
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/infiniband/ulp/iser/iscsi_iser.c iscsi_iser_ep_disconnect(__u64 ep_handle) · line 549
Quoted 4 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +3 -0
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)
+{
+ blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_ANY);
 
You really don't want to do this. That signals to the block
layer that
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
we have an iommu, although it's practically the same thing as a
64 bit
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be
mismatched.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Oh. I decided to add that line for symmetry with TCP, and was convinced by the arguments here:
    commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56
    Author: Mike Christie <[email protected]>
    Date:   Thu Jul 26 12:46:47 2007 -0500
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
[SCSI] iscsi_tcp: Turn off bounce buffers
It was found by LSI that on setups with large amounts of memory we were bouncing buffers when we did not need to. If the
iscsi tcp
code touches the data buffer (or a helper does),
it will kmap the buffer. iscsi_tcp also does not interact with
hardware,
Quoted 1 line
Quoted 1 line
Quoted 1 line
so it does not have any hw dma restrictions. This patch sets
the bounce
Quoted 1 line
Quoted 1 line
Quoted 1 line
buffer settings for our device queue so buffers should not be
bounced
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
because of a driver limit.
I don't see a convenient place to callback into particular iscsi devices to set the DMA mask per-host. It has to go on the shost_gendev, right?, but only for TCP and iSER, not qla4xxx, which handles its DMA mask during device probe.
Quoted 10 lines
Quoted 3 lines
You should be taking your mask from the underlying infiniband device as part of the setup, shouldn't you?
I think you're right about this. All the existing IB HW tries to set a 64-bit dma mask, but that's no reason to disable the mechanism entirely in iser. I'll remove that line that disables bouncing in my patch. Perhaps Mike will know if the iscsi_tcp usage is still appropriate.
Let me make sure that I understand: you say that the IB HW driver (e.g. ib_mthca) tries to set a 64-bit dma mask:
    err = pci_set_dma_mask(pdev, DMA_64BIT_MASK);
    if (err) {
        dev_warn(&pdev->dev, "Warning: couldn't set 64-bit PCI DMA
mask.\n");
        err = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
        if (err) {
            dev_err(&pdev->dev, "Can't set PCI DMA mask, aborting.\n");
            goto err_free_res;
        }
    }
Quoted 4 lines
So, in the example above, the driver will use a 64-bit mask or a 32-bit mask (or fail). According to that, iSER (and SRP) needs to call blk_queue_bounce_limit with the appropriate parameter, right?
Quoted 6 lines
Roland, James,
I'm trying to fix this potential problem in iSER, and I have some questions about that. How can I get the DMA mask that the HCA driver is using (DMA_64BIT_MASK or DMA_32BIT_MASK)? Can I get it somehow from struct ib_device? Is it in ib_device->device?
I think what Erez is asking, or maybe it is something I was wondering is, that scsi drivers like lpfc or qla2xxx will do something like:
if (dma_set_mask(&scsi_host->pdev->dev, DMA_64BIT_MASK))
    dma_set_mask(&scsi_host->pdev->dev, DMA_32BIT_MASK)

Quoted 8 lines

Quoted 3 lines
And when __scsi_alloc_queue calls scsi_calculate_bounce_limit it checks the host's parent dma_mask and sets the bounce_limit for the driver.
Does srp/iser need to call the dma_set_mask functions or does the ib_device's device already have the dma info set up?
Nevermind. I misread the mail. We know the ib hw driver sets the mask. I
guess what we are debating is if we should set the scsi_host's parent
to the ib_device so the dma mask is picked up, or if should just set them in our slave_configure by calling blk_queue_bounce_limit. And if we use the blk_queue_bounce_limit path, what function do we call to get the dma_mask.

Oh man, I should have looked at the code before posting. For this last part, if we do not set a correct host parent I guess we have to just dupicate what scsi_calculate_bounce_limit does. It would be a waste to copy that code for iser. I guess we could modify scsi_calculate_bounce_limit somehow.

Mike Christie

2008-02-14 17:57 UTC #13 lore raw

Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

In reply to James Bottomley

Quoted 20 lines · James Bottomley

Quoted 19 lines · Pete Wyckoff
iscsi_iser does not have any hardware DMA restrictions. Add a slave_configure function to remove any DMA alignment restriction, allowing the use of direct IO from arbitrary offsets within a page. Also disable page bouncing; iser has no restrictions on which pages it can address.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
index be1b9fb..1b272a6 100644
@@ -543,6 +543,13 @@ iscsi_iser_ep_disconnect(__u64 ep_handle)
 	iser_conn_terminate(ib_conn);
 }
 

Quoted 10 lines

drivers/infiniband/ulp/iser/iscsi_iser.c iscsi_iser_ep_disconnect(__u64 ep_handle) · line 548
Quoted 3 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +3 -0
+static int iscsi_iser_slave_configure(struct scsi_device *sdev)
+{
+	blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_ANY);
You really don't want to do this. That signals to the block layer that we have an iommu, although it's practically the same thing as a 64 bit DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be mismatched.

I thought BLK_BOUNCE_ANY just meant "don't bounce anything" (that was from the blkdev.h comments). We used it for iscsi_tcp because the network layer can take any type of page and will do the right thing for the hardware it eventually gets sent to.

James Bottomley

maintainer

2008-02-14 18:10 UTC #14 lore raw

Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

Quoted 9 lines · Mike Christie

Quoted 6 lines
You really don't want to do this. That signals to the block layer that we have an iommu, although it's practically the same thing as a 64 bit DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be mismatched.
I thought BLK_BOUNCE_ANY just meant "don't bounce anything" (that was from the blkdev.h comments).

It does ... that's why it's used in the IOMMU case ... and why it's practically the same as a 64 bit mask.

Quoted 3 lines

We used it for iscsi_tcp because the network layer can take any type of page and will do the right thing for the hardware it eventually gets sent to.

Right, to you it means never bounce because net wants to do it instead. However, I don't think that's the case for iSER, is it? ... as in, if I've got the pathway tracing correct, it just goes to the infiniband device and gets mapped there. If there's a mask mismatch (very unlikely, I know) we get a very subtle and hard to trace error.

James

Mike Christie

2008-02-14 18:21 UTC #15 lore raw

Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

Quoted 12 lines · James Bottomley

Quoted 7 lines · Mike Christie
Quoted 6 lines
You really don't want to do this. That signals to the block layer that we have an iommu, although it's practically the same thing as a 64 bit DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be mismatched.
I thought BLK_BOUNCE_ANY just meant "don't bounce anything" (that was from the blkdev.h comments).
It does ... that's why it's used in the IOMMU case ... and why it's practically the same as a 64 bit mask.

Quoted 6 lines

Quoted 3 lines
We used it for iscsi_tcp because the network layer can take any type of page and will do the right thing for the hardware it eventually gets sent to.
Right, to you it means never bounce because net wants to do it instead. However, I don't think that's the case for iSER, is it? ... as in, if

I will leave that to Roland and them :)

My only concern with a simple patch to use the ib interface's dma values would be, if there is some event that causes us to start using inteface ib0 then switch to ib1 (maybe like some sort of route table change if that is possible), then we will have to loop over all the scsi devices's queues and update the dma values. And if there is IO already queued then would we have to possible rebuild those commands if something like the dma_mask changed?

Quoted 4 lines

I've got the pathway tracing correct, it just goes to the infiniband device and gets mapped there. If there's a mask mismatch (very unlikely, I know) we get a very subtle and hard to trace error.

Mike Christie

2008-02-14 18:34 UTC #16 lore raw

Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

Quoted 17 lines · Mike Christie

Quoted 16 lines · James Bottomley
Quoted 7 lines · Mike Christie
Quoted 6 lines
You really don't want to do this. That signals to the block layer that we have an iommu, although it's practically the same thing as a 64 bit DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be mismatched.
I thought BLK_BOUNCE_ANY just meant "don't bounce anything" (that was from the blkdev.h comments).
It does ... that's why it's used in the IOMMU case ... and why it's practically the same as a 64 bit mask.
Quoted 3 lines
We used it for iscsi_tcp because the network layer can take any type of page and will do the right thing for the hardware it eventually gets sent to.

Quoted 7 lines

Quoted 2 lines
Right, to you it means never bounce because net wants to do it instead. However, I don't think that's the case for iSER, is it? ... as in, if
I will leave that to Roland and them :)
My only concern with a simple patch to use the ib interface's dma values would be, if there is some event that causes us to start using inteface ib0 then switch to ib1 (maybe like some sort of route table change if that is possible), then we will have to loop over all the scsi devices's queues and update the dma values. And if there is IO already queued then would we have to possible rebuild those commands if something like the dma_mask changed?

I guess we can just handle this like how FC handles the case when the dev loss tmo expires, but then the rport comes back later.

Mike Christie

2008-02-14 19:05 UTC #17 lore raw

Re: [PATCH 1/3] iscsi iser: remove DMA restrictions

Quoted 14 lines · Mike Christie

Quoted 13 lines · Mike Christie
Quoted 12 lines · James Bottomley
Quoted 7 lines · Mike Christie
Quoted 6 lines
You really don't want to do this. That signals to the block layer that we have an iommu, although it's practically the same thing as a 64 bit DMA mask ... but I'd just leave it to the DMA mask to set this up correctly. Anything else is asking for a subtle bug to turn up years from now when something causes the mask and the limit to be mismatched.
I thought BLK_BOUNCE_ANY just meant "don't bounce anything" (that was from the blkdev.h comments).
It does ... that's why it's used in the IOMMU case ... and why it's practically the same as a 64 bit mask.

Quoted 8 lines

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
We used it for iscsi_tcp because the network layer can take any type of page and will do the right thing for the hardware it eventually gets sent to.
Right, to you it means never bounce because net wants to do it instead. However, I don't think that's the case for iSER, is it? ... as in, if
I will leave that to Roland and them :)

Ooops, I misread your mail. I think you are right and for iser we need to use the ib devices dma values.

For some reason I was thinking you were asking if there was a different interface which abstracted all that away for us like with iscsi_tcp.

Quoted 5 lines

Quoted 3 lines
My only concern with a simple patch to use the ib interface's dma values would be, if there is some event that causes us to start using inteface ib0 then switch to ib1 (maybe like some sort of route table change if that is possible), then we will have to loop over all the scsi devices's queues and update the dma values. And if there is IO already queued then would we have to possible rebuild those commands if something like the dma_mask changed?
I guess we can just handle this like how FC handles the case when the dev loss tmo expires, but then the rport comes back later.

Yuck, I guess we would have to do something like this if ib0 and ib1 had different dma restrictions.

Pete Wyckoff

author

2008-02-12 20:54 UTC #18 lore raw

PATCH 2/3 iscsi iser: increase max_sectors

In reply to Pete Wyckoff

iser has no limit on max sectors. This lets iscsi iser support large pass through commands just like iscsi TCP.

Signed-off-by Pete Wyckoff <[email protected]>

---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/infiniband/ulp/iser/iscsi_iser.c +1 -1

index 1b272a6..78f3242 100644
@@ -557,7 +557,7 @@ static struct scsi_host_template iscsi_iser_sht = {
 	.change_queue_depth	= iscsi_change_queue_depth,
 	.can_queue		= ISCSI_DEF_XMIT_CMDS_MAX - 1,
 	.sg_tablesize           = ISCSI_ISER_SG_TABLESIZE,
-	.max_sectors		= 1024,
+	.max_sectors		= 0xffff,
 	.cmd_per_lun            = ISCSI_MAX_CMD_PER_LUN,
 	.eh_abort_handler       = iscsi_eh_abort,
 	.eh_device_reset_handler= iscsi_eh_device_reset,

Signature · 1 line

1.5.3.8

Erez Zilber

2008-05-05 13:35 UTC #19 lore raw

Re: [PATCH 2/3] iscsi iser: increase max_sectors

Quoted 13 lines · Pete Wyckoff

iser has no limit on max sectors. This lets iscsi iser support large pass through commands just like iscsi TCP.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
index 1b272a6..78f3242 100644

drivers/infiniband/ulp/iser/iscsi_iser.c static struct scsi_host_template iscsi_iser_sht = { · line 561

Quoted 6 lines

drivers/infiniband/ulp/iser/iscsi_iser.c +1 -1

@@ -557,7 +557,7 @@ static struct scsi_host_template iscsi_iser_sht = {
 	.change_queue_depth	= iscsi_change_queue_depth,
 	.can_queue		= ISCSI_DEF_XMIT_CMDS_MAX - 1,
 	.sg_tablesize           = ISCSI_ISER_SG_TABLESIZE,
-	.max_sectors		= 1024,
+	.max_sectors		= 0xffff,

This could be problematic because, AFAIK, some HCAs (memfree, I think) don't support FMRs of more than 2MB. Roland, am I right?

Roland Dreier

2008-05-05 20:43 UTC #20 lore raw

Re: [PATCH 2/3] iscsi iser: increase max_sectors

Quoted 2 lines

This could be problematic because, AFAIK, some HCAs (memfree, I think) don't support FMRs of more than 2MB. Roland, am I right?

Mem-free HCAs don't support FMRs where the list of pages is bigger than a page itself. So yes, with 4 KB pages, you get 512 * 4 KB that way.

Of course the current iSER driver has much bigger FMR problems, since some adapters don't support FMRs at all...

- R.

Mike Christie

2008-05-05 17:49 UTC #21 lore raw

Re: [PATCH 2/3] iscsi iser: increase max_sectors

In reply to Pete Wyckoff

Quoted 13 lines · Pete Wyckoff

iser has no limit on max sectors. This lets iscsi iser support large pass through commands just like iscsi TCP.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
index 1b272a6..78f3242 100644

drivers/infiniband/ulp/iser/iscsi_iser.c static struct scsi_host_template iscsi_iser_sht = { · line 563

Quoted 8 lines

drivers/infiniband/ulp/iser/iscsi_iser.c +1 -1

@@ -557,7 +557,7 @@ static struct scsi_host_template iscsi_iser_sht = {
 	.change_queue_depth	= iscsi_change_queue_depth,
 	.can_queue		= ISCSI_DEF_XMIT_CMDS_MAX - 1,
 	.sg_tablesize           = ISCSI_ISER_SG_TABLESIZE,
-	.max_sectors		= 1024,
+	.max_sectors		= 0xffff,
 	.cmd_per_lun            = ISCSI_MAX_CMD_PER_LUN,
 	.eh_abort_handler       = iscsi_eh_abort,
 	.eh_device_reset_handler= iscsi_eh_device_reset,

Do we need to modify sg_tablesize and the related preallocations to take advantage of this? iser sets the sg_tablesize to ISCSI_ISER_SG_TABLESIZE and disables clustering, scsi-ml/block will only send commands up to ISCSI_ISER_SG_TABLESIZE * PAGE_SIZE.

Pete Wyckoff

author

2008-05-07 15:53 UTC #22 lore raw

Re: [PATCH 2/3] iscsi iser: increase max_sectors

[email protected] wrote on Mon, 05 May 2008 12:49 -0500:

Quoted 19 lines

Quoted 18 lines · Pete Wyckoff
iser has no limit on max sectors. This lets iscsi iser support large pass through commands just like iscsi TCP.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.c +1 -1
index 1b272a6..78f3242 100644
@@ -557,7 +557,7 @@ static struct scsi_host_template iscsi_iser_sht = {
 	.change_queue_depth	= iscsi_change_queue_depth,
 	.can_queue		= ISCSI_DEF_XMIT_CMDS_MAX - 1,
 	.sg_tablesize           = ISCSI_ISER_SG_TABLESIZE,
-	.max_sectors		= 1024,
+	.max_sectors		= 0xffff,

Quoted 8 lines

drivers/infiniband/ulp/iser/iscsi_iser.c static struct scsi_host_template iscsi_iser_sht = { · line 563
Quoted 3 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
 	.cmd_per_lun            = ISCSI_MAX_CMD_PER_LUN,
 	.eh_abort_handler       = iscsi_eh_abort,
 	.eh_device_reset_handler= iscsi_eh_device_reset,
Do we need to modify sg_tablesize and the related preallocations to take advantage of this? iser sets the sg_tablesize to ISCSI_ISER_SG_TABLESIZE and disables clustering, scsi-ml/block will only send commands up to ISCSI_ISER_SG_TABLESIZE * PAGE_SIZE.

Yes. I run with another patch to change ISCSI_ISER_SG_TABLESIZE to 1 MB / 4kB + 1, allowing up to 1 MB transfers. (I think that was 3/3 way back when.) That's because iser limits the fmr code like this:

        params.max_pages_per_fmr = ISCSI_ISER_SG_TABLESIZE;

The plus 1 handles the case that the 1 MB transfer doesn't start on a page boundary.

But you point out that this sg_tablesize setting is also used to limit the maximum number of pages when building bios to send to the device. So these numbers should all agree.

[email protected] wrote on Mon, 05 May 2008 13:43 -0700:

Quoted 8 lines

Quoted 2 lines
This could be problematic because, AFAIK, some HCAs (memfree, I think) don't support FMRs of more than 2MB. Roland, am I right?
Mem-free HCAs don't support FMRs where the list of pages is bigger than a page itself. So yes, with 4 KB pages, you get 512 * 4 KB that way.
Of course the current iSER driver has much bigger FMR problems, since some adapters don't support FMRs at all...

And certain current hardware is 2 MB tops anyway, but not all. Iser cannot place a limit on the SCSI queue until it knows on which device it will use to communicate.

The goal with these patches is to increase througphut for big transfers.

What's the right thing to do then? Maybe punt to the user. How about a module parameter like SRP, then we calculate the right limits for each of these three related settigns: max_sectors, sg_tablesize, max_pages_per_fmr.

Erez, is this something you're interested in figuring out? I'll be offline for a couple of weeks at least.

		-- Pete

Erez Zilber

2008-05-12 12:09 UTC #23 lore raw

Re: [PATCH 2/3] iscsi iser: increase max_sectors

Quoted 54 lines · Pete Wyckoff

[email protected] wrote on Mon, 05 May 2008 12:49 -0500:
Quoted 20 lines
Quoted 18 lines · Pete Wyckoff
iser has no limit on max sectors. This lets iscsi iser support large pass through commands just like iscsi TCP.
Signed-off-by Pete Wyckoff <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.c +1 -1
index 1b272a6..78f3242 100644
@@ -557,7 +557,7 @@ static struct scsi_host_template iscsi_iser_sht = {
 	.change_queue_depth	= iscsi_change_queue_depth,
 	.can_queue		= ISCSI_DEF_XMIT_CMDS_MAX - 1,
 	.sg_tablesize           = ISCSI_ISER_SG_TABLESIZE,
-	.max_sectors		= 1024,
+	.max_sectors		= 0xffff,
Quoted 9 lines
drivers/infiniband/ulp/iser/iscsi_iser.c static struct scsi_host_template iscsi_iser_sht = { · line 563
Quoted 4 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
 	.cmd_per_lun            = ISCSI_MAX_CMD_PER_LUN,
 	.eh_abort_handler       = iscsi_eh_abort,
 	.eh_device_reset_handler= iscsi_eh_device_reset,
Do we need to modify sg_tablesize and the related preallocations to take advantage of this? iser sets the sg_tablesize to ISCSI_ISER_SG_TABLESIZE and disables clustering, scsi-ml/block will only send commands up to ISCSI_ISER_SG_TABLESIZE * PAGE_SIZE.
Yes. I run with another patch to change ISCSI_ISER_SG_TABLESIZE to 1 MB / 4kB + 1, allowing up to 1 MB transfers. (I think that was 3/3 way back when.) That's because iser limits the fmr code like this:
        params.max_pages_per_fmr = ISCSI_ISER_SG_TABLESIZE;
The plus 1 handles the case that the 1 MB transfer doesn't start on a page boundary.
But you point out that this sg_tablesize setting is also used to limit the maximum number of pages when building bios to send to the device. So these numbers should all agree.
[email protected] wrote on Mon, 05 May 2008 13:43 -0700:
Quoted 6 lines
Quoted 2 lines
This could be problematic because, AFAIK, some HCAs (memfree, I think) don't support FMRs of more than 2MB. Roland, am I right?
Mem-free HCAs don't support FMRs where the list of pages is bigger than a page itself. So yes, with 4 KB pages, you get 512 * 4 KB that way.

Quoted 9 lines

Quoted 3 lines
Of course the current iSER driver has much bigger FMR problems, since some adapters don't support FMRs at all...
And certain current hardware is 2 MB tops anyway, but not all. Iser cannot place a limit on the SCSI queue until it knows on which device it will use to communicate.

Do you suggest that iSER should get these limits from the HW and set max_sectors accordingly? I don't know if it's possible. Roland - do the HCA drivers publish this limitation somehow? If not, maybe they should.

Quoted 3 lines

The goal with these patches is to increase througphut for big transfers.

Are you interested in transfers > 2MB?

Erez

Quoted 8 lines

What's the right thing to do then? Maybe punt to the user. How about a module parameter like SRP, then we calculate the right limits for each of these three related settigns: max_sectors, sg_tablesize, max_pages_per_fmr.
Erez, is this something you're interested in figuring out? I'll be offline for a couple of weeks at least.

Currently, we don't need transfers > 2MB. BTW - I will be offline for more than a couple of weeks :-) (see my other e-mail in openfabrics-general). I'm adding Eli Dorfman from Voltaire to the discussion. He will replace me in iSER related tasks.

Erez

Pete Wyckoff

author

2008-02-12 20:54 UTC #24 lore raw

PATCH 3/3 iscsi iser: increase sg_tablesize

In reply to Pete Wyckoff

Increase FMR limits from 512 kB to 1 MB. This matches the limit used by SRP which also uses FMRs for memory mapping. Also correct a bug where the sg_tablesize was 1 smaller than what was allocated, by folding the "+ 1" used everywhere into the definition of the constant.

Signed-off-by Pete Wyckoff <[email protected]>

---
 drivers/infiniband/ulp/iser/iscsi_iser.h |    3 ++-
 drivers/infiniband/ulp/iser/iser_verbs.c |    6 ++----
 2 files changed, 4 insertions(+), 5 deletions(-)

drivers/infiniband/ulp/iser/iscsi_iser.h +2 -1

index 1ee867b..db8f81a 100644
@@ -87,7 +87,8 @@
 #define MASK_4K	(~(SIZE_4K-1))
 
 					/* support upto 512KB in one RDMA */
-#define ISCSI_ISER_SG_TABLESIZE         (0x80000 >> SHIFT_4K)
+/* FMR space for 1 MB of 4k-page transfers, plus 1 if not page aligned */
+#define ISCSI_ISER_SG_TABLESIZE         (((1<<20) >> SHIFT_4K) + 1)
 #define ISCSI_ISER_MAX_LUN		256
 #define ISCSI_ISER_MAX_CMD_LEN		16

drivers/infiniband/ulp/iser/iser_verbs.c +2 -4

index 714b8db..c5b374f 100644
@@ -140,7 +140,7 @@ static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 	device = ib_conn->device;
 
 	ib_conn->page_vec = kmalloc(sizeof(struct iser_page_vec) +
-				    (sizeof(u64) * (ISCSI_ISER_SG_TABLESIZE +1)),
+				    sizeof(u64) * ISCSI_ISER_SG_TABLESIZE,
 				    GFP_KERNEL);
 	if (!ib_conn->page_vec) {
 		ret = -ENOMEM;

@@ -149,9 +149,7 @@ static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 	ib_conn->page_vec->pages = (u64 *) (ib_conn->page_vec + 1);
 
 	params.page_shift        = SHIFT_4K;
-	/* when the first/last SG element are not start/end *
-	 * page aligned, the map whould be of N+1 pages     */
-	params.max_pages_per_fmr = ISCSI_ISER_SG_TABLESIZE + 1;
+	params.max_pages_per_fmr = ISCSI_ISER_SG_TABLESIZE;
 	/* make the pool size twice the max number of SCSI commands *
 	 * the ML is expected to queue, watermark for unmap at 50%  */
 	params.pool_size	 = ISCSI_DEF_XMIT_CMDS_MAX * 2;

Signature · 1 line

1.5.3.8

Erez Zilber

2008-03-02 13:56 UTC #25 lore raw

In reply to Pete Wyckoff

Quoted 11 lines · Pete Wyckoff

These three patches remove various block device limits to the iSER iSCSI transport. I guess in theory they should go in via the IB tree, although ACKs from the iSCSI side would be welcome.
The first two look quite similar to some TCP iSCSI patches that Mike applied a while back:
d1d81c01f4bdd50577d9f89aa4a8e6344f63aa70
[SCSI] iscsi_tcp: remove DMA alignment restriction

Quoted 8 lines

8231f0eddbe425cc3b54f2d723bb03531925272e
[SCSI] iscsi_tcp: increase max_sectors
The third fixes a problem with mapping an unaligned 512 kB buffer and takes the opportunity to increase the FMR mapping limit to 1 MB, as is done in SRP.

Pete,

In the future, please send iSER patches also to
[email protected].

Thanks,
Erez