Re: [PATCH v2 1/2] Introduce maximum WQE size to check limits

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxx>

Introduce maximum WQE size to impose limits on max SGE's and inline data

Signed-off-by: Rao Shoaib <rao.shoaib@xxxxxxxxxx>
   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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux