| Message ID | 20191021021030.1037-3-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: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
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); + } + } } /*
> 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
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.
> 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.
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
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.
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); + } + } } /*
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(-)