[PATCH 0/4] Fixes related to the DMA max_segment_size parameter

[PATCH 0/4] Fixes related to the DMA max_segment_size parameter

[Bart Van Assche]

Hi Jason,

These four patches are what I came up with while analyzing a kernel warning
triggered by the ib_srp kernel driver. Please consider these patches for
inclusion in the Linux kernel.

Thanks,

Bart.

Bart Van Assche (4):
  RDMA/core: Fix ib_dma_max_seg_size()
  RDMA/core: Set DMA parameters correctly
  rdma_rxe: Increase DMA max_segment_size parameter
  siw: Increase DMA max_segment_size parameter

 drivers/infiniband/core/device.c      | 13 +++++++++++--
 drivers/infiniband/sw/rxe/rxe_verbs.c |  3 +++
 drivers/infiniband/sw/rxe/rxe_verbs.h |  1 +
 drivers/infiniband/sw/siw/siw.h       |  1 +
 drivers/infiniband/sw/siw/siw_main.c  |  3 +++
 include/rdma/ib_verbs.h               |  4 +---
 6 files changed, 20 insertions(+), 5 deletions(-)

-- 
2.23.0

[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

[PATCH 2/4] RDMA/core: Set DMA parameters correctly

[Bart Van Assche]

The effect of the dma_set_max_seg_size() call in setup_dma_device() is
as follows:
- If device->dev.dma_parms is NULL, that call has no effect at all.
- If device->dev.dma_parms is not NULL, since that pointer points at
  the DMA parameters of the parent device, modify the DMA limits of the
  parent device.

Both actions are wrong. Instead of changing the DMA parameters of the
parent device, use the DMA parameters of the parent device if these
parameters are available.

Compile-tested only.

Cc: Michael J. Ruhl <michael.j.ruhl@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Adit Ranadive <aditr@vmware.com>
Cc: Shiraz Saleem <shiraz.saleem@intel.com>
Cc: Gal Pressman <galpress@amazon.com>
Cc: Selvin Xavier <selvin.xavier@broadcom.com>
Fixes: d10bcf947a3e ("RDMA/umem: Combine contiguous PAGE_SIZE regions in SGEs")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/core/device.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 536310fb6510..b33d684a2a99 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -1199,9 +1199,18 @@ static void setup_dma_device(struct ib_device *device)
 		WARN_ON_ONCE(!parent);
 		device->dma_device = parent;
 	}
-	/* Setup default max segment size for all IB devices */
-	dma_set_max_seg_size(device->dma_device, SZ_2G);
 
