From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Christie Subject: Re: [ofa-general] Re: [PATCH 1/3] iscsi iser: remove DMA restrictions Date: Wed, 23 Apr 2008 12:43:30 -0500 Message-ID: <480F7542.4070000@cs.wisc.edu> References: <20080212205252.GB13643@osc.edu> <20080212205403.GC13643@osc.edu><1202850645.3137.132.camel@localhost.localdomain><20080212214632.GA14397@osc.edu><1202853468.3137.148.camel@localhost.localdomain><20080213195912.GC7372@osc.edu> <480C9BF8.9050401@Voltaire.COM> <480F3C84.40606@Voltaire.COM> <480F64ED.7010705@cs.wisc.edu> <480F6EDA.9050004@cs.wisc.edu> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <480F6EDA.9050004@cs.wisc.edu> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: general-bounces@lists.openfabrics.org Errors-To: general-bounces@lists.openfabrics.org To: Erez Zilber Cc: James Bottomley , general@lists.openfabrics.org, linux-scsi List-Id: linux-scsi@vger.kernel.org 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 >>>>>>>>> --- >>>>>>>>> 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 >>>>>>> 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.