From: Rao Shoaib <[email protected]>
I have incorportaed suggestions from Jason. There are two patches.
Patch #1 introduces max WQE size as suggested by Jason
Patch #2 allocates resources requested and makes sure that the buffer size
is same for SG entries and inline data, maximum of the two values
requested is used.
Rao Shoaib (2):
Introduce maximum WQE size to check limits
SGE buffer and max_inline data must have same size
drivers/infiniband/sw/rxe/rxe_param.h | 3 ++-
drivers/infiniband/sw/rxe/rxe_qp.c | 26 ++++++++++++++------------
2 files changed, 16 insertions(+), 13 deletions(-)
--
1.8.3.1
From: Rao Shoaib <[email protected]>
SGE buffer size and max_inline data should be same. Maximum of the
two values requested is used.
Signed-off-by: Rao Shoaib <[email protected]>
---
drivers/infiniband/sw/rxe/rxe_qp.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
index 323e43d..1062f60 100644
--- a/drivers/infiniband/sw/rxe/rxe_qp.c
+++ b/drivers/infiniband/sw/rxe/rxe_qp.c
@@ -238,18 +238,17 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
return err;
qp->sk->sk->sk_user_data = qp;
- qp->sq.max_wr = init->cap.max_send_wr;
- qp->sq.max_sge = init->cap.max_send_sge;
- qp->sq.max_inline = init->cap.max_inline_data;
-
- wqe_size = max_t(int, sizeof(struct rxe_send_wqe) +
- qp->sq.max_sge * sizeof(struct ib_sge),
- sizeof(struct rxe_send_wqe) +
- qp->sq.max_inline);
-
- qp->sq.queue = rxe_queue_init(rxe,
- &qp->sq.max_wr,
- wqe_size);
+ wqe_size = max_t(int, init->cap.max_send_sge * sizeof(struct ib_sge),
+ init->cap.max_inline_data);
+ qp->sq.max_sge = wqe_size/sizeof(struct ib_sge);
+ qp->sq.max_inline = wqe_size;
+
+ wqe_size += sizeof(struct rxe_send_wqe);
+
+ qp->sq.max_wr = init->cap.max_send_wr;
+
+ qp->sq.queue = rxe_queue_init(rxe, &qp->sq.max_wr, wqe_size);
+
if (!qp->sq.queue)
return -ENOMEM;
--
1.8.3.1
From: Rao Shoaib <[email protected]>
Introduce maximum WQE size to impose limits on max SGE's and inline data
Signed-off-by: Rao Shoaib <[email protected]>
---
drivers/infiniband/sw/rxe/rxe_param.h | 3 ++-
drivers/infiniband/sw/rxe/rxe_qp.c | 7 +++++--
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h
index 1b596fb..31fb5c7 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
@@ -79,7 +78,9 @@ enum rxe_device_param {
| IB_DEVICE_RC_RNR_NAK_GEN
| IB_DEVICE_SRQ_RESIZE
| IB_DEVICE_MEM_MGT_EXTENSIONS,
+ RXE_MAX_WQE_SIZE = 0x2d0, /* For RXE_MAX_SGE */
RXE_MAX_SGE = 32,
+ RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE,
RXE_MAX_SGE_RD = 32,
RXE_MAX_CQ = 16384,
RXE_MAX_LOG_CQE = 15,
diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
index aeea994..323e43d 100644
--- a/drivers/infiniband/sw/rxe/rxe_qp.c
+++ b/drivers/infiniband/sw/rxe/rxe_qp.c
@@ -78,9 +78,12 @@ static int rxe_qp_chk_cap(struct rxe_dev *rxe, struct ib_qp_cap *cap,
}
}
- if (cap->max_inline_data > rxe->max_inline_data) {
+ if (cap->max_inline_data >
+ rxe->max_inline_data - sizeof(struct rxe_send_wqe)) {
pr_warn("invalid max inline data = %d > %d\n",
- cap->max_inline_data, rxe->max_inline_data);
+ cap->max_inline_data,
+ rxe->max_inline_data -
+ (u32)sizeof(struct rxe_send_wqe));
goto err1;
}
--
1.8.3.1
On Mon, Nov 18, 2019 at 11:54:38AM -0800, rao Shoaib wrote:
> From: Rao Shoaib <[email protected]>
>
> Introduce maximum WQE size to impose limits on max SGE's and inline data
>
> Signed-off-by: Rao Shoaib <[email protected]>
> drivers/infiniband/sw/rxe/rxe_param.h | 3 ++-
> drivers/infiniband/sw/rxe/rxe_qp.c | 7 +++++--
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h
> index 1b596fb..31fb5c7 100644
> +++ 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
> @@ -79,7 +78,9 @@ enum rxe_device_param {
> | IB_DEVICE_RC_RNR_NAK_GEN
> | IB_DEVICE_SRQ_RESIZE
> | IB_DEVICE_MEM_MGT_EXTENSIONS,
> + RXE_MAX_WQE_SIZE = 0x2d0, /* For RXE_MAX_SGE */
This shouldn't just be a random constant, I think you are trying to
say:
RXE_MAX_WQE_SIZE = sizeof(struct rxe_send_wqe) + sizeof(struct ib_sge)*RXE_MAX_SGE
Just say that
> RXE_MAX_SGE = 32,
> + RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE,
This is mixed up now, it should be
RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE - sizeof(rxe_send_wqe)
> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
> index aeea994..323e43d 100644
> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
> @@ -78,9 +78,12 @@ static int rxe_qp_chk_cap(struct rxe_dev *rxe, struct ib_qp_cap *cap,
> }
> }
>
> - if (cap->max_inline_data > rxe->max_inline_data) {
> + if (cap->max_inline_data >
> + rxe->max_inline_data - sizeof(struct rxe_send_wqe)) {
> pr_warn("invalid max inline data = %d > %d\n",
> - cap->max_inline_data, rxe->max_inline_data);
> + cap->max_inline_data,
> + rxe->max_inline_data -
> + (u32)sizeof(struct rxe_send_wqe));
Then this isn't needed
And this code in the other patch:
+ wqe_size = max_t(int, init->cap.max_send_sge * sizeof(struct ib_sge),
+ init->cap.max_inline_data);
+ qp->sq.max_sge = wqe_size/sizeof(struct ib_sge);
+ qp->sq.max_inline = wqe_size;
Makes sense as both max_inline_data and 'init->cap.max_send_sge *
sizeof(struct ib_sge)' are exclusive of the wqe header
Jason
On 11/19/19 12:31 PM, Jason Gunthorpe wrote:
> On Mon, Nov 18, 2019 at 11:54:38AM -0800, rao Shoaib wrote:
>> From: Rao Shoaib <[email protected]>
>>
>> Introduce maximum WQE size to impose limits on max SGE's and inline data
>>
>> Signed-off-by: Rao Shoaib <[email protected]>
>> drivers/infiniband/sw/rxe/rxe_param.h | 3 ++-
>> drivers/infiniband/sw/rxe/rxe_qp.c | 7 +++++--
>> 2 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h
>> index 1b596fb..31fb5c7 100644
>> +++ 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
>> @@ -79,7 +78,9 @@ enum rxe_device_param {
>> | IB_DEVICE_RC_RNR_NAK_GEN
>> | IB_DEVICE_SRQ_RESIZE
>> | IB_DEVICE_MEM_MGT_EXTENSIONS,
>> + RXE_MAX_WQE_SIZE = 0x2d0, /* For RXE_MAX_SGE */
> This shouldn't just be a random constant, I think you are trying to
> say:
>
> RXE_MAX_WQE_SIZE = sizeof(struct rxe_send_wqe) + sizeof(struct ib_sge)*RXE_MAX_SGE
I thought you wanted this value to be independent of RXE_MAX_SGE, else
why are defining it.
>
> Just say that
>
>> RXE_MAX_SGE = 32,
>> + RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE,
> This is mixed up now, it should be
>
> RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE - sizeof(rxe_send_wqe)
I agree to what you are suggesting, it will make the current patch
better. However, In my previous patch I had
RXE_MAX_INLINE_DATA = RXE_MAX_SGE * sizeof(struct ib_sge)
IMHO that conveys the intent much better. I do not see the reason for
defining RXE_MAX_WQE_SIZE, ib_device_attr does not even have an entry
for it and hence the value is not used anywhere by rxe or by any other
relevant driver.
I will re-submit with the changes you have suggested.
Shoaib
>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
>> index aeea994..323e43d 100644
>> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
>> @@ -78,9 +78,12 @@ static int rxe_qp_chk_cap(struct rxe_dev *rxe, struct ib_qp_cap *cap,
>> }
>> }
>>
>> - if (cap->max_inline_data > rxe->max_inline_data) {
>> + if (cap->max_inline_data >
>> + rxe->max_inline_data - sizeof(struct rxe_send_wqe)) {
>> pr_warn("invalid max inline data = %d > %d\n",
>> - cap->max_inline_data, rxe->max_inline_data);
>> + cap->max_inline_data,
>> + rxe->max_inline_data -
>> + (u32)sizeof(struct rxe_send_wqe));
> Then this isn't needed
>
> And this code in the other patch:
>
> + wqe_size = max_t(int, init->cap.max_send_sge * sizeof(struct ib_sge),
> + init->cap.max_inline_data);
> + qp->sq.max_sge = wqe_size/sizeof(struct ib_sge);
> + qp->sq.max_inline = wqe_size;
>
> Makes sense as both max_inline_data and 'init->cap.max_send_sge *
> sizeof(struct ib_sge)' are exclusive of the wqe header
>
> Jason
On Tue, Nov 19, 2019 at 02:38:23PM -0800, Rao Shoaib wrote:
>
> On 11/19/19 12:31 PM, Jason Gunthorpe wrote:
> > On Mon, Nov 18, 2019 at 11:54:38AM -0800, rao Shoaib wrote:
> > > From: Rao Shoaib <[email protected]>
> > >
> > > Introduce maximum WQE size to impose limits on max SGE's and inline data
> > >
> > > Signed-off-by: Rao Shoaib <[email protected]>
> > > drivers/infiniband/sw/rxe/rxe_param.h | 3 ++-
> > > drivers/infiniband/sw/rxe/rxe_qp.c | 7 +++++--
> > > 2 files changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h
> > > index 1b596fb..31fb5c7 100644
> > > +++ 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
> > > @@ -79,7 +78,9 @@ enum rxe_device_param {
> > > | IB_DEVICE_RC_RNR_NAK_GEN
> > > | IB_DEVICE_SRQ_RESIZE
> > > | IB_DEVICE_MEM_MGT_EXTENSIONS,
> > > + RXE_MAX_WQE_SIZE = 0x2d0, /* For RXE_MAX_SGE */
> > This shouldn't just be a random constant, I think you are trying to
> > say:
> >
> > RXE_MAX_WQE_SIZE = sizeof(struct rxe_send_wqe) + sizeof(struct ib_sge)*RXE_MAX_SGE
> I thought you wanted this value to be independent of RXE_MAX_SGE, else why
> are defining it.
Then define
RXE_MAX_SGE = (RXE_MAX_WQE_SIZE - sizeof(rxe_send_wqe))/sizeof(rxe_sge)
And drive everything off RXE_MAX_WQE_SIZE, which sounds good
> > Just say that
> >
> > > RXE_MAX_SGE = 32,
> > > + RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE,
> > This is mixed up now, it should be
> >
> > RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE - sizeof(rxe_send_wqe)
>
> I agree to what you are suggesting, it will make the current patch better.
> However, In my previous patch I had
>
> RXE_MAX_INLINE_DATA = RXE_MAX_SGE * sizeof(struct ib_sge)
>
> IMHO that conveys the intent much better. I do not see the reason for
> defining RXE_MAX_WQE_SIZE, ib_device_attr does not even have an entry for it
> and hence the value is not used anywhere by rxe or by any other relevant
> driver.
Because WQE_SIZE is what you are actually concerned with here, using
MAX_SGE as a proxy for the max WQE is confusing
Jason
On 11/19/19 3:13 PM, Jason Gunthorpe wrote:
> On Tue, Nov 19, 2019 at 02:38:23PM -0800, Rao Shoaib wrote:
>> On 11/19/19 12:31 PM, Jason Gunthorpe wrote:
>>> On Mon, Nov 18, 2019 at 11:54:38AM -0800, rao Shoaib wrote:
>>>> From: Rao Shoaib <[email protected]>
>>>>
>>>> Introduce maximum WQE size to impose limits on max SGE's and inline data
>>>>
>>>> Signed-off-by: Rao Shoaib <[email protected]>
>>>> drivers/infiniband/sw/rxe/rxe_param.h | 3 ++-
>>>> drivers/infiniband/sw/rxe/rxe_qp.c | 7 +++++--
>>>> 2 files changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h
>>>> index 1b596fb..31fb5c7 100644
>>>> +++ 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
>>>> @@ -79,7 +78,9 @@ enum rxe_device_param {
>>>> | IB_DEVICE_RC_RNR_NAK_GEN
>>>> | IB_DEVICE_SRQ_RESIZE
>>>> | IB_DEVICE_MEM_MGT_EXTENSIONS,
>>>> + RXE_MAX_WQE_SIZE = 0x2d0, /* For RXE_MAX_SGE */
>>> This shouldn't just be a random constant, I think you are trying to
>>> say:
>>>
>>> RXE_MAX_WQE_SIZE = sizeof(struct rxe_send_wqe) + sizeof(struct ib_sge)*RXE_MAX_SGE
>> I thought you wanted this value to be independent of RXE_MAX_SGE, else why
>> are defining it.
> Then define
>
> RXE_MAX_SGE = (RXE_MAX_WQE_SIZE - sizeof(rxe_send_wqe))/sizeof(rxe_sge)
>
> And drive everything off RXE_MAX_WQE_SIZE, which sounds good
>
>>> Just say that
>>>
>>>> RXE_MAX_SGE = 32,
>>>> + RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE,
>>> This is mixed up now, it should be
>>>
>>> RXE_MAX_INLINE_DATA = RXE_MAX_WQE_SIZE - sizeof(rxe_send_wqe)
>> I agree to what you are suggesting, it will make the current patch better.
>> However, In my previous patch I had
>>
>> RXE_MAX_INLINE_DATA = RXE_MAX_SGE * sizeof(struct ib_sge)
>>
>> IMHO that conveys the intent much better. I do not see the reason for
>> defining RXE_MAX_WQE_SIZE, ib_device_attr does not even have an entry for it
>> and hence the value is not used anywhere by rxe or by any other relevant
>> driver.
> Because WQE_SIZE is what you are actually concerned with here, using
> MAX_SGE as a proxy for the max WQE is confusing
>
> Jason
My intent is that we calculate and use the maximum buffer size using the
maximum of, number of SGE's and inline data requested, not controlling
the size of WQE buffer. If I was trying to limit WQE size I would agree
with you. Defining MAX_WQE_SIZE based on MAX_SGE and recalculating
MAX_SGE does not make sense to me. MAX_SGE and inline_data are
independent variables and define the size of wqe size not the other wise
around. I did make inline_dependent on MAX_SGE.
Your and my preferences can be different but you are the maintainer and
what you say goes. We have had a good discussion and I am going with
your previous suggestion.
Kind Regards,
Shoaib
On Tue, Nov 19, 2019 at 03:55:35PM -0800, Rao Shoaib wrote:
> My intent is that we calculate and use the maximum buffer size using the
> maximum of, number of SGE's and inline data requested, not controlling the
> size of WQE buffer. If I was trying to limit WQE size I would agree with
> you. Defining MAX_WQE_SIZE based on MAX_SGE and recalculating MAX_SGE does
> not make sense to me. MAX_SGE and inline_data are independent variables and
> define the size of wqe size not the other wise around. I did make
> inline_dependent on MAX_SGE.
What you are trying to do is limit the size of the WQE to some maximum
and from there you can compute the upper limit on the SGE and the
inline data arrays, depending on how the WQE is being used.
If a limit must be had then the limit is the WQE size. It is also
reasonable to ask why rxe has a limit at all, or why the limit is so
small ie why can't it be 2k or something? But that is something else
Jason
On Tue, Dec 17, 2019 at 11:38:52AM -0800, Rao Shoaib wrote:
> Any update on my patch?
>
> If there is some change needed please let me know.
You need to repost it with the comments addressed
https://patchwork.kernel.org/patch/11250179/
Jason
Hi Jason,
I thought I had addressed the comments and literally did what you
suggested. Sorry if I missed something, can you please point it out.
Shoaib
On 12/19/19 10:25 AM, Jason Gunthorpe wrote:
> On Tue, Dec 17, 2019 at 11:38:52AM -0800, Rao Shoaib wrote:
>> Any update on my patch?
>>
>> If there is some change needed please let me know.
> You need to repost it with the comments addressed
>
> https://patchwork.kernel.org/patch/11250179/
>
> Jason
>
On 12/19/19 10:25 AM, Jason Gunthorpe wrote:
> On Tue, Dec 17, 2019 at 11:38:52AM -0800, Rao Shoaib wrote:
>> Any update on my patch?
>>
>> If there is some change needed please let me know.
> You need to repost it with the comments addressed
>
> https://patchwork.kernel.org/patch/11250179/
>
> Jason
>
Jason,
Following is a pointer to the patch that I posted in response to your
comments
https://www.spinics.net/lists/linux-rdma/msg86241.html
I posted this on Nov 18. Can you please take a look and let me know what
else has to be done.
Shoaib
On Mon, Jan 13, 2020 at 10:35:14AM -0800, Rao Shoaib wrote:
>
> On 12/19/19 10:25 AM, Jason Gunthorpe wrote:
> > On Tue, Dec 17, 2019 at 11:38:52AM -0800, Rao Shoaib wrote:
> > > Any update on my patch?
> > >
> > > If there is some change needed please let me know.
> > You need to repost it with the comments addressed
> >
> > https://patchwork.kernel.org/patch/11250179/
> >
> > Jason
> >
> Jason,
>
> Following is a pointer to the patch that I posted in response to your
> comments
>
> https://www.spinics.net/lists/linux-rdma/msg86241.html
>
> I posted this on Nov 18. Can you please take a look and let me know what
> else has to be done.
You mean this:
https://www.spinics.net/lists/linux-rdma/msg86333.html
?
Don't mix the inline size and the # SGEs. They both drive the maximum
WQE size and all the math should be directly connected.
Jason
On 1/13/20 10:47 AM, Jason Gunthorpe wrote:
> On Mon, Jan 13, 2020 at 10:35:14AM -0800, Rao Shoaib wrote:
>> On 12/19/19 10:25 AM, Jason Gunthorpe wrote:
>>> On Tue, Dec 17, 2019 at 11:38:52AM -0800, Rao Shoaib wrote:
>>>> Any update on my patch?
>>>>
>>>> If there is some change needed please let me know.
>>> You need to repost it with the comments addressed
>>>
>>> https://patchwork.kernel.org/patch/11250179/
>>>
>>> Jason
>>>
>> Jason,
>>
>> Following is a pointer to the patch that I posted in response to your
>> comments
>>
>> https://www.spinics.net/lists/linux-rdma/msg86241.html
>>
>> I posted this on Nov 18. Can you please take a look and let me know what
>> else has to be done.
> You mean this:
>
> https://www.spinics.net/lists/linux-rdma/msg86333.html
>
> ?
>
> Don't mix the inline size and the # SGEs. They both drive the maximum
> WQE size and all the math should be directly connected.
>
> Jason
Nope. I have just resent the patch. Sorry for the confusion.
Shoaib.