From: Rao Shoaib <[email protected]>
Resending because of typo in the email addresses.
Currently rxe driver has a hard coded value for inline data size, where as mlx5 driver calculates the size of inline data and number of SGE's to use based on the values in the qp request. Some applications depend on this behavior. This patch changes rxe to dynamically calculate the values.
Rao Shoaib (1):
rxe: calculate inline data size based on requested values
drivers/infiniband/sw/rxe/rxe_param.h | 2 +-
drivers/infiniband/sw/rxe/rxe_qp.c | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)
--
1.8.3.1
From: Rao Shoaib <[email protected]>
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 <[email protected]>
---
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,
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);
--
1.8.3.1
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
>>
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
On Wed, Oct 23, 2019 at 10:32:37AM -0700, rao Shoaib wrote:
> From: Rao Shoaib <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>