Re: [PATCH rdma-core 8/8] verbs: Use kabi for create_qp

[Yishai Hadas <yishaih@xxxxxxxxxxxxxxxxxx>]

On 3/14/2018 12:07 AM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
>
> Instead of creating a dummy structure in kern-abi.h, just use the
> normal command structure provided by the kernel with some simple
> container_of help.

We didn't come up to finalize the review on this patch, however it was 
already merged..

Will take it to run in our regression system in parallel to review it.
One note already below.

> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> ---
>   libibverbs/cmd.c       | 13 ++++++------
>   libibverbs/kern-abi.h  | 54 ++++++++++++++++++--------------------------------
>   providers/mlx4/verbs.c |  5 +----
>   providers/mlx5/verbs.c |  5 +----
>   4 files changed, 28 insertions(+), 49 deletions(-)
>
> diff --git a/libibverbs/cmd.c b/libibverbs/cmd.c
> index e30603dd5d4194..36d62eb09ad107 100644
> --- a/libibverbs/cmd.c
> +++ b/libibverbs/cmd.c
> @@ -781,7 +781,7 @@ int ibv_cmd_destroy_srq(struct ibv_srq *srq)
>   static int create_qp_ex_common(struct verbs_qp *qp,
>   			       struct ibv_qp_init_attr_ex *qp_attr,
>   			       struct verbs_xrcd *vxrcd,
> -			       struct ibv_create_qp_common *cmd)
> +			       struct ib_uverbs_create_qp *cmd)
>   {
>   	cmd->user_handle = (uintptr_t)qp;
>   
> @@ -896,7 +896,8 @@ int ibv_cmd_create_qp_ex2(struct ibv_context *context,
>   	IBV_INIT_CMD_RESP_EX_V(cmd, cmd_core_size, cmd_size, CREATE_QP, resp,
>   			       resp_core_size, resp_size);
>   
> -	err = create_qp_ex_common(qp, qp_attr, vxrcd, &cmd->base);
> +	err = create_qp_ex_common(qp, qp_attr, vxrcd,
> +				  ibv_create_cq_ex_to_reg(cmd));

Why ibv_create_cq_ex_to_reg ? we are in create_qp_ex. (same note for its 
usage around).

>   	if (err)
>   		return err;
>   
> @@ -913,10 +914,10 @@ int ibv_cmd_create_qp_ex2(struct ibv_context *context,
>   	}
>   
>   	if (qp_attr->comp_mask & IBV_QP_INIT_ATTR_IND_TABLE) {
> -		if (cmd_core_size < offsetof(struct ibv_create_qp_ex, ind_tbl_handle) +
> -				    sizeof(cmd->ind_tbl_handle))
> +		if (cmd_core_size < offsetof(struct ibv_create_qp_ex, rwq_ind_tbl_handle) +
> +				    sizeof(cmd->rwq_ind_tbl_handle))
>   			return EINVAL;
> -		cmd->ind_tbl_handle = qp_attr->rwq_ind_tbl->ind_tbl_handle;
> +		cmd->rwq_ind_tbl_handle = qp_attr->rwq_ind_tbl->ind_tbl_handle;
>   		cmd->comp_mask = IB_UVERBS_CREATE_QP_MASK_IND_TABLE;
>   	}
>   
> @@ -947,7 +948,7 @@ int ibv_cmd_create_qp_ex(struct ibv_context *context,
>   		return ENOSYS;
>   
>   	err = create_qp_ex_common(qp, attr_ex, vxrcd,
> -				  (struct ibv_create_qp_common *)&cmd->user_handle);
> +				  &cmd->core_payload);
>   	if (err)
>   		return err;
>   
> diff --git a/libibverbs/kern-abi.h b/libibverbs/kern-abi.h
> index 26bdf19d616b5a..157f64f649af09 100644
> --- a/libibverbs/kern-abi.h
> +++ b/libibverbs/kern-abi.h
> @@ -37,6 +37,7 @@
>   
>   #include <linux/types.h>
>   #include <assert.h>
> +#include <ccan/container_of.h>
>   
>   #include <rdma/ib_user_verbs.h>
>   #include <kernel-abi/ib_user_verbs.h>
> @@ -170,6 +171,7 @@ DECLARE_CMDX(IB_USER_VERBS_CMD_CLOSE_XRCD, ibv_close_xrcd, ib_uverbs_close_xrcd,
>   DECLARE_CMD(IB_USER_VERBS_CMD_CREATE_AH, ibv_create_ah, ib_uverbs_create_ah);
>   DECLARE_CMD(IB_USER_VERBS_CMD_CREATE_COMP_CHANNEL, ibv_create_comp_channel, ib_uverbs_create_comp_channel);
>   DECLARE_CMD(IB_USER_VERBS_CMD_CREATE_CQ, ibv_create_cq, ib_uverbs_create_cq);
> +DECLARE_CMD(IB_USER_VERBS_CMD_CREATE_QP, ibv_create_qp, ib_uverbs_create_qp);
>   DECLARE_CMD(IB_USER_VERBS_CMD_CREATE_SRQ, ibv_create_srq, ib_uverbs_create_srq);
>   DECLARE_CMDX(IB_USER_VERBS_CMD_CREATE_XSRQ, ibv_create_xsrq, ib_uverbs_create_xsrq, ib_uverbs_create_srq_resp);
>   DECLARE_CMDX(IB_USER_VERBS_CMD_DEALLOC_MW, ibv_dealloc_mw, ib_uverbs_dealloc_mw, empty);
> @@ -200,6 +202,7 @@ DECLARE_CMD(IB_USER_VERBS_CMD_RESIZE_CQ, ibv_resize_cq, ib_uverbs_resize_cq);
>   
>   DECLARE_CMD_EX(IB_USER_VERBS_EX_CMD_CREATE_CQ, ibv_create_cq_ex, ib_uverbs_ex_create_cq);
>   DECLARE_CMD_EX(IB_USER_VERBS_EX_CMD_CREATE_FLOW, ibv_create_flow, ib_uverbs_create_flow);
> +DECLARE_CMD_EX(IB_USER_VERBS_EX_CMD_CREATE_QP, ibv_create_qp_ex, ib_uverbs_ex_create_qp);
>   DECLARE_CMD_EX(IB_USER_VERBS_EX_CMD_CREATE_RWQ_IND_TBL, ibv_create_rwq_ind_table, ib_uverbs_ex_create_rwq_ind_table);
>   DECLARE_CMD_EX(IB_USER_VERBS_EX_CMD_CREATE_WQ, ibv_create_wq, ib_uverbs_ex_create_wq);
>   DECLARE_CMD_EXX(IB_USER_VERBS_EX_CMD_DESTROY_FLOW, ibv_destroy_flow, ib_uverbs_destroy_flow, empty);
> @@ -210,6 +213,22 @@ DECLARE_CMD_EX(IB_USER_VERBS_EX_CMD_MODIFY_QP, ibv_modify_qp_ex, ib_uverbs_ex_mo
>   DECLARE_CMD_EXX(IB_USER_VERBS_EX_CMD_MODIFY_WQ, ibv_modify_wq, ib_uverbs_ex_modify_wq, empty);
>   DECLARE_CMD_EX(IB_USER_VERBS_EX_CMD_QUERY_DEVICE, ibv_query_device_ex, ib_uverbs_ex_query_device);
>   
> +/*
> + * Both ib_uverbs_create_qp and ib_uverbs_ex_create_qp start with the same
> + * structure, this function converts the ex version into the normal version
> + */
> +static inline struct ib_uverbs_create_qp *
> +ibv_create_cq_ex_to_reg(struct ibv_create_qp_ex *cmd_ex)
> +{
> +	/*
> +	 * user_handle is the start in both places, note that the ex
> +	 * does not have response located in the same place, so response
> +	 * cannot be touched.
> +	 */
> +	return container_of(&cmd_ex->user_handle, struct ib_uverbs_create_qp,
> +			    user_handle);
> +}
> +
>   /*
>    * This file contains copied data from the kernel's include/uapi/rdma/ib_user_verbs.h,
>    * now included above.
> @@ -218,41 +237,6 @@ DECLARE_CMD_EX(IB_USER_VERBS_EX_CMD_QUERY_DEVICE, ibv_query_device_ex, ib_uverbs
>    * copying from that header into this file.
>    */
>   
> -#define IBV_CREATE_QP_COMMON	\
> -	__u64 user_handle;	\
> -	__u32 pd_handle;	\
> -	__u32 send_cq_handle;	\
> -	__u32 recv_cq_handle;	\
> -	__u32 srq_handle;	\
> -	__u32 max_send_wr;	\
> -	__u32 max_recv_wr;	\
> -	__u32 max_send_sge;	\
> -	__u32 max_recv_sge;	\
> -	__u32 max_inline_data;	\
> -	__u8  sq_sig_all;	\
> -	__u8  qp_type;		\
> -	__u8  is_srq;		\
> -	__u8  reserved
> -
> -struct ibv_create_qp {
> -	struct ib_uverbs_cmd_hdr hdr;
> -	__u64 response;
> -	IBV_CREATE_QP_COMMON;
> -};
> -
> -struct ibv_create_qp_common {
> -	IBV_CREATE_QP_COMMON;
> -};
> -
> -struct ibv_create_qp_ex {
> -	struct ex_hdr	hdr;
> -	struct ibv_create_qp_common base;
> -	__u32 comp_mask;
> -	__u32 create_flags;
> -	__u32 ind_tbl_handle;
> -	__u32 source_qpn;
> -};
> -
>   struct ibv_kern_ipv4_filter {
>   	__u32 src_ip;
>   	__u32 dst_ip;
> diff --git a/providers/mlx4/verbs.c b/providers/mlx4/verbs.c
> index 3daa025cff78c2..9d8d3403bb6394 100644
> --- a/providers/mlx4/verbs.c
> +++ b/providers/mlx4/verbs.c
> @@ -831,10 +831,7 @@ static int mlx4_cmd_create_qp_ex(struct ibv_context *context,
>   	int ret;
>   
>   	memset(&cmd_ex, 0, sizeof(cmd_ex));
> -	memcpy(&cmd_ex.ibv_cmd.base, &cmd->ibv_cmd.user_handle,
> -	       offsetof(typeof(cmd->ibv_cmd), is_srq) +
> -	       sizeof(cmd->ibv_cmd.is_srq) -
> -	       offsetof(typeof(cmd->ibv_cmd), user_handle));
> +	*ibv_create_cq_ex_to_reg(&cmd_ex.ibv_cmd) = cmd->ibv_cmd.core_payload;
>   
>   	memcpy(&cmd_ex.drv_ex, &cmd->buf_addr,
>   	       offsetof(typeof(*cmd), sq_no_prefetch) +
> diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
> index cab65478bbe597..082149e4fb35f3 100644
> --- a/providers/mlx5/verbs.c
> +++ b/providers/mlx5/verbs.c
> @@ -1466,10 +1466,7 @@ static int mlx5_cmd_create_qp_ex(struct ibv_context *context,
>   	int ret;
>   
>   	memset(&cmd_ex, 0, sizeof(cmd_ex));
> -	memcpy(&cmd_ex.ibv_cmd.base, &cmd->ibv_cmd.user_handle,
> -	       offsetof(typeof(cmd->ibv_cmd), is_srq) +
> -	       sizeof(cmd->ibv_cmd.is_srq) -
> -	       offsetof(typeof(cmd->ibv_cmd), user_handle));
> +	*ibv_create_cq_ex_to_reg(&cmd_ex.ibv_cmd) = cmd->ibv_cmd.core_payload;
>   
>   	memcpy(&cmd_ex.drv_ex, &cmd->buf_addr,
>   	       offsetof(typeof(*cmd), sq_buf_addr) +

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html