diff mbox series

[v1,1/1] rxe: calculate inline data size based on requested values

Message ID 1571851957-3524-2-git-send-email-rao.shoaib@oracle.com (mailing list archive)
State Changes Requested
Delegated to: Jason Gunthorpe
Headers show
Series rxe driver should dynamically caclculate inline data size | expand

Commit Message

Rao Shoaib Oct. 23, 2019, 5:32 p.m. UTC 
From: Rao Shoaib <rao.shoaib@oracle.com>

rxe driver has a hard coded value for the size of inline data, where as
mlx5 driver calculates number of SGE's and inline data size based on the
values in the qp request. This patch modifies rxe driver to do the same
so that applications can work seamlessly across drivers.

Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com>
---
 drivers/infiniband/sw/rxe/rxe_param.h | 2 +-
 drivers/infiniband/sw/rxe/rxe_qp.c    | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Jason Gunthorpe Oct. 29, 2019, 7:11 p.m. UTC | #1 
On Wed, Oct 23, 2019 at 10:32:37AM -0700, rao Shoaib wrote:
> From: Rao Shoaib <rao.shoaib@oracle.com>
> 
> rxe driver has a hard coded value for the size of inline data, where as
> mlx5 driver calculates number of SGE's and inline data size based on the
> values in the qp request. This patch modifies rxe driver to do the same
> so that applications can work seamlessly across drivers.

This description doesn't seem accurate at all, and this patch seems to
be doing two things:

> Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_param.h | 2 +-
>  drivers/infiniband/sw/rxe/rxe_qp.c    | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h
> index 1b596fb..657f9303 100644
> --- a/drivers/infiniband/sw/rxe/rxe_param.h
> +++ b/drivers/infiniband/sw/rxe/rxe_param.h
> @@ -68,7 +68,6 @@ enum rxe_device_param {
>  	RXE_HW_VER			= 0,
>  	RXE_MAX_QP			= 0x10000,
>  	RXE_MAX_QP_WR			= 0x4000,
> -	RXE_MAX_INLINE_DATA		= 400,
>  	RXE_DEVICE_CAP_FLAGS		= IB_DEVICE_BAD_PKEY_CNTR
>  					| IB_DEVICE_BAD_QKEY_CNTR
>  					| IB_DEVICE_AUTO_PATH_MIG
> @@ -81,6 +80,7 @@ enum rxe_device_param {
>  					| IB_DEVICE_MEM_MGT_EXTENSIONS,
>  	RXE_MAX_SGE			= 32,
>  	RXE_MAX_SGE_RD			= 32,
> +	RXE_MAX_INLINE_DATA		= RXE_MAX_SGE * sizeof(struct ib_sge),
>  	RXE_MAX_CQ			= 16384,
>  	RXE_MAX_LOG_CQE			= 15,
>  	RXE_MAX_MR			= 2 * 1024,

Increasing RXE_MAX_INLINE_DATA to match the WQE size limited the
MAX_SGE. IMHO this is done in a hacky way, instead we should define a
maximim WQE size and from there derive the MAX_INLINE_DATA and MAX_SGE
limitations.

> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
> index aeea994..45b5da5 100644
> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
> @@ -229,6 +229,7 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
>  {
>  	int err;
>  	int wqe_size;
> +	unsigned int inline_size;
>  
>  	err = sock_create_kern(&init_net, AF_INET, SOCK_DGRAM, 0, &qp->sk);
>  	if (err < 0)
> @@ -244,6 +245,9 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
>  			 sizeof(struct rxe_send_wqe) +
>  			 qp->sq.max_inline);
>  
> +	inline_size = wqe_size - sizeof(struct rxe_send_wqe);
> +	qp->sq.max_inline = inline_size;
> +	init->cap.max_inline_data = inline_size;

Whatever this is doing. Is this trying to expand the supported inline
data when max_sge is provided? That seems reasonable but
peculiar. Should be it's own patch.

Also don't double initialize qp->sq.max_inline in the same function,
and there is no need for the temporary 'inline_size'

Jason


>  	qp->sq.queue = rxe_queue_init(rxe,
>  				      &qp->sq.max_wr,
>  				      wqe_size);
> -- 
> 1.8.3.1
>
Rao Shoaib Oct. 29, 2019, 7:31 p.m. UTC | #2 
Hi Jason,

Thanks for the comments, please see inline.

On 10/29/19 12:11 PM, Jason Gunthorpe wrote:
> On Wed, Oct 23, 2019 at 10:32:37AM -0700, rao Shoaib wrote:
>> From: Rao Shoaib <rao.shoaib@oracle.com>
>>
>> rxe driver has a hard coded value for the size of inline data, where as
>> mlx5 driver calculates number of SGE's and inline data size based on the
>> values in the qp request. This patch modifies rxe driver to do the same
>> so that applications can work seamlessly across drivers.
> This description doesn't seem accurate at all, and this patch seems to
> be doing two things:
I thought the note described the change, I will try harder next time.
>
>> Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com>
>> ---
>>   drivers/infiniband/sw/rxe/rxe_param.h | 2 +-
>>   drivers/infiniband/sw/rxe/rxe_qp.c    | 4 ++++
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h
>> index 1b596fb..657f9303 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_param.h
>> +++ b/drivers/infiniband/sw/rxe/rxe_param.h
>> @@ -68,7 +68,6 @@ enum rxe_device_param {
>>   	RXE_HW_VER			= 0,
>>   	RXE_MAX_QP			= 0x10000,
>>   	RXE_MAX_QP_WR			= 0x4000,
>> -	RXE_MAX_INLINE_DATA		= 400,
>>   	RXE_DEVICE_CAP_FLAGS		= IB_DEVICE_BAD_PKEY_CNTR
>>   					| IB_DEVICE_BAD_QKEY_CNTR
>>   					| IB_DEVICE_AUTO_PATH_MIG
>> @@ -81,6 +80,7 @@ enum rxe_device_param {
>>   					| IB_DEVICE_MEM_MGT_EXTENSIONS,
>>   	RXE_MAX_SGE			= 32,
>>   	RXE_MAX_SGE_RD			= 32,
>> +	RXE_MAX_INLINE_DATA		= RXE_MAX_SGE * sizeof(struct ib_sge),
>>   	RXE_MAX_CQ			= 16384,
>>   	RXE_MAX_LOG_CQE			= 15,
>>   	RXE_MAX_MR			= 2 * 1024,
> Increasing RXE_MAX_INLINE_DATA to match the WQE size limited the
> MAX_SGE. IMHO this is done in a hacky way, instead we should define a
> maximim WQE size and from there derive the MAX_INLINE_DATA and MAX_SGE
> limitations.
There was already RXE_MAX_SGE defined so I did not define MAX_WQE. If 
that is what is preference I can submit a patch with that. What is a 
good value for MAX_WQE?
>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
>> index aeea994..45b5da5 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
>> @@ -229,6 +229,7 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
>>   {
>>   	int err;
>>   	int wqe_size;
>> +	unsigned int inline_size;
>>   
>>   	err = sock_create_kern(&init_net, AF_INET, SOCK_DGRAM, 0, &qp->sk);
>>   	if (err < 0)
>> @@ -244,6 +245,9 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
>>   			 sizeof(struct rxe_send_wqe) +
>>   			 qp->sq.max_inline);
>>   
>> +	inline_size = wqe_size - sizeof(struct rxe_send_wqe);
>> +	qp->sq.max_inline = inline_size;
>> +	init->cap.max_inline_data = inline_size;
> Whatever this is doing. Is this trying to expand the supported inline
> data when max_sge is provided? That seems reasonable but
> peculiar. Should be it's own patch.
Yes that is what it is dong same as mlx5 which takes the larger of the 
two values reqquested and bumps the other. I will submit a separate patch.
>
> Also don't double initialize qp->sq.max_inline in the same function,
> and there is no need for the temporary 'inline_size'

I used a separate variable as I would have to repeat the calculation 
twice. I do not understand your comment about double initialization, can 
you please clarify that for me.

Thanks,

Shoaib

>
> Jason
>
>
>>   	qp->sq.queue = rxe_queue_init(rxe,
>>   				      &qp->sq.max_wr,
>>   				      wqe_size);
>> -- 
>> 1.8.3.1
>>
Jason Gunthorpe Oct. 29, 2019, 7:44 p.m. UTC | #3 
On Tue, Oct 29, 2019 at 12:31:03PM -0700, Rao Shoaib wrote:

> > > @@ -81,6 +80,7 @@ enum rxe_device_param {
> > >   					| IB_DEVICE_MEM_MGT_EXTENSIONS,
> > >   	RXE_MAX_SGE			= 32,
> > >   	RXE_MAX_SGE_RD			= 32,
> > > +	RXE_MAX_INLINE_DATA		= RXE_MAX_SGE * sizeof(struct ib_sge),
> > >   	RXE_MAX_CQ			= 16384,
> > >   	RXE_MAX_LOG_CQE			= 15,
> > >   	RXE_MAX_MR			= 2 * 1024,
> > Increasing RXE_MAX_INLINE_DATA to match the WQE size limited the
> > MAX_SGE. IMHO this is done in a hacky way, instead we should define a
> > maximim WQE size and from there derive the MAX_INLINE_DATA and MAX_SGE
> > limitations.
> There was already RXE_MAX_SGE defined so I did not define MAX_WQE. If that
> is what is preference I can submit a patch with that. What is a good value
> for MAX_WQE?

I would arrange it so that RXE_MAX_SGE doesn't change

> > Also don't double initialize qp->sq.max_inline in the same function,
> > and there is no need for the temporary 'inline_size'
> 
> I used a separate variable as I would have to repeat the calculation twice.
> I do not understand your comment about double initialization, can you please
> clarify that for me.

Assign it to qp->sq.max_inline and then read it to get the init

Look above in the function, there is already an assignment to
qp->sq.max_inline

Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h
index 1b596fb..657f9303 100644
--- a/drivers/infiniband/sw/rxe/rxe_param.h
+++ b/drivers/infiniband/sw/rxe/rxe_param.h
@@ -68,7 +68,6 @@  enum rxe_device_param {
 	RXE_HW_VER			= 0,
 	RXE_MAX_QP			= 0x10000,
 	RXE_MAX_QP_WR			= 0x4000,
-	RXE_MAX_INLINE_DATA		= 400,
 	RXE_DEVICE_CAP_FLAGS		= IB_DEVICE_BAD_PKEY_CNTR
 					| IB_DEVICE_BAD_QKEY_CNTR
 					| IB_DEVICE_AUTO_PATH_MIG
@@ -81,6 +80,7 @@  enum rxe_device_param {
 					| IB_DEVICE_MEM_MGT_EXTENSIONS,
 	RXE_MAX_SGE			= 32,
 	RXE_MAX_SGE_RD			= 32,
+	RXE_MAX_INLINE_DATA		= RXE_MAX_SGE * sizeof(struct ib_sge),
 	RXE_MAX_CQ			= 16384,
 	RXE_MAX_LOG_CQE			= 15,
 	RXE_MAX_MR			= 2 * 1024,
diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
index aeea994..45b5da5 100644
--- a/drivers/infiniband/sw/rxe/rxe_qp.c
+++ b/drivers/infiniband/sw/rxe/rxe_qp.c
@@ -229,6 +229,7 @@  static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
 {
 	int err;
 	int wqe_size;
+	unsigned int inline_size;
 
 	err = sock_create_kern(&init_net, AF_INET, SOCK_DGRAM, 0, &qp->sk);
 	if (err < 0)
@@ -244,6 +245,9 @@  static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
 			 sizeof(struct rxe_send_wqe) +
 			 qp->sq.max_inline);
 
+	inline_size = wqe_size - sizeof(struct rxe_send_wqe);
+	qp->sq.max_inline = inline_size;
+	init->cap.max_inline_data = inline_size;
 	qp->sq.queue = rxe_queue_init(rxe,
 				      &qp->sq.max_wr,
 				      wqe_size);