Re: [PATCH v1 1/1] rxe: calculate inline data size based on requested values

[Jason Gunthorpe]

On Wed, Oct 23, 2019 at 10:32:37AM -0700, rao Shoaib wrote:
> From: Rao Shoaib <rao.shoaib@xxxxxxxxxx>
> 
> rxe driver has a hard coded value for the size of inline data, where as
> mlx5 driver calculates number of SGE's and inline data size based on the
> values in the qp request. This patch modifies rxe driver to do the same
> so that applications can work seamlessly across drivers.

This description doesn't seem accurate at all, and this patch seems to
be doing two things:

> Signed-off-by: Rao Shoaib <rao.shoaib@xxxxxxxxxx>
> ---
>  drivers/infiniband/sw/rxe/rxe_param.h | 2 +-
>  drivers/infiniband/sw/rxe/rxe_qp.c    | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h
> index 1b596fb..657f9303 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
> @@ -81,6 +80,7 @@ enum rxe_device_param {
>  					| IB_DEVICE_MEM_MGT_EXTENSIONS,
>  	RXE_MAX_SGE			= 32,
>  	RXE_MAX_SGE_RD			= 32,
> +	RXE_MAX_INLINE_DATA		= RXE_MAX_SGE * sizeof(struct ib_sge),
>  	RXE_MAX_CQ			= 16384,
>  	RXE_MAX_LOG_CQE			= 15,
>  	RXE_MAX_MR			= 2 * 1024,

Increasing RXE_MAX_INLINE_DATA to match the WQE size limited the
MAX_SGE. IMHO this is done in a hacky way, instead we should define a
maximim WQE size and from there derive the MAX_INLINE_DATA and MAX_SGE
limitations.

> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
> index aeea994..45b5da5 100644
> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
> @@ -229,6 +229,7 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
>  {
>  	int err;
>  	int wqe_size;
> +	unsigned int inline_size;
>  
>  	err = sock_create_kern(&init_net, AF_INET, SOCK_DGRAM, 0, &qp->sk);
>  	if (err < 0)
> @@ -244,6 +245,9 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
>  			 sizeof(struct rxe_send_wqe) +
>  			 qp->sq.max_inline);
>  
> +	inline_size = wqe_size - sizeof(struct rxe_send_wqe);
> +	qp->sq.max_inline = inline_size;
> +	init->cap.max_inline_data = inline_size;

Whatever this is doing. Is this trying to expand the supported inline
data when max_sge is provided? That seems reasonable but
peculiar. Should be it's own patch.

Also don't double initialize qp->sq.max_inline in the same function,
and there is no need for the temporary 'inline_size'

Jason


>  	qp->sq.queue = rxe_queue_init(rxe,
>  				      &qp->sq.max_wr,
>  				      wqe_size);
> -- 
> 1.8.3.1
>