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

[Mike Christie]

Mike Christie wrote:
> Mike Christie wrote:
>> Erez Zilber wrote:
>>> Erez Zilber wrote:
>>>> Pete Wyckoff wrote:
>>>>> James.Bottomley at 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 at 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 at 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 at 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.