lkml.org 
[lkml]   [2019]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/2] Introduce maximum WQE size to check limits
From
Date

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 <rao.shoaib@oracle.com>
>>>>
>>>> Introduce maximum WQE size to impose limits on max SGE's and inline data
>>>>
>>>> Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com>
>>>> 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

\
 
 \ /
  Last update: 2019-11-20 00:59    [W:0.094 / U:0.248 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site