[2/4] RDMA/core: Set DMA parameters correctly
diff mbox series

Message ID 20191021021030.1037-3-bvanassche@acm.org
State Superseded
Headers show
Series
  • Fixes related to the DMA max_segment_size parameter
Related show

Commit Message

Bart Van Assche Oct. 21, 2019, 2:10 a.m. UTC
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(-)

Comments

Jason Gunthorpe Oct. 21, 2019, 2:10 p.m. UTC | #1
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
Bart Van Assche Oct. 21, 2019, 4:26 p.m. UTC | #2
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);
+		}
+	}
  }

  /*
Saleem, Shiraz Oct. 21, 2019, 5:44 p.m. UTC | #3
> 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
Bart Van Assche Oct. 21, 2019, 6:10 p.m. UTC | #4
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.
Saleem, Shiraz Oct. 21, 2019, 8:32 p.m. UTC | #5
> 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.
Jason Gunthorpe Oct. 22, 2019, 2:40 p.m. UTC | #6
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
Bart Van Assche Oct. 22, 2019, 10:11 p.m. UTC | #7
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.

Patch
diff mbox series

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);
+		}
+	}
 }
 
 /*