diff mbox series

[rdma-next,3/3] RDMA/rw: Support threshold for registration vs scattering to local pages

Message ID 20191006155955.31445-4-leon@kernel.org
State Not Applicable
Delegated to: David Miller
Headers show
Series Optimize SGL registration | expand

Commit Message

Leon Romanovsky Oct. 6, 2019, 3:59 p.m. UTC 
From: Yamin Friedman <yaminf@mellanox.com>

If there are more scatter entries than the recommended limit provided by
the ib device, UMR registration is used. This will provide optimal
performance when performing large RDMA READs over devices that advertise
the threshold capability.

With ConnectX-5 running NVMeoF RDMA with FIO single QP 128KB writes:
Without use of cap: 70Gb/sec
With use of cap: 84Gb/sec

Signed-off-by: Yamin Friedman <yaminf@mellanox.com>
Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/rw.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Christoph Hellwig Oct. 7, 2019, 6:58 a.m. UTC | #1 
>  /*
> - * 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.
>   */

Please keep the important bits of this comments instead of just
removing them.

>  {
> @@ -30,6 +28,8 @@ static inline bool rdma_rw_can_use_mr(struct ib_device *dev, u8 port_num)
>  		return true;
>  	if (unlikely(rdma_rw_force_mr))
>  		return true;
> +	if (dev->attrs.max_sgl_rd)
> +		return true;

Logically this should go before the rdma_rw_force_mr check.

>  	if (unlikely(rdma_rw_force_mr))
>  		return true;
> +	if (dev->attrs.max_sgl_rd && dir == DMA_FROM_DEVICE
> +	    && dma_nents > dev->attrs.max_sgl_rd)

Wrong indendation.  The && belongs on the first line.  And again, this
logically belongs before the rdma_rw_force_mr check.
Leon Romanovsky Oct. 7, 2019, 7:54 a.m. UTC | #2 
On Sun, Oct 06, 2019 at 11:58:25PM -0700, Christoph Hellwig wrote:
> >  /*
> > - * 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.
> >   */
>
> Please keep the important bits of this comments instead of just
> removing them.
>
> >  {
> > @@ -30,6 +28,8 @@ static inline bool rdma_rw_can_use_mr(struct ib_device *dev, u8 port_num)
> >  		return true;
> >  	if (unlikely(rdma_rw_force_mr))
> >  		return true;
> > +	if (dev->attrs.max_sgl_rd)
> > +		return true;
>
> Logically this should go before the rdma_rw_force_mr check.
>
> >  	if (unlikely(rdma_rw_force_mr))
> >  		return true;
> > +	if (dev->attrs.max_sgl_rd && dir == DMA_FROM_DEVICE
> > +	    && dma_nents > dev->attrs.max_sgl_rd)
>
> Wrong indendation.  The && belongs on the first line.  And again, this
> logically belongs before the rdma_rw_force_mr check.

I'll fix.

Thanks
Max Gurtovoy Oct. 7, 2019, 10:07 a.m. UTC | #3 
On 10/7/2019 10:54 AM, Leon Romanovsky wrote:
> On Sun, Oct 06, 2019 at 11:58:25PM -0700, Christoph Hellwig wrote:
>>>   /*
>>> - * 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.
>>>    */
>> Please keep the important bits of this comments instead of just
>> removing them.
>>
>>>   {
>>> @@ -30,6 +28,8 @@ static inline bool rdma_rw_can_use_mr(struct ib_device *dev, u8 port_num)
>>>   		return true;
>>>   	if (unlikely(rdma_rw_force_mr))
>>>   		return true;
>>> +	if (dev->attrs.max_sgl_rd)
>>> +		return true;
>> Logically this should go before the rdma_rw_force_mr check.
>>
>>>   	if (unlikely(rdma_rw_force_mr))
>>>   		return true;
>>> +	if (dev->attrs.max_sgl_rd && dir == DMA_FROM_DEVICE
>>> +	    && dma_nents > dev->attrs.max_sgl_rd)
>> Wrong indendation.  The && belongs on the first line.  And again, this
>> logically belongs before the rdma_rw_force_mr check.
> I'll fix.
>
> Thanks

The above comments looks reasonable.

Nice optimization Yamin.
diff mbox series

Patch

diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
index 5337393d4dfe..ecff40efcb88 100644
--- a/drivers/infiniband/core/rw.c
+++ b/drivers/infiniband/core/rw.c
@@ -20,9 +20,7 @@  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.
  */
 static inline bool rdma_rw_can_use_mr(struct ib_device *dev, u8 port_num)
 {
@@ -30,6 +28,8 @@  static inline bool rdma_rw_can_use_mr(struct ib_device *dev, u8 port_num)
 		return true;
 	if (unlikely(rdma_rw_force_mr))
 		return true;
+	if (dev->attrs.max_sgl_rd)
+		return true;
 	return false;
 }
 
@@ -37,9 +37,6 @@  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)
@@ -48,6 +45,9 @@  static inline bool rdma_rw_io_needs_mr(struct ib_device *dev, u8 port_num,
 		return true;
 	if (unlikely(rdma_rw_force_mr))
 		return true;
+	if (dev->attrs.max_sgl_rd && dir == DMA_FROM_DEVICE
+	    && dma_nents > dev->attrs.max_sgl_rd)
+		return true;
 	return false;
 }