| Message ID | 20191021021030.1037-2-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes related to the DMA max_segment_size parameter | expand |
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
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.
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
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.
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); } /**
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(-)