* [PATCH 0/3] iscsi iser limits
@ 2008-02-12 20:52 Pete Wyckoff
2008-02-12 20:54 ` [PATCH 1/3] iscsi iser: remove DMA restrictions Pete Wyckoff
` (3 more replies)
0 siblings, 4 replies; 26+ messages in thread
From: Pete Wyckoff @ 2008-02-12 20:52 UTC (permalink / raw)
To: Mike Christie, Erez Zilber, Roland Dreier; +Cc: linux-scsi
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
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-02-12 20:52 [PATCH 0/3] iscsi iser limits Pete Wyckoff @ 2008-02-12 20:54 ` Pete Wyckoff 2008-02-12 21:10 ` James Bottomley 2008-02-12 20:54 ` [PATCH 2/3] iscsi iser: increase max_sectors Pete Wyckoff ` (2 subsequent siblings) 3 siblings, 1 reply; 26+ messages in thread From: Pete Wyckoff @ 2008-02-12 20:54 UTC (permalink / raw) To: Mike Christie, Erez Zilber, Roland Dreier; +Cc: linux-scsi 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 <pw@osc.edu> --- drivers/infiniband/ulp/iser/iscsi_iser.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-) diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c index be1b9fb..1b272a6 100644 --- a/drivers/infiniband/ulp/iser/iscsi_iser.c +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c @@ -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, }; -- 1.5.3.8 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-02-12 20:54 ` [PATCH 1/3] iscsi iser: remove DMA restrictions Pete Wyckoff @ 2008-02-12 21:10 ` James Bottomley 2008-02-12 21:46 ` Pete Wyckoff 2008-02-14 17:56 ` Mike Christie 0 siblings, 2 replies; 26+ messages in thread From: James Bottomley @ 2008-02-12 21:10 UTC (permalink / raw) To: Pete Wyckoff; +Cc: Mike Christie, Erez Zilber, Roland Dreier, linux-scsi On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote: > 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 <pw@osc.edu> > --- > drivers/infiniband/ulp/iser/iscsi_iser.c | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c > index be1b9fb..1b272a6 100644 > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > @@ -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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-02-12 21:10 ` James Bottomley @ 2008-02-12 21:46 ` Pete Wyckoff 2008-02-12 21:57 ` James Bottomley 2008-02-14 17:56 ` Mike Christie 1 sibling, 1 reply; 26+ messages in thread From: Pete Wyckoff @ 2008-02-12 21:46 UTC (permalink / raw) To: James Bottomley; +Cc: Mike Christie, Erez Zilber, Roland Dreier, linux-scsi James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 15:10 -0600: > On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote: > > 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 <pw@osc.edu> > > --- > > drivers/infiniband/ulp/iser/iscsi_iser.c | 8 ++++++++ > > 1 files changed, 8 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c > > index be1b9fb..1b272a6 100644 > > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > > @@ -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. Oh. I decided to add that line for symmetry with TCP, and was convinced by the arguments here: commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56 Author: Mike Christie <michaelc@cs.wisc.edu> 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-02-12 21:46 ` Pete Wyckoff @ 2008-02-12 21:57 ` James Bottomley 2008-02-13 19:59 ` Pete Wyckoff 0 siblings, 1 reply; 26+ messages in thread From: James Bottomley @ 2008-02-12 21:57 UTC (permalink / raw) To: Pete Wyckoff; +Cc: Mike Christie, Erez Zilber, Roland Dreier, linux-scsi On Tue, 2008-02-12 at 16:46 -0500, Pete Wyckoff wrote: > James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 15:10 -0600: > > On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote: > > > 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 <pw@osc.edu> > > > --- > > > drivers/infiniband/ulp/iser/iscsi_iser.c | 8 ++++++++ > > > 1 files changed, 8 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c > > > index be1b9fb..1b272a6 100644 > > > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > > > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > > > @@ -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. > > Oh. I decided to add that line for symmetry with TCP, and was > convinced by the arguments here: > > commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56 > Author: Mike Christie <michaelc@cs.wisc.edu> > 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. You should be taking your mask from the underlying infiniband device as part of the setup, shouldn't you? James ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-02-12 21:57 ` James Bottomley @ 2008-02-13 19:59 ` Pete Wyckoff 2008-02-14 21:10 ` [PATCH 1/3 v2] iscsi iser: remove DMA alignment restriction Pete Wyckoff 2008-04-21 13:51 ` [PATCH 1/3] iscsi iser: remove DMA restrictions Erez Zilber 0 siblings, 2 replies; 26+ messages in thread From: Pete Wyckoff @ 2008-02-13 19:59 UTC (permalink / raw) To: James Bottomley; +Cc: Mike Christie, Erez Zilber, Roland Dreier, linux-scsi James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 15:57 -0600: > On Tue, 2008-02-12 at 16:46 -0500, Pete Wyckoff wrote: > > James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 15:10 -0600: > > > On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote: > > > > 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 <pw@osc.edu> > > > > --- > > > > drivers/infiniband/ulp/iser/iscsi_iser.c | 8 ++++++++ > > > > 1 files changed, 8 insertions(+), 0 deletions(-) > > > > > > > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c > > > > index be1b9fb..1b272a6 100644 > > > > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > > > > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > > > > @@ -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. > > > > Oh. I decided to add that line for symmetry with TCP, and was > > convinced by the arguments here: > > > > commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56 > > Author: Mike Christie <michaelc@cs.wisc.edu> > > 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. > > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/3 v2] iscsi iser: remove DMA alignment restriction 2008-02-13 19:59 ` Pete Wyckoff @ 2008-02-14 21:10 ` Pete Wyckoff 2008-05-05 13:19 ` Erez Zilber 2008-04-21 13:51 ` [PATCH 1/3] iscsi iser: remove DMA restrictions Erez Zilber 1 sibling, 1 reply; 26+ messages in thread From: Pete Wyckoff @ 2008-02-14 21:10 UTC (permalink / raw) To: Mike Christie, Erez Zilber, Roland Dreier; +Cc: James Bottomley, linux-scsi 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 <pw@osc.edu> 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 <pw@osc.edu> --- drivers/infiniband/ulp/iser/iscsi_iser.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c index be1b9fb..313f102 100644 --- a/drivers/infiniband/ulp/iser/iscsi_iser.c +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c @@ -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, }; -- 1.5.3.8 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3 v2] iscsi iser: remove DMA alignment restriction 2008-02-14 21:10 ` [PATCH 1/3 v2] iscsi iser: remove DMA alignment restriction Pete Wyckoff @ 2008-05-05 13:19 ` Erez Zilber 0 siblings, 0 replies; 26+ messages in thread From: Erez Zilber @ 2008-05-05 13:19 UTC (permalink / raw) To: Pete Wyckoff; +Cc: Mike Christie, Roland Dreier, James Bottomley, linux-scsi Pete Wyckoff wrote: > 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 <pw@osc.edu> > 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. > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-02-13 19:59 ` Pete Wyckoff 2008-02-14 21:10 ` [PATCH 1/3 v2] iscsi iser: remove DMA alignment restriction Pete Wyckoff @ 2008-04-21 13:51 ` Erez Zilber 2008-04-23 13:41 ` [ofa-general] " Erez Zilber 1 sibling, 1 reply; 26+ messages in thread From: Erez Zilber @ 2008-04-21 13:51 UTC (permalink / raw) To: Pete Wyckoff, Roland Dreier Cc: James Bottomley, Mike Christie, linux-scsi, general Pete Wyckoff wrote: > James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 15:57 -0600: > >> On Tue, 2008-02-12 at 16:46 -0500, Pete Wyckoff wrote: >> >>> James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 15:10 -0600: >>> >>>> On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote: >>>> >>>>> 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 <pw@osc.edu> >>>>> --- >>>>> drivers/infiniband/ulp/iser/iscsi_iser.c | 8 ++++++++ >>>>> 1 files changed, 8 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c >>>>> index be1b9fb..1b272a6 100644 >>>>> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c >>>>> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c >>>>> @@ -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. >>>> >>> Oh. I decided to add that line for symmetry with TCP, and was >>> convinced by the arguments here: >>> >>> commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56 >>> Author: Mike Christie <michaelc@cs.wisc.edu> >>> 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. >>> >> 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; } } 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [ofa-general] Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-04-21 13:51 ` [PATCH 1/3] iscsi iser: remove DMA restrictions Erez Zilber @ 2008-04-23 13:41 ` Erez Zilber 2008-04-23 16:33 ` Mike Christie 0 siblings, 1 reply; 26+ messages in thread From: Erez Zilber @ 2008-04-23 13:41 UTC (permalink / raw) To: Roland Dreier, James Bottomley; +Cc: Mike Christie, general, linux-scsi Erez Zilber wrote: > > Pete Wyckoff wrote: > > James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 > 15:57 -0600: > > > >> On Tue, 2008-02-12 at 16:46 -0500, Pete Wyckoff wrote: > >> > >>> James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 > 15:10 -0600: > >>> > >>>> On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote: > >>>> > >>>>> 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 <pw@osc.edu> > >>>>> --- > >>>>> drivers/infiniband/ulp/iser/iscsi_iser.c | 8 ++++++++ > >>>>> 1 files changed, 8 insertions(+), 0 deletions(-) > >>>>> > >>>>> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c > b/drivers/infiniband/ulp/iser/iscsi_iser.c > >>>>> index be1b9fb..1b272a6 100644 > >>>>> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > >>>>> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > >>>>> @@ -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. > >>>> > >>> Oh. I decided to add that line for symmetry with TCP, and was > >>> convinced by the arguments here: > >>> > >>> commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56 > >>> Author: Mike Christie <michaelc@cs.wisc.edu> > >>> 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. > >>> > >> 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; > } > } > > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [ofa-general] Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-04-23 13:41 ` [ofa-general] " Erez Zilber @ 2008-04-23 16:33 ` Mike Christie 2008-04-23 17:16 ` Mike Christie 0 siblings, 1 reply; 26+ messages in thread From: Mike Christie @ 2008-04-23 16:33 UTC (permalink / raw) To: Erez Zilber; +Cc: James Bottomley, general, linux-scsi Erez Zilber wrote: > Erez Zilber wrote: >> Pete Wyckoff wrote: >>> James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 >> 15:57 -0600: >>> >>>> On Tue, 2008-02-12 at 16:46 -0500, Pete Wyckoff wrote: >>>> >>>>> James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 >> 15:10 -0600: >>>>> >>>>>> On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote: >>>>>> >>>>>>> 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 <pw@osc.edu> >>>>>>> --- >>>>>>> drivers/infiniband/ulp/iser/iscsi_iser.c | 8 ++++++++ >>>>>>> 1 files changed, 8 insertions(+), 0 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c >> b/drivers/infiniband/ulp/iser/iscsi_iser.c >>>>>>> index be1b9fb..1b272a6 100644 >>>>>>> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c >>>>>>> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c >>>>>>> @@ -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. >>>>>> >>>>> Oh. I decided to add that line for symmetry with TCP, and was >>>>> convinced by the arguments here: >>>>> >>>>> commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56 >>>>> Author: Mike Christie <michaelc@cs.wisc.edu> >>>>> 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. >>>>> >>>> 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; >> } >> } >> >> 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? 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? > > 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? ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [ofa-general] Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-04-23 16:33 ` Mike Christie @ 2008-04-23 17:16 ` Mike Christie 2008-04-23 17:43 ` Mike Christie 0 siblings, 1 reply; 26+ messages in thread From: Mike Christie @ 2008-04-23 17:16 UTC (permalink / raw) To: Erez Zilber; +Cc: James Bottomley, general, linux-scsi Mike Christie wrote: > Erez Zilber wrote: >> Erez Zilber wrote: >>> Pete Wyckoff wrote: >>>> James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 >>> 15:57 -0600: >>>> >>>>> On Tue, 2008-02-12 at 16:46 -0500, Pete Wyckoff wrote: >>>>> >>>>>> James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 >>> 15:10 -0600: >>>>>> >>>>>>> On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote: >>>>>>> >>>>>>>> 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 <pw@osc.edu> >>>>>>>> --- >>>>>>>> drivers/infiniband/ulp/iser/iscsi_iser.c | 8 ++++++++ >>>>>>>> 1 files changed, 8 insertions(+), 0 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c >>> b/drivers/infiniband/ulp/iser/iscsi_iser.c >>>>>>>> index be1b9fb..1b272a6 100644 >>>>>>>> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c >>>>>>>> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c >>>>>>>> @@ -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. >>>>>>> >>>>>> Oh. I decided to add that line for symmetry with TCP, and was >>>>>> convinced by the arguments here: >>>>>> >>>>>> commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56 >>>>>> Author: Mike Christie <michaelc@cs.wisc.edu> >>>>>> 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. >>>>>> >>>>> 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; >>> } >>> } >>> >>> 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? > > 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? ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [ofa-general] Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-04-23 17:16 ` Mike Christie @ 2008-04-23 17:43 ` Mike Christie 0 siblings, 0 replies; 26+ messages in thread From: Mike Christie @ 2008-04-23 17:43 UTC (permalink / raw) To: Erez Zilber; +Cc: James Bottomley, general, linux-scsi Mike Christie wrote: > Mike Christie wrote: >> Erez Zilber wrote: >>> Erez Zilber wrote: >>>> Pete Wyckoff wrote: >>>>> James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 >>>> 15:57 -0600: >>>>> >>>>>> On Tue, 2008-02-12 at 16:46 -0500, Pete Wyckoff wrote: >>>>>> >>>>>>> James.Bottomley@HansenPartnership.com wrote on Tue, 12 Feb 2008 >>>> 15:10 -0600: >>>>>>> >>>>>>>> On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote: >>>>>>>> >>>>>>>>> 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 <pw@osc.edu> >>>>>>>>> --- >>>>>>>>> drivers/infiniband/ulp/iser/iscsi_iser.c | 8 ++++++++ >>>>>>>>> 1 files changed, 8 insertions(+), 0 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c >>>> b/drivers/infiniband/ulp/iser/iscsi_iser.c >>>>>>>>> index be1b9fb..1b272a6 100644 >>>>>>>>> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c >>>>>>>>> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c >>>>>>>>> @@ -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. >>>>>>>> >>>>>>> Oh. I decided to add that line for symmetry with TCP, and was >>>>>>> convinced by the arguments here: >>>>>>> >>>>>>> commit b6d44fe9582b9d90a0b16f508ac08a90d899bf56 >>>>>>> Author: Mike Christie <michaelc@cs.wisc.edu> >>>>>>> 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. >>>>>>> >>>>>> 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; >>>> } >>>> } >>>> >>>> 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? >> >> 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. > 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-02-12 21:10 ` James Bottomley 2008-02-12 21:46 ` Pete Wyckoff @ 2008-02-14 17:56 ` Mike Christie 2008-02-14 18:10 ` James Bottomley 1 sibling, 1 reply; 26+ messages in thread From: Mike Christie @ 2008-02-14 17:56 UTC (permalink / raw) To: James Bottomley; +Cc: Pete Wyckoff, Erez Zilber, Roland Dreier, linux-scsi James Bottomley wrote: > On Tue, 2008-02-12 at 15:54 -0500, Pete Wyckoff wrote: >> 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 <pw@osc.edu> >> --- >> drivers/infiniband/ulp/iser/iscsi_iser.c | 8 ++++++++ >> 1 files changed, 8 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c >> index be1b9fb..1b272a6 100644 >> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c >> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c >> @@ -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. > 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-02-14 17:56 ` Mike Christie @ 2008-02-14 18:10 ` James Bottomley 2008-02-14 18:21 ` Mike Christie 0 siblings, 1 reply; 26+ messages in thread From: James Bottomley @ 2008-02-14 18:10 UTC (permalink / raw) To: Mike Christie; +Cc: Pete Wyckoff, Erez Zilber, Roland Dreier, linux-scsi On Thu, 2008-02-14 at 11:56 -0600, Mike Christie wrote: > > 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. > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-02-14 18:10 ` James Bottomley @ 2008-02-14 18:21 ` Mike Christie 2008-02-14 18:34 ` Mike Christie 0 siblings, 1 reply; 26+ messages in thread From: Mike Christie @ 2008-02-14 18:21 UTC (permalink / raw) To: James Bottomley; +Cc: Pete Wyckoff, Erez Zilber, Roland Dreier, linux-scsi James Bottomley wrote: > On Thu, 2008-02-14 at 11:56 -0600, Mike Christie wrote: >>> 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. > >> 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? > 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. > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-02-14 18:21 ` Mike Christie @ 2008-02-14 18:34 ` Mike Christie 2008-02-14 19:04 ` Mike Christie 0 siblings, 1 reply; 26+ messages in thread From: Mike Christie @ 2008-02-14 18:34 UTC (permalink / raw) To: James Bottomley; +Cc: Pete Wyckoff, Erez Zilber, Roland Dreier, linux-scsi Mike Christie wrote: > James Bottomley wrote: >> On Thu, 2008-02-14 at 11:56 -0600, Mike Christie wrote: >>>> 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. >> >>> 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? > 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] iscsi iser: remove DMA restrictions 2008-02-14 18:34 ` Mike Christie @ 2008-02-14 19:04 ` Mike Christie 0 siblings, 0 replies; 26+ messages in thread From: Mike Christie @ 2008-02-14 19:04 UTC (permalink / raw) To: James Bottomley; +Cc: Pete Wyckoff, Erez Zilber, Roland Dreier, linux-scsi Mike Christie wrote: > Mike Christie wrote: >> James Bottomley wrote: >>> On Thu, 2008-02-14 at 11:56 -0600, Mike Christie wrote: >>>>> 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. >>> >>>> 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. >> >> 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/3] iscsi iser: increase max_sectors 2008-02-12 20:52 [PATCH 0/3] iscsi iser limits Pete Wyckoff 2008-02-12 20:54 ` [PATCH 1/3] iscsi iser: remove DMA restrictions Pete Wyckoff @ 2008-02-12 20:54 ` Pete Wyckoff 2008-05-05 13:36 ` Erez Zilber 2008-05-05 17:49 ` Mike Christie 2008-02-12 20:54 ` [PATCH 3/3] iscsi iser: increase sg_tablesize Pete Wyckoff 2008-03-02 13:56 ` [PATCH 0/3] iscsi iser limits Erez Zilber 3 siblings, 2 replies; 26+ messages in thread From: Pete Wyckoff @ 2008-02-12 20:54 UTC (permalink / raw) To: Mike Christie, Erez Zilber, Roland Dreier; +Cc: linux-scsi 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 <pw@osc.edu> --- drivers/infiniband/ulp/iser/iscsi_iser.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c index 1b272a6..78f3242 100644 --- a/drivers/infiniband/ulp/iser/iscsi_iser.c +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c @@ -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, -- 1.5.3.8 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 2/3] iscsi iser: increase max_sectors 2008-02-12 20:54 ` [PATCH 2/3] iscsi iser: increase max_sectors Pete Wyckoff @ 2008-05-05 13:36 ` Erez Zilber 2008-05-05 20:43 ` Roland Dreier 2008-05-05 17:49 ` Mike Christie 1 sibling, 1 reply; 26+ messages in thread From: Erez Zilber @ 2008-05-05 13:36 UTC (permalink / raw) To: Pete Wyckoff, Roland Dreier; +Cc: Mike Christie, linux-scsi Pete Wyckoff wrote: > 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 <pw@osc.edu> > --- > drivers/infiniband/ulp/iser/iscsi_iser.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c > index 1b272a6..78f3242 100644 > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > @@ -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? ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/3] iscsi iser: increase max_sectors 2008-05-05 13:36 ` Erez Zilber @ 2008-05-05 20:43 ` Roland Dreier 0 siblings, 0 replies; 26+ messages in thread From: Roland Dreier @ 2008-05-05 20:43 UTC (permalink / raw) To: Erez Zilber; +Cc: Pete Wyckoff, Roland Dreier, Mike Christie, linux-scsi > 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/3] iscsi iser: increase max_sectors 2008-02-12 20:54 ` [PATCH 2/3] iscsi iser: increase max_sectors Pete Wyckoff 2008-05-05 13:36 ` Erez Zilber @ 2008-05-05 17:49 ` Mike Christie 2008-05-07 15:53 ` Pete Wyckoff 1 sibling, 1 reply; 26+ messages in thread From: Mike Christie @ 2008-05-05 17:49 UTC (permalink / raw) To: Pete Wyckoff; +Cc: Erez Zilber, Roland Dreier, linux-scsi Pete Wyckoff wrote: > 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 <pw@osc.edu> > --- > drivers/infiniband/ulp/iser/iscsi_iser.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c > index 1b272a6..78f3242 100644 > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > @@ -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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/3] iscsi iser: increase max_sectors 2008-05-05 17:49 ` Mike Christie @ 2008-05-07 15:53 ` Pete Wyckoff 2008-05-12 12:10 ` Erez Zilber 0 siblings, 1 reply; 26+ messages in thread From: Pete Wyckoff @ 2008-05-07 15:53 UTC (permalink / raw) To: Mike Christie; +Cc: Erez Zilber, Roland Dreier, linux-scsi michaelc@cs.wisc.edu wrote on Mon, 05 May 2008 12:49 -0500: > Pete Wyckoff wrote: >> 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 <pw@osc.edu> >> --- >> drivers/infiniband/ulp/iser/iscsi_iser.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c >> index 1b272a6..78f3242 100644 >> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c >> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c >> @@ -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. 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. rdreier@cisco.com wrote on Mon, 05 May 2008 13:43 -0700: > > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/3] iscsi iser: increase max_sectors 2008-05-07 15:53 ` Pete Wyckoff @ 2008-05-12 12:10 ` Erez Zilber 0 siblings, 0 replies; 26+ messages in thread From: Erez Zilber @ 2008-05-12 12:10 UTC (permalink / raw) To: Pete Wyckoff, Roland Dreier; +Cc: Mike Christie, linux-scsi, Eli Dorfman Pete Wyckoff wrote: > michaelc@cs.wisc.edu wrote on Mon, 05 May 2008 12:49 -0500: > >> Pete Wyckoff wrote: >> >>> 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 <pw@osc.edu> >>> --- >>> drivers/infiniband/ulp/iser/iscsi_iser.c | 2 +- >>> 1 files changed, 1 insertions(+), 1 deletions(-) >>> >>> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c >>> index 1b272a6..78f3242 100644 >>> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c >>> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c >>> @@ -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. >> > > 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. > > rdreier@cisco.com wrote on Mon, 05 May 2008 13:43 -0700: > >> > 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. > > 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. > The goal with these patches is to increase througphut for big > transfers. > Are you interested in transfers > 2MB? Erez > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/3] iscsi iser: increase sg_tablesize 2008-02-12 20:52 [PATCH 0/3] iscsi iser limits Pete Wyckoff 2008-02-12 20:54 ` [PATCH 1/3] iscsi iser: remove DMA restrictions Pete Wyckoff 2008-02-12 20:54 ` [PATCH 2/3] iscsi iser: increase max_sectors Pete Wyckoff @ 2008-02-12 20:54 ` Pete Wyckoff 2008-03-02 13:56 ` [PATCH 0/3] iscsi iser limits Erez Zilber 3 siblings, 0 replies; 26+ messages in thread From: Pete Wyckoff @ 2008-02-12 20:54 UTC (permalink / raw) To: Mike Christie, Erez Zilber, Roland Dreier; +Cc: linux-scsi 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 <pw@osc.edu> --- drivers/infiniband/ulp/iser/iscsi_iser.h | 3 ++- drivers/infiniband/ulp/iser/iser_verbs.c | 6 ++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h index 1ee867b..db8f81a 100644 --- a/drivers/infiniband/ulp/iser/iscsi_iser.h +++ b/drivers/infiniband/ulp/iser/iscsi_iser.h @@ -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 diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c index 714b8db..c5b374f 100644 --- a/drivers/infiniband/ulp/iser/iser_verbs.c +++ b/drivers/infiniband/ulp/iser/iser_verbs.c @@ -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; -- 1.5.3.8 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] iscsi iser limits 2008-02-12 20:52 [PATCH 0/3] iscsi iser limits Pete Wyckoff ` (2 preceding siblings ...) 2008-02-12 20:54 ` [PATCH 3/3] iscsi iser: increase sg_tablesize Pete Wyckoff @ 2008-03-02 13:56 ` Erez Zilber 3 siblings, 0 replies; 26+ messages in thread From: Erez Zilber @ 2008-03-02 13:56 UTC (permalink / raw) To: Pete Wyckoff; +Cc: Mike Christie, Roland Dreier, linux-scsi Pete Wyckoff wrote: > 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, In the future, please send iSER patches also to general@lists.openfabrics.org. Thanks, Erez ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2008-05-12 12:09 UTC | newest] Thread overview: 26+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-02-12 20:52 [PATCH 0/3] iscsi iser limits Pete Wyckoff 2008-02-12 20:54 ` [PATCH 1/3] iscsi iser: remove DMA restrictions Pete Wyckoff 2008-02-12 21:10 ` James Bottomley 2008-02-12 21:46 ` Pete Wyckoff 2008-02-12 21:57 ` James Bottomley 2008-02-13 19:59 ` Pete Wyckoff 2008-02-14 21:10 ` [PATCH 1/3 v2] iscsi iser: remove DMA alignment restriction Pete Wyckoff 2008-05-05 13:19 ` Erez Zilber 2008-04-21 13:51 ` [PATCH 1/3] iscsi iser: remove DMA restrictions Erez Zilber 2008-04-23 13:41 ` [ofa-general] " Erez Zilber 2008-04-23 16:33 ` Mike Christie 2008-04-23 17:16 ` Mike Christie 2008-04-23 17:43 ` Mike Christie 2008-02-14 17:56 ` Mike Christie 2008-02-14 18:10 ` James Bottomley 2008-02-14 18:21 ` Mike Christie 2008-02-14 18:34 ` Mike Christie 2008-02-14 19:04 ` Mike Christie 2008-02-12 20:54 ` [PATCH 2/3] iscsi iser: increase max_sectors Pete Wyckoff 2008-05-05 13:36 ` Erez Zilber 2008-05-05 20:43 ` Roland Dreier 2008-05-05 17:49 ` Mike Christie 2008-05-07 15:53 ` Pete Wyckoff 2008-05-12 12:10 ` Erez Zilber 2008-02-12 20:54 ` [PATCH 3/3] iscsi iser: increase sg_tablesize Pete Wyckoff 2008-03-02 13:56 ` [PATCH 0/3] iscsi iser limits Erez Zilber
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).