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
