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

[Mike Christie]

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?

> 
> 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?