Re: [PATCH 3/6] RDMA/core: remove use of dma_virt_ops
On Thu, Nov 05, 2020 at 01:58:16PM -0400, Jason Gunthorpe wrote: > > I noticed there were a couple of places expecting dma_device to be set > > to !NULL: > > > > drivers/infiniband/core/umem.c: > > dma_get_max_seg_size(device->dma_device), sg, npages, > > drivers/nvme/host/rdma.c: ctrl->ctrl.numa_node = > > dev_to_node(ctrl->device->dev->dma_device); > > Don't know much about NUMA, but do you think the ib device setup > should autocopy the numa node from the dma_device to the ib_device and > this usage should just refer to the ib_device? FYI, I ended up just lifting the ibdev_to_node from rds to ib_verbs.h. That uses the parent pointer in the ib_device and should generally work ok. If not we can improve іt as we now have a proper abstraction. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/6] RDMA/core: remove use of dma_virt_ops
On Thu, Nov 05, 2020 at 01:52:53PM -0400, Jason Gunthorpe wrote: > On Thu, Nov 05, 2020 at 08:42:02AM +0100, Christoph Hellwig wrote: > > @@ -1341,7 +1322,14 @@ int ib_register_device(struct ib_device *device, > > const char *name, > > if (ret) > > return ret; > > > > - setup_dma_device(device, dma_device); > > + /* > > +* If the caller does not provide a DMA capable device then the IB core > > +* will set up ib_sge and scatterlist structures that stash the kernel > > +* virtual address into the address field. > > +*/ > > + device->dma_device = dma_device; > > + WARN_ON(dma_device && !dma_device->dma_parms); > > I noticed there were a couple of places expecting dma_device to be set > to !NULL: > > drivers/infiniband/core/umem.c: > dma_get_max_seg_size(device->dma_device), sg, npages, This needs to use ib_dma_max_seg_size. > drivers/nvme/host/rdma.c: ctrl->ctrl.numa_node = > dev_to_node(ctrl->device->dev->dma_device); > Don't know much about NUMA, but do you think the ib device setup > should autocopy the numa node from the dma_device to the ib_device and > this usage should just refer to the ib_device? IMHO we could add a ib_device_get_numa_node API or something like that, which uses dev_to_node on the DMA device is present and otherwise returns -1. If needed we can refine that later. > net/rds/ib.c: device->dma_device, > > No sure what to do about RDS.. Yikes, this is completely broken. We either need a wrapper for the dma_pool API, or get rid of it. Let me dig into that. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/6] RDMA/core: remove use of dma_virt_ops
On Thu, Nov 05, 2020 at 01:52:53PM -0400, Jason Gunthorpe wrote: > On Thu, Nov 05, 2020 at 08:42:02AM +0100, Christoph Hellwig wrote: > > @@ -1341,7 +1322,14 @@ int ib_register_device(struct ib_device *device, > > const char *name, > > if (ret) > > return ret; > > > > - setup_dma_device(device, dma_device); > > + /* > > +* If the caller does not provide a DMA capable device then the IB core > > +* will set up ib_sge and scatterlist structures that stash the kernel > > +* virtual address into the address field. > > +*/ > > + device->dma_device = dma_device; > > + WARN_ON(dma_device && !dma_device->dma_parms); > > I noticed there were a couple of places expecting dma_device to be set > to !NULL: > > drivers/infiniband/core/umem.c: > dma_get_max_seg_size(device->dma_device), sg, npages, > drivers/nvme/host/rdma.c: ctrl->ctrl.numa_node = > dev_to_node(ctrl->device->dev->dma_device); Don't know much about NUMA, but do you think the ib device setup should autocopy the numa node from the dma_device to the ib_device and this usage should just refer to the ib_device? Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/6] RDMA/core: remove use of dma_virt_ops
On Thu, Nov 05, 2020 at 08:42:02AM +0100, Christoph Hellwig wrote: > @@ -1341,7 +1322,14 @@ int ib_register_device(struct ib_device *device, const > char *name, > if (ret) > return ret; > > - setup_dma_device(device, dma_device); > + /* > + * If the caller does not provide a DMA capable device then the IB core > + * will set up ib_sge and scatterlist structures that stash the kernel > + * virtual address into the address field. > + */ > + device->dma_device = dma_device; > + WARN_ON(dma_device && !dma_device->dma_parms); I noticed there were a couple of places expecting dma_device to be set to !NULL: drivers/infiniband/core/umem.c: dma_get_max_seg_size(device->dma_device), sg, npages, drivers/nvme/host/rdma.c: ctrl->ctrl.numa_node = dev_to_node(ctrl->device->dev->dma_device); net/rds/ib.c: device->dma_device, No sure what to do about RDS.. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/6] RDMA/core: remove use of dma_virt_ops
On Thu, Nov 05, 2020 at 10:34:15AM -0400, Jason Gunthorpe wrote:
> On Thu, Nov 05, 2020 at 08:42:02AM +0100, Christoph Hellwig wrote:
> > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> > index 5f8fd7976034e0..661e4a22b3cb81 100644
> > +++ b/include/rdma/ib_verbs.h
> > @@ -3950,6 +3950,8 @@ static inline int ib_req_ncomp_notif(struct ib_cq
> > *cq, int wc_cnt)
> > */
> > static inline int ib_dma_mapping_error(struct ib_device *dev, u64 dma_addr)
> > {
> > + if (!dev->dma_device)
> > + return 0;
>
> How about:
>
> static inline bool ib_uses_virt_dma(struct ib_device *dev)
> {
> return IS_ENABLED(CONFIG_INFINIBAND_VIRT_DMA) && !dev->dma_device;
> }
>
> Which is a a little more guidance that driver authors need to set this
> config symbol.
Sure, I'll do that.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/6] RDMA/core: remove use of dma_virt_ops
On Thu, Nov 05, 2020 at 08:42:02AM +0100, Christoph Hellwig wrote:
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 5f8fd7976034e0..661e4a22b3cb81 100644
> +++ b/include/rdma/ib_verbs.h
> @@ -3950,6 +3950,8 @@ static inline int ib_req_ncomp_notif(struct ib_cq *cq,
> int wc_cnt)
> */
> static inline int ib_dma_mapping_error(struct ib_device *dev, u64 dma_addr)
> {
> + if (!dev->dma_device)
> + return 0;
How about:
static inline bool ib_uses_virt_dma(struct ib_device *dev)
{
return IS_ENABLED(CONFIG_INFINIBAND_VIRT_DMA) && !dev->dma_device;
}
Which is a a little more guidance that driver authors need to set this
config symbol.
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
