[PATCH 1/4] RDMA/core: Fix ib_dma_max_seg_size()

[PATCH 1/4] RDMA/core: Fix ib_dma_max_seg_size()

[Bart Van Assche]

If dev->dma_device->params == NULL then the maximum DMA segment size is
64 KB. See also the dma_get_max_seg_size() implementation. This patch
fixes the following kernel warning:

DMA-API: infiniband rxe0: mapping sg segment longer than device claims to support [len=126976] [max=65536]
WARNING: CPU: 4 PID: 4848 at kernel/dma/debug.c:1220 debug_dma_map_sg+0x3d9/0x450
RIP: 0010:debug_dma_map_sg+0x3d9/0x450
Call Trace:
 srp_queuecommand+0x626/0x18d0 [ib_srp]
 scsi_queue_rq+0xd02/0x13e0 [scsi_mod]
 __blk_mq_try_issue_directly+0x2b3/0x3f0
 blk_mq_request_issue_directly+0xac/0xf0
 blk_insert_cloned_request+0xdf/0x170
 dm_mq_queue_rq+0x43d/0x830 [dm_mod]
 __blk_mq_try_issue_directly+0x2b3/0x3f0
 blk_mq_request_issue_directly+0xac/0xf0
 blk_mq_try_issue_list_directly+0xb8/0x170
 blk_mq_sched_insert_requests+0x23c/0x3b0
 blk_mq_flush_plug_list+0x529/0x730
 blk_flush_plug_list+0x21f/0x260
 blk_mq_make_request+0x56b/0xf20
 generic_make_request+0x196/0x660
 submit_bio+0xae/0x290
 blkdev_direct_IO+0x822/0x900
 generic_file_direct_write+0x110/0x200
 __generic_file_write_iter+0x124/0x2a0
 blkdev_write_iter+0x168/0x270
 aio_write+0x1c4/0x310
 io_submit_one+0x971/0x1390
 __x64_sys_io_submit+0x12a/0x390
 do_syscall_64+0x6f/0x2e0
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Cc: Christoph Hellwig <hch@lst.de>
Cc: <stable@vger.kernel.org>
Fixes: 0b5cb3300ae5 ("RDMA/srp: Increase max_segment_size")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/rdma/ib_verbs.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 6a47ba85c54c..e6c167d03aae 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -4043,9 +4043,7 @@ static inline void ib_dma_unmap_sg_attrs(struct ib_device *dev,
  */
 static inline unsigned int ib_dma_max_seg_size(struct ib_device *dev)
 {
-	struct device_dma_parameters *p = dev->dma_device->dma_parms;
-
-	return p ? p->max_segment_size : UINT_MAX;
+	return dma_get_max_seg_size(dev->dma_device);
 }
 
 /**
-- 
2.23.0

Re: [PATCH 1/4] RDMA/core: Fix ib_dma_max_seg_size()

[Jason Gunthorpe]

On Sun, Oct 20, 2019 at 07:10:27PM -0700, Bart Van Assche wrote:
> If dev->dma_device->params == NULL then the maximum DMA segment size is
> 64 KB. See also the dma_get_max_seg_size() implementation. This patch
> fixes the following kernel warning:
> 
> DMA-API: infiniband rxe0: mapping sg segment longer than device claims to support [len=126976] [max=65536]
> WARNING: CPU: 4 PID: 4848 at kernel/dma/debug.c:1220 debug_dma_map_sg+0x3d9/0x450
> RIP: 0010:debug_dma_map_sg+0x3d9/0x450
> Call Trace:
>  srp_queuecommand+0x626/0x18d0 [ib_srp]
>  scsi_queue_rq+0xd02/0x13e0 [scsi_mod]
>  __blk_mq_try_issue_directly+0x2b3/0x3f0
>  blk_mq_request_issue_directly+0xac/0xf0
>  blk_insert_cloned_request+0xdf/0x170
>  dm_mq_queue_rq+0x43d/0x830 [dm_mod]
>  __blk_mq_try_issue_directly+0x2b3/0x3f0
>  blk_mq_request_issue_directly+0xac/0xf0
>  blk_mq_try_issue_list_directly+0xb8/0x170
>  blk_mq_sched_insert_requests+0x23c/0x3b0
>  blk_mq_flush_plug_list+0x529/0x730
>  blk_flush_plug_list+0x21f/0x260
>  blk_mq_make_request+0x56b/0xf20
>  generic_make_request+0x196/0x660
>  submit_bio+0xae/0x290
>  blkdev_direct_IO+0x822/0x900
>  generic_file_direct_write+0x110/0x200
>  __generic_file_write_iter+0x124/0x2a0
>  blkdev_write_iter+0x168/0x270
>  aio_write+0x1c4/0x310
>  io_submit_one+0x971/0x1390
>  __x64_sys_io_submit+0x12a/0x390
>  do_syscall_64+0x6f/0x2e0
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: <stable@vger.kernel.org>
> Fixes: 0b5cb3300ae5 ("RDMA/srp: Increase max_segment_size")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>  include/rdma/ib_verbs.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 6a47ba85c54c..e6c167d03aae 100644
> +++ b/include/rdma/ib_verbs.h
> @@ -4043,9 +4043,7 @@ static inline void ib_dma_unmap_sg_attrs(struct ib_device *dev,
>   */
>  static inline unsigned int ib_dma_max_seg_size(struct ib_device *dev)
>  {
> -	struct device_dma_parameters *p = dev->dma_device->dma_parms;
> -
> -	return p ? p->max_segment_size : UINT_MAX;
> +	return dma_get_max_seg_size(dev->dma_device);
>  }

Should we get rid of this wrapper?

Jason

Re: [PATCH 1/4] RDMA/core: Fix ib_dma_max_seg_size()

[Bart Van Assche]

On 10/21/19 7:09 AM, Jason Gunthorpe wrote:
> On Sun, Oct 20, 2019 at 07:10:27PM -0700, Bart Van Assche wrote:
>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>> index 6a47ba85c54c..e6c167d03aae 100644
>> +++ b/include/rdma/ib_verbs.h
>> @@ -4043,9 +4043,7 @@ static inline void ib_dma_unmap_sg_attrs(struct ib_device *dev,
>>    */
>>   static inline unsigned int ib_dma_max_seg_size(struct ib_device *dev)
>>   {
>> -	struct device_dma_parameters *p = dev->dma_device->dma_parms;
>> -
>> -	return p ? p->max_segment_size : UINT_MAX;
>> +	return dma_get_max_seg_size(dev->dma_device);
>>   }
> 
> Should we get rid of this wrapper?

Hi Jason,

In general I agree that getting rid of single line inline functions is 
good. In this case however I'd like to keep the wrapper such that RDMA 
ULP code does not have to deal with the choice between dev->dma_device 
and &dev->dev. From struct ib_device:
  /* Do not access @dma_device directly from ULP nor from HW drivers. */
struct device                *dma_device;

Thanks,

Bart.

Re: [PATCH 1/4] RDMA/core: Fix ib_dma_max_seg_size()

[Jason Gunthorpe]

On Mon, Oct 21, 2019 at 08:03:32AM -0700, Bart Van Assche wrote:
> On 10/21/19 7:09 AM, Jason Gunthorpe wrote:
> > On Sun, Oct 20, 2019 at 07:10:27PM -0700, Bart Van Assche wrote:
> > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> > > index 6a47ba85c54c..e6c167d03aae 100644
> > > +++ b/include/rdma/ib_verbs.h
> > > @@ -4043,9 +4043,7 @@ static inline void ib_dma_unmap_sg_attrs(struct ib_device *dev,
> > >    */
> > >   static inline unsigned int ib_dma_max_seg_size(struct ib_device *dev)
> > >   {
> > > -	struct device_dma_parameters *p = dev->dma_device->dma_parms;
> > > -
> > > -	return p ? p->max_segment_size : UINT_MAX;
> > > +	return dma_get_max_seg_size(dev->dma_device);
> > >   }
> > 
> > Should we get rid of this wrapper?
> 
> Hi Jason,
> 
> In general I agree that getting rid of single line inline functions is good.
> In this case however I'd like to keep the wrapper such that RDMA ULP code
> does not have to deal with the choice between dev->dma_device and &dev->dev.
> From struct ib_device:
>  /* Do not access @dma_device directly from ULP nor from HW drivers. */
> struct device                *dma_device;

Do you think it is a mistake we have dma_device at all?

Can the modern dma framework let us make the 'struct ib_device' into a
full dma_device that is still connected to some PCI device?

Jason

Re: [PATCH 1/4] RDMA/core: Fix ib_dma_max_seg_size()

[Bart Van Assche]

On 10/21/19 8:27 AM, Jason Gunthorpe wrote:
> On Mon, Oct 21, 2019 at 08:03:32AM -0700, Bart Van Assche wrote:
>> On 10/21/19 7:09 AM, Jason Gunthorpe wrote:
>>> On Sun, Oct 20, 2019 at 07:10:27PM -0700, Bart Van Assche wrote:
>>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>>>> index 6a47ba85c54c..e6c167d03aae 100644
>>>> +++ b/include/rdma/ib_verbs.h
>>>> @@ -4043,9 +4043,7 @@ static inline void ib_dma_unmap_sg_attrs(struct ib_device *dev,
>>>>     */
>>>>    static inline unsigned int ib_dma_max_seg_size(struct ib_device *dev)
>>>>    {
>>>> -	struct device_dma_parameters *p = dev->dma_device->dma_parms;
>>>> -
>>>> -	return p ? p->max_segment_size : UINT_MAX;
>>>> +	return dma_get_max_seg_size(dev->dma_device);
>>>>    }
>>>
>>> Should we get rid of this wrapper?
>>
>> Hi Jason,
>>
>> In general I agree that getting rid of single line inline functions is good.
>> In this case however I'd like to keep the wrapper such that RDMA ULP code
>> does not have to deal with the choice between dev->dma_device and &dev->dev.
>>  From struct ib_device:
>>   /* Do not access @dma_device directly from ULP nor from HW drivers. */
>> struct device                *dma_device;
> 
> Do you think it is a mistake we have dma_device at all?
> 
> Can the modern dma framework let us make the 'struct ib_device' into a
> full dma_device that is still connected to some PCI device?

Hi Jason,

My understanding is that dma_device is passed as the first argument to 
DMA mapping functions. Before PCIe P2P support was introduced in the 
RDMA code, the only struct device members used by DMA mapping functions 
were dma_ops, dma_mask, coherent_dma_mask, bus_dma_mask and dma_parms. I 
think it would have been sufficient to copy all these members from the 
PCI device into struct ib_device before PCIe P2P support was introduced. 
The dma_device pointer however is also passed to the pci_p2pdma_map_sg() 
and pci_p2pdma_unmap_sg() functions. These functions use several 
additional members of struct pci_dev. Although it may be possible to 
eliminate the dma_device member, this may make maintaining the RDMA core 
harder because setup_dma_device() will have to be modified every time a 
DMA mapping function uses an additional member from struct device.

Bart.