+	if (!device->dev.dma_parms) {
+		if (parent) {
+			/*
+			 * The caller did not provide DMA parameters. Use the
+			 * DMA parameters of the parent device.
+			 */
+			device->dev.dma_parms = parent->dma_parms;
+		} else {
+			WARN_ON_ONCE(true);
+		}
+	}
 }
 
 /*
-- 
2.23.0

[PATCH 3/4] rdma_rxe: Increase DMA max_segment_size parameter

[Bart Van Assche]

Increase the DMA max_segment_size parameter from 64 KB to UINT_MAX.

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/sw/rxe/rxe_verbs.c | 3 +++
 drivers/infiniband/sw/rxe/rxe_verbs.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index fa47bdcc7f54..2ec085b53907 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
@@ -1175,6 +1175,9 @@ int rxe_register_device(struct rxe_dev *rxe, const char *ibdev_name)
 	addrconf_addr_eui48((unsigned char *)&dev->node_guid,
 			    rxe->ndev->dev_addr);
 	dev->dev.dma_ops = &dma_virt_ops;
+	dev->dev.dma_parms = &rxe->dma_parms;
+	rxe->dma_parms = (struct device_dma_parameters)
+		{ .max_segment_size = UINT_MAX };
 	dma_coerce_mask_and_coherent(&dev->dev,
 				     dma_get_required_mask(&dev->dev));
 
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h
index 5c4b2239129c..95834206c80c 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.h
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.h
@@ -384,6 +384,7 @@ struct rxe_port {
 struct rxe_dev {
 	struct ib_device	ib_dev;
 	struct ib_device_attr	attr;
+	struct device_dma_parameters dma_parms;
 	int			max_ucontext;
 	int			max_inline_data;
 	struct mutex	usdev_lock;
-- 
2.23.0

[PATCH 4/4] siw: Increase DMA max_segment_size parameter

[Bart Van Assche]

Increase the DMA max_segment_size parameter from 64 KB to UINT_MAX.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Bernard Metzler <bmt@zurich.ibm.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/sw/siw/siw.h      | 1 +
 drivers/infiniband/sw/siw/siw_main.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h
index dba4535494ab..1ea3ed249e7b 100644
--- a/drivers/infiniband/sw/siw/siw.h
+++ b/drivers/infiniband/sw/siw/siw.h
@@ -70,6 +70,7 @@ struct siw_pd {
 
 struct siw_device {
 	struct ib_device base_dev;
+	struct device_dma_parameters dma_parms;
 	struct net_device *netdev;
 	struct siw_dev_cap attrs;
 
diff --git a/drivers/infiniband/sw/siw/siw_main.c b/drivers/infiniband/sw/siw/siw_main.c
index d1a1b7aa7d83..041496376047 100644
--- a/drivers/infiniband/sw/siw/siw_main.c
+++ b/drivers/infiniband/sw/siw/siw_main.c
@@ -402,6 +402,9 @@ static struct siw_device *siw_device_create(struct net_device *netdev)
 	base_dev->phys_port_cnt = 1;
 	base_dev->dev.parent = parent;
 	base_dev->dev.dma_ops = &dma_virt_ops;
+	base_dev->dev.dma_parms = &sdev->dma_parms;
+	sdev->dma_parms = (struct device_dma_parameters)
+		{ .max_segment_size = UINT_MAX };
 	base_dev->num_comp_vectors = num_possible_cpus();
 
 	ib_set_device_ops(base_dev, &siw_device_ops);
-- 
2.23.0

Re: [PATCH 4/4] siw: Increase DMA max_segment_size parameter

[Bernard Metzler]

-----"Bart Van Assche" <bvanassche@acm.org> wrote: -----

>To: "Jason Gunthorpe" <jgg@ziepe.ca>
>From: "Bart Van Assche" <bvanassche@acm.org>
>Date: 10/21/2019 04:10AM
>Cc: "Leon Romanovsky" <leonro@mellanox.com>, "Doug Ledford"
><dledford@redhat.com>, linux-rdma@vger.kernel.org, "Bart Van Assche"
><bvanassche@acm.org>, "Christoph Hellwig" <hch@lst.de>, "Bernard
>Metzler" <bmt@zurich.ibm.com>
>Subject: [EXTERNAL] [PATCH 4/4] siw: Increase DMA max_segment_size
>parameter
>
>Increase the DMA max_segment_size parameter from 64 KB to UINT_MAX.
>

Hi Bart,
Why don't we make device_dma_parameters siw_dma_params 
just a const in siw_main.c? Having it per siw_device suggests
more flexibility than we actually need and support? Probably
true as well for rxe driver. This is all driver specific.

Independent of this current patch, probably even true for
siw_device.attrs. We do not have those capabilities siw
device specific, but just siw driver specific.

Best regards,
Bernard.
>Cc: Christoph Hellwig <hch@lst.de>
>Cc: Bernard Metzler <bmt@zurich.ibm.com>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>---
> drivers/infiniband/sw/siw/siw.h      | 1 +
> drivers/infiniband/sw/siw/siw_main.c | 3 +++
> 2 files changed, 4 insertions(+)
>
>diff --git a/drivers/infiniband/sw/siw/siw.h
>b/drivers/infiniband/sw/siw/siw.h
>index dba4535494ab..1ea3ed249e7b 100644
>--- a/drivers/infiniband/sw/siw/siw.h
>+++ b/drivers/infiniband/sw/siw/siw.h
>@@ -70,6 +70,7 @@ struct siw_pd {
> 
> struct siw_device {
> 	struct ib_device base_dev;
>+	struct device_dma_parameters dma_parms;
> 	struct net_device *netdev;
> 	struct siw_dev_cap attrs;
> 
>diff --git a/drivers/infiniband/sw/siw/siw_main.c
>b/drivers/infiniband/sw/siw/siw_main.c
>index d1a1b7aa7d83..041496376047 100644
>--- a/drivers/infiniband/sw/siw/siw_main.c
>+++ b/drivers/infiniband/sw/siw/siw_main.c
>@@ -402,6 +402,9 @@ static struct siw_device
>*siw_device_create(struct net_device *netdev)
> 	base_dev->phys_port_cnt = 1;
> 	base_dev->dev.parent = parent;
> 	base_dev->dev.dma_ops = &dma_virt_ops;
>+	base_dev->dev.dma_parms = &sdev->dma_parms;
>+	sdev->dma_parms = (struct device_dma_parameters)
>+		{ .max_segment_size = UINT_MAX };
> 	base_dev->num_comp_vectors = num_possible_cpus();
> 
> 	ib_set_device_ops(base_dev, &siw_device_ops);
>-- 
>2.23.0
>
>

Re: [PATCH 4/4] siw: Increase DMA max_segment_size parameter

[Bart Van Assche]

On 10/21/19 2:36 AM, Bernard Metzler wrote:
> Why don't we make device_dma_parameters siw_dma_params 
> just a const in siw_main.c? Having it per siw_device suggests
> more flexibility than we actually need and support? Probably
> true as well for rxe driver. This is all driver specific.
> 
> Independent of this current patch, probably even true for
> siw_device.attrs. We do not have those capabilities siw
> device specific, but just siw driver specific.

Hi Bernard,

The dma_parms pointer in struct device has not been declared const.
Assigning a pointer to a const structure to the dma_parms member of
struct device would trigger a compiler warning. I think everyone wants
to keep the RDMA code free from compiler warnings.

Thanks,

Bart.

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 2/4] RDMA/core: Set DMA parameters correctly

[Jason Gunthorpe]

On Sun, Oct 20, 2019 at 07:10:28PM -0700, Bart Van Assche wrote:
> The effect of the dma_set_max_seg_size() call in setup_dma_device() is
> as follows:
> - If device->dev.dma_parms is NULL, that call has no effect at all.
> - If device->dev.dma_parms is not NULL, since that pointer points at
>   the DMA parameters of the parent device, modify the DMA limits of the
>   parent device.
> 
> Both actions are wrong. Instead of changing the DMA parameters of the
> parent device, use the DMA parameters of the parent device if these
> parameters are available.
> 
> Compile-tested only.
> 
> Cc: Michael J. Ruhl <michael.j.ruhl@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Adit Ranadive <aditr@vmware.com>
> Cc: Shiraz Saleem <shiraz.saleem@intel.com>
> Cc: Gal Pressman <galpress@amazon.com>
> Cc: Selvin Xavier <selvin.xavier@broadcom.com>
> Fixes: d10bcf947a3e ("RDMA/umem: Combine contiguous PAGE_SIZE regions in SGEs")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>  drivers/infiniband/core/device.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 536310fb6510..b33d684a2a99 100644
> +++ b/drivers/infiniband/core/device.c
> @@ -1199,9 +1199,18 @@ static void setup_dma_device(struct ib_device *device)
>  		WARN_ON_ONCE(!parent);
>  		device->dma_device = parent;
>  	}
> -	/* Setup default max segment size for all IB devices */
> -	dma_set_max_seg_size(device->dma_device, SZ_2G);
>  
> +	if (!device->dev.dma_parms) {
> +		if (parent) {
> +			/*
> +			 * The caller did not provide DMA parameters. Use the
> +			 * DMA parameters of the parent device.
> +			 */
> +			device->dev.dma_parms = parent->dma_parms;
> +		} else {
> +			WARN_ON_ONCE(true);
> +		}
> +	}
>  }

