Message ID | 20191007115819.9211-3-leon@kernel.org |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
Series | Optimize SGL registration | expand |
Sorry for nitpicking again, but.. On Mon, Oct 07, 2019 at 02:58:18PM +0300, Leon Romanovsky wrote: > @@ -37,15 +39,15 @@ static inline bool rdma_rw_can_use_mr(struct ib_device *dev, u8 port_num) > * Check if the device will use memory registration for this RW operation. > * We currently always use memory registrations for iWarp RDMA READs, and > * have a debug option to force usage of MRs. > - * > - * XXX: In the future we can hopefully fine tune this based on HCA driver > - * input. The above comment needs an updated a la: * Check if the device will use memory registration for this RW operation. * For RDMA READs we must use MRs on iWarp and can optionaly use them as an * optimaztion otherwise. Additionally we have a debug option to force usage * of MRs to help testing this code path. > if (rdma_protocol_iwarp(dev, port_num) && dir == DMA_FROM_DEVICE) > return true; > + if (dev->attrs.max_sgl_rd && dir == DMA_FROM_DEVICE && > + dma_nents > dev->attrs.max_sgl_rd) > + return true; This can be simplified to: if (dir == DMA_FROM_DEVICE && (rdma_protocol_iwarp(dev, port_num) || (dev->attrs.max_sgl_rd && dma_nents > dev->attrs.max_sgl_rd))) return true;
On Mon, Oct 07, 2019 at 05:12:44AM -0700, Christoph Hellwig wrote: > Sorry for nitpicking again, but.. > > On Mon, Oct 07, 2019 at 02:58:18PM +0300, Leon Romanovsky wrote: > > @@ -37,15 +39,15 @@ static inline bool rdma_rw_can_use_mr(struct ib_device *dev, u8 port_num) > > * Check if the device will use memory registration for this RW operation. > > * We currently always use memory registrations for iWarp RDMA READs, and > > * have a debug option to force usage of MRs. > > - * > > - * XXX: In the future we can hopefully fine tune this based on HCA driver > > - * input. > > The above comment needs an updated a la: > > * Check if the device will use memory registration for this RW operation. > * For RDMA READs we must use MRs on iWarp and can optionaly use them as an > * optimaztion otherwise. Additionally we have a debug option to force usage > * of MRs to help testing this code path. > > > > if (rdma_protocol_iwarp(dev, port_num) && dir == DMA_FROM_DEVICE) > > return true; > > + if (dev->attrs.max_sgl_rd && dir == DMA_FROM_DEVICE && > > + dma_nents > dev->attrs.max_sgl_rd) > > + return true; > > This can be simplified to: > > if (dir == DMA_FROM_DEVICE && > (rdma_protocol_iwarp(dev, port_num) || > (dev->attrs.max_sgl_rd && dma_nents > dev->attrs.max_sgl_rd))) > return true; I don't think that it simplifies and wanted to make separate checks to be separated. For example, rdma_protocol_iwarp() has nothing to do with attrs.max_sgl_rd. I'll fix comment. Thanks
On Mon, Oct 07, 2019 at 03:36:56PM +0300, Leon Romanovsky wrote: > > > if (rdma_protocol_iwarp(dev, port_num) && dir == DMA_FROM_DEVICE) > > > return true; > > > + if (dev->attrs.max_sgl_rd && dir == DMA_FROM_DEVICE && > > > + dma_nents > dev->attrs.max_sgl_rd) > > > + return true; > > > > This can be simplified to: > > > > if (dir == DMA_FROM_DEVICE && > > (rdma_protocol_iwarp(dev, port_num) || > > (dev->attrs.max_sgl_rd && dma_nents > dev->attrs.max_sgl_rd))) > > return true; > > I don't think that it simplifies and wanted to make separate checks to > be separated. For example, rdma_protocol_iwarp() has nothing to do with > attrs.max_sgl_rd. The important bit is to have the DMA_FROM_DEVICE check only once, as we only do the registration for reads with either parameter. So if you want it more verbose the wya would be: if (dir == DMA_FROM_DEVICE) { if (rdma_protocol_iwarp(dev, port_num)) return true; if (dev->attrs.max_sgl_rd && dma_nents > dev->attrs.max_sgl_rd) return true; }
On Mon, Oct 07, 2019 at 05:48:31AM -0700, Christoph Hellwig wrote: > On Mon, Oct 07, 2019 at 03:36:56PM +0300, Leon Romanovsky wrote: > > > > if (rdma_protocol_iwarp(dev, port_num) && dir == DMA_FROM_DEVICE) > > > > return true; > > > > + if (dev->attrs.max_sgl_rd && dir == DMA_FROM_DEVICE && > > > > + dma_nents > dev->attrs.max_sgl_rd) > > > > + return true; > > > > > > This can be simplified to: > > > > > > if (dir == DMA_FROM_DEVICE && > > > (rdma_protocol_iwarp(dev, port_num) || > > > (dev->attrs.max_sgl_rd && dma_nents > dev->attrs.max_sgl_rd))) > > > return true; > > > > I don't think that it simplifies and wanted to make separate checks to > > be separated. For example, rdma_protocol_iwarp() has nothing to do with > > attrs.max_sgl_rd. > > The important bit is to have the DMA_FROM_DEVICE check only once, as > we only do the registration for reads with either parameter. So if > you want it more verbose the wya would be: > > if (dir == DMA_FROM_DEVICE) { > if (rdma_protocol_iwarp(dev, port_num)) > return true; > if (dev->attrs.max_sgl_rd && dma_nents > dev->attrs.max_sgl_rd) > return true; > } I'm doing it now, Thank you for taking time to explain.
diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c index 5337393d4dfe..8739bd28232b 100644 --- a/drivers/infiniband/core/rw.c +++ b/drivers/infiniband/core/rw.c @@ -20,14 +20,16 @@ module_param_named(force_mr, rdma_rw_force_mr, bool, 0); MODULE_PARM_DESC(force_mr, "Force usage of MRs for RDMA READ/WRITE operations"); /* - * Check if the device might use memory registration. This is currently only - * true for iWarp devices. In the future we can hopefully fine tune this based - * on HCA driver input. + * Check if the device might use memory registration. This is currently + * true for iWarp devices and devices that have optimized SGL registration + * logic. */ static inline bool rdma_rw_can_use_mr(struct ib_device *dev, u8 port_num) { if (rdma_protocol_iwarp(dev, port_num)) return true; + if (dev->attrs.max_sgl_rd) + return true; if (unlikely(rdma_rw_force_mr)) return true; return false; @@ -37,15 +39,15 @@ static inline bool rdma_rw_can_use_mr(struct ib_device *dev, u8 port_num) * Check if the device will use memory registration for this RW operation. * We currently always use memory registrations for iWarp RDMA READs, and * have a debug option to force usage of MRs. - * - * XXX: In the future we can hopefully fine tune this based on HCA driver - * input. */ static inline bool rdma_rw_io_needs_mr(struct ib_device *dev, u8 port_num, enum dma_data_direction dir, int dma_nents) { if (rdma_protocol_iwarp(dev, port_num) && dir == DMA_FROM_DEVICE) return true; + if (dev->attrs.max_sgl_rd && dir == DMA_FROM_DEVICE && + dma_nents > dev->attrs.max_sgl_rd) + return true; if (unlikely(rdma_rw_force_mr)) return true; return false; diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 4f671378dbfc..60fd98a9b7e8 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -445,6 +445,8 @@ struct ib_device_attr { struct ib_tm_caps tm_caps; struct ib_cq_caps cq_caps; u64 max_dm_size; + /* Max entries for sgl for optimized performance per READ */ + u32 max_sgl_rd; }; enum ib_mtu {