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

[Rao Shoaib]

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