We really do want to, by default, increase the max_seg_size above 64k
though, so this approach doesn't seem right either.

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.

Re: [PATCH 2/4] RDMA/core: Set DMA parameters correctly

[Bart Van Assche]

On 10/21/19 7:10 AM, Jason Gunthorpe wrote:
> On Sun, Oct 20, 2019 at 07:10:28PM -0700, Bart Van Assche wrote:
>> The effect of the dma_set_max_seg_size() call in setup_dma_device() is
>> as follows:
>> - If device->dev.dma_parms is NULL, that call has no effect at all.
>> - If device->dev.dma_parms is not NULL, since that pointer points at
>>    the DMA parameters of the parent device, modify the DMA limits of the
>>    parent device.
>>
>> Both actions are wrong. Instead of changing the DMA parameters of the
>> parent device, use the DMA parameters of the parent device if these
>> parameters are available.
>>
>> Compile-tested only.
>>
>> Cc: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> Cc: Adit Ranadive <aditr@vmware.com>
>> Cc: Shiraz Saleem <shiraz.saleem@intel.com>
>> Cc: Gal Pressman <galpress@amazon.com>
>> Cc: Selvin Xavier <selvin.xavier@broadcom.com>
>> Fixes: d10bcf947a3e ("RDMA/umem: Combine contiguous PAGE_SIZE regions in SGEs")
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>   drivers/infiniband/core/device.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
>> index 536310fb6510..b33d684a2a99 100644
>> +++ b/drivers/infiniband/core/device.c
>> @@ -1199,9 +1199,18 @@ static void setup_dma_device(struct ib_device *device)
>>   		WARN_ON_ONCE(!parent);
>>   		device->dma_device = parent;
>>   	}
>> -	/* Setup default max segment size for all IB devices */
>> -	dma_set_max_seg_size(device->dma_device, SZ_2G);
>>   
>> +	if (!device->dev.dma_parms) {
>> +		if (parent) {
>> +			/*
>> +			 * The caller did not provide DMA parameters. Use the
>> +			 * DMA parameters of the parent device.
>> +			 */
>> +			device->dev.dma_parms = parent->dma_parms;
>> +		} else {
>> +			WARN_ON_ONCE(true);
>> +		}
>> +	}
>>   }
> 
> We really do want to, by default, increase the max_seg_size above 64k
> though, so this approach doesn't seem right either.

How about replacing this patch by the patch below and by moving this patch to
the end of this series?

Thanks,

Bart.


Subject: [PATCH] RDMA/core: Set DMA parameters correctly

The dma_set_max_seg_size() call in setup_dma_device() does not have any
effect since device->dev.dma_parms is NULL. Fix this by initializing
device->dev.dma_parms first.

Cc: Michael J. Ruhl <michael.j.ruhl@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Adit Ranadive <aditr@vmware.com>
Cc: Shiraz Saleem <shiraz.saleem@intel.com>
Cc: Gal Pressman <galpress@amazon.com>
Cc: Selvin Xavier <selvin.xavier@broadcom.com>
Fixes: d10bcf947a3e ("RDMA/umem: Combine contiguous PAGE_SIZE regions in SGEs")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  drivers/infiniband/core/device.c | 16 ++++++++++++++--
  1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index a667636f74bf..a523d844ad9d 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -1199,9 +1199,21 @@ static void setup_dma_device(struct ib_device *device)
  		WARN_ON_ONCE(!parent);
  		device->dma_device = parent;
  	}
-	/* Setup default max segment size for all IB devices */
-	dma_set_max_seg_size(device->dma_device, SZ_2G);

+	if (!device->dev.dma_parms) {
+		if (parent) {
+			/*
+			 * The caller did not provide DMA parameters, so
+			 * 'parent' probably represents a PCI device. The PCI
+			 * core sets the maximum segment size to 64
+			 * KB. Increase this parameter to 2G.
+			 */
+			device->dev.dma_parms = parent->dma_parms;
+			dma_set_max_seg_size(device->dma_device, SZ_2G);
+		} else {
+			WARN_ON_ONCE(true);
+		}
+	}
  }

  /*

RE: [PATCH 2/4] RDMA/core: Set DMA parameters correctly

[Saleem, Shiraz]

> Subject: Re: [PATCH 2/4] RDMA/core: Set DMA parameters correctly
> 
> On 10/21/19 7:10 AM, Jason Gunthorpe wrote:
> > On Sun, Oct 20, 2019 at 07:10:28PM -0700, Bart Van Assche wrote:
> >> The effect of the dma_set_max_seg_size() call in setup_dma_device()
> >> is as follows:
> >> - If device->dev.dma_parms is NULL, that call has no effect at all.
> >> - If device->dev.dma_parms is not NULL, since that pointer points at
> >>    the DMA parameters of the parent device, modify the DMA limits of the
> >>    parent device.
> >>
> >> Both actions are wrong. Instead of changing the DMA parameters of the
> >> parent device, use the DMA parameters of the parent device if these
> >> parameters are available.
> >>
> >> Compile-tested only.
> >>
> >> Cc: Michael J. Ruhl <michael.j.ruhl@intel.com>
> >> Cc: Ira Weiny <ira.weiny@intel.com>
> >> Cc: Adit Ranadive <aditr@vmware.com>
> >> Cc: Shiraz Saleem <shiraz.saleem@intel.com>
> >> Cc: Gal Pressman <galpress@amazon.com>
> >> Cc: Selvin Xavier <selvin.xavier@broadcom.com>
> >> Fixes: d10bcf947a3e ("RDMA/umem: Combine contiguous PAGE_SIZE regions
> >> in SGEs")
> >> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> >>   drivers/infiniband/core/device.c | 13 +++++++++++--
> >>   1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/core/device.c
> >> b/drivers/infiniband/core/device.c
> >> index 536310fb6510..b33d684a2a99 100644
> >> +++ b/drivers/infiniband/core/device.c
> >> @@ -1199,9 +1199,18 @@ static void setup_dma_device(struct ib_device
> *device)
> >>   		WARN_ON_ONCE(!parent);
> >>   		device->dma_device = parent;
> >>   	}
> >> -	/* Setup default max segment size for all IB devices */
> >> -	dma_set_max_seg_size(device->dma_device, SZ_2G);
> >>
> >> +	if (!device->dev.dma_parms) {
> >> +		if (parent) {
> >> +			/*
> >> +			 * The caller did not provide DMA parameters. Use the
> >> +			 * DMA parameters of the parent device.
> >> +			 */
> >> +			device->dev.dma_parms = parent->dma_parms;
> >> +		} else {
> >> +			WARN_ON_ONCE(true);
> >> +		}
> >> +	}
> >>   }
> >
> > We really do want to, by default, increase the max_seg_size above 64k
> > though, so this approach doesn't seem right either.
> 
> How about replacing this patch by the patch below and by moving this patch to
> the end of this series?
> 
> Thanks,
> 
> Bart.
> 
> 
> Subject: [PATCH] RDMA/core: Set DMA parameters correctly
> 
> The dma_set_max_seg_size() call in setup_dma_device() does not have any effect
> since device->dev.dma_parms is NULL. Fix this by initializing
> device->dev.dma_parms first.
> 
> Cc: Michael J. Ruhl <michael.j.ruhl@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Adit Ranadive <aditr@vmware.com>
> Cc: Shiraz Saleem <shiraz.saleem@intel.com>
> Cc: Gal Pressman <galpress@amazon.com>
> Cc: Selvin Xavier <selvin.xavier@broadcom.com>
> Fixes: d10bcf947a3e ("RDMA/umem: Combine contiguous PAGE_SIZE regions in
> SGEs")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/infiniband/core/device.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index a667636f74bf..a523d844ad9d 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -1199,9 +1199,21 @@ static void setup_dma_device(struct ib_device *device)
>   		WARN_ON_ONCE(!parent);
>   		device->dma_device = parent;
>   	}
> -	/* Setup default max segment size for all IB devices */
> -	dma_set_max_seg_size(device->dma_device, SZ_2G);
> 
> +	if (!device->dev.dma_parms) {
> +		if (parent) {
> +			/*
> +			 * The caller did not provide DMA parameters, so
> +			 * 'parent' probably represents a PCI device. The PCI
> +			 * core sets the maximum segment size to 64
> +			 * KB. Increase this parameter to 2G.
> +			 */
> +			device->dev.dma_parms = parent->dma_parms;
> +			dma_set_max_seg_size(device->dma_device, SZ_2G);

Did you mean dma_set_max_seg_size(&device->dev, SZ_2G)?

device->dma_device could be pointing to parent if the caller
did not provide dma_ops. So wont this update the parent device
dma params?

Also do we want to ensure all callers device max_seg_sz
params >= threshold (=2G)? If so, perhaps we can do something
similar to vb2_dma_contig_set_max_seg_size()

https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/media/common/videobuf2/videobuf2-dma-contig.c#L734

Re: [PATCH 2/4] RDMA/core: Set DMA parameters correctly

[Bart Van Assche]

On 10/21/19 10:44 AM, Saleem, Shiraz wrote:
>> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
>> index a667636f74bf..a523d844ad9d 100644
>> --- a/drivers/infiniband/core/device.c
>> +++ b/drivers/infiniband/core/device.c
>> @@ -1199,9 +1199,21 @@ static void setup_dma_device(struct ib_device *device)
>>    		WARN_ON_ONCE(!parent);
>>    		device->dma_device = parent;
>>    	}
>> -	/* Setup default max segment size for all IB devices */
>> -	dma_set_max_seg_size(device->dma_device, SZ_2G);
>>
>> +	if (!device->dev.dma_parms) {
>> +		if (parent) {
>> +			/*
>> +			 * The caller did not provide DMA parameters, so
>> +			 * 'parent' probably represents a PCI device. The PCI
>> +			 * core sets the maximum segment size to 64
>> +			 * KB. Increase this parameter to 2G.
>> +			 */
>> +			device->dev.dma_parms = parent->dma_parms;
>> +			dma_set_max_seg_size(device->dma_device, SZ_2G);
> 
> Did you mean dma_set_max_seg_size(&device->dev, SZ_2G)?

Have you realized that that call has the same effect as what I proposed 
since both devices share the dma_parms parameter?

> device->dma_device could be pointing to parent if the caller
> did not provide dma_ops. So wont this update the parent device
> dma params?

That's correct, this will update the parent device DMA parameters.

> Also do we want to ensure all callers device max_seg_sz
> params >= threshold (=2G)? If so, perhaps we can do something
> similar to vb2_dma_contig_set_max_seg_size()
> 
> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/media/common/videobuf2/videobuf2-dma-contig.c#L734

It depends on what PCIe RDMA adapters support. If all PCIe RDMA adapters 
supported by the Linux kernel support max_segment_size >= 2G the above 
code is probably the easiest approach.

Thanks,

Bart.

RE: [PATCH 2/4] RDMA/core: Set DMA parameters correctly

[Saleem, Shiraz]

> Subject: Re: [PATCH 2/4] RDMA/core: Set DMA parameters correctly
> 
> On 10/21/19 10:44 AM, Saleem, Shiraz wrote:
> >> diff --git a/drivers/infiniband/core/device.c
> >> b/drivers/infiniband/core/device.c
> >> index a667636f74bf..a523d844ad9d 100644
> >> --- a/drivers/infiniband/core/device.c
> >> +++ b/drivers/infiniband/core/device.c
> >> @@ -1199,9 +1199,21 @@ static void setup_dma_device(struct ib_device
> *device)
> >>    		WARN_ON_ONCE(!parent);
> >>    		device->dma_device = parent;
> >>    	}
> >> -	/* Setup default max segment size for all IB devices */
> >> -	dma_set_max_seg_size(device->dma_device, SZ_2G);
> >>
> >> +	if (!device->dev.dma_parms) {
> >> +		if (parent) {
> >> +			/*
> >> +			 * The caller did not provide DMA parameters, so
> >> +			 * 'parent' probably represents a PCI device. The PCI
> >> +			 * core sets the maximum segment size to 64
> >> +			 * KB. Increase this parameter to 2G.
> >> +			 */
> >> +			device->dev.dma_parms = parent->dma_parms;
> >> +			dma_set_max_seg_size(device->dma_device, SZ_2G);
> >
> > Did you mean dma_set_max_seg_size(&device->dev, SZ_2G)?
> 
> Have you realized that that call has the same effect as what I proposed since both
> devices share the dma_parms parameter?
> 
I hadn’t. This makes more sense now.

Re: [PATCH 2/4] RDMA/core: Set DMA parameters correctly

[Jason Gunthorpe]

On Mon, Oct 21, 2019 at 11:10:12AM -0700, Bart Van Assche wrote:
> > device->dma_device could be pointing to parent if the caller
> > did not provide dma_ops. So wont this update the parent device
> > dma params?
> 
> That's correct, this will update the parent device DMA parameters.
> 
> > Also do we want to ensure all callers device max_seg_sz
> > params >= threshold (=2G)? If so, perhaps we can do something
> > similar to vb2_dma_contig_set_max_seg_size()
> > 
> > https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/media/common/videobuf2/videobuf2-dma-contig.c#L734
> 
> It depends on what PCIe RDMA adapters support. If all PCIe RDMA adapters
> supported by the Linux kernel support max_segment_size >= 2G the above code
> is probably the easiest approach.

All drivers chop the sgls up into whatever size they support, unlike
block we don't need the dma mapper to produce sgls in specific formats.

Jason

Re: [PATCH 2/4] RDMA/core: Set DMA parameters correctly

[Bart Van Assche]

On 2019-10-22 07:40, Jason Gunthorpe wrote:
> All drivers chop the sgls up into whatever size they support, unlike
> block we don't need the dma mapper to produce sgls in specific formats.

Thanks Jason, this is very useful feedback. I will rework this patch
series based on that feedback.

Bart.