Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Aug 05, 2019 at 04:17:08PM +0200, Bernard Metzler wrote:
> Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com>
> ---

Don't send patches with empty commit messages. Every patch must have a
comprehensive commit message from now on.

>  drivers/infiniband/sw/siw/Kconfig     |  2 +-
>  drivers/infiniband/sw/siw/siw.h       |  2 +-
>  drivers/infiniband/sw/siw/siw_qp.c    | 14 ++++++++++----
>  drivers/infiniband/sw/siw/siw_verbs.c | 16 +++++++++++-----
>  include/uapi/rdma/siw-abi.h           |  3 ++-
>  5 files changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/Kconfig b/drivers/infiniband/sw/siw/Kconfig
> index dace276aea14..b622fc62f2cd 100644
> --- a/drivers/infiniband/sw/siw/Kconfig
> +++ b/drivers/infiniband/sw/siw/Kconfig
> @@ -1,6 +1,6 @@
>  config RDMA_SIW
>  	tristate "Software RDMA over TCP/IP (iWARP) driver"
> -	depends on INET && INFINIBAND && LIBCRC32C && 64BIT
> +	depends on INET && INFINIBAND && LIBCRC32C
>  	select DMA_VIRT_OPS
>  	help
>  	This driver implements the iWARP RDMA transport over
> diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h
> index 03fd7b2f595f..77b1aabf6ff3 100644
> --- a/drivers/infiniband/sw/siw/siw.h
> +++ b/drivers/infiniband/sw/siw/siw.h
> @@ -214,7 +214,7 @@ struct siw_wqe {
>  struct siw_cq {
>  	struct ib_cq base_cq;
>  	spinlock_t lock;
> -	u64 *notify;
> +	struct siw_cq_ctrl *notify;
>  	struct siw_cqe *queue;
>  	u32 cq_put;
>  	u32 cq_get;
> diff --git a/drivers/infiniband/sw/siw/siw_qp.c b/drivers/infiniband/sw/siw/siw_qp.c
> index e27bd5b35b96..0990307c5d2c 100644
> --- a/drivers/infiniband/sw/siw/siw_qp.c
> +++ b/drivers/infiniband/sw/siw/siw_qp.c
> @@ -1013,18 +1013,24 @@ int siw_activate_tx(struct siw_qp *qp)
>   */
>  static bool siw_cq_notify_now(struct siw_cq *cq, u32 flags)
>  {
> -	u64 cq_notify;
> +	u32 cq_notify;
>  
>  	if (!cq->base_cq.comp_handler)
>  		return false;
>  
> -	cq_notify = READ_ONCE(*cq->notify);
> +	/* Read application shared notification state */
> +	cq_notify = READ_ONCE(cq->notify->flags);
>  
>  	if ((cq_notify & SIW_NOTIFY_NEXT_COMPLETION) ||
>  	    ((cq_notify & SIW_NOTIFY_SOLICITED) &&
>  	     (flags & SIW_WQE_SOLICITED))) {
> -		/* dis-arm CQ */
> -		smp_store_mb(*cq->notify, SIW_NOTIFY_NOT);
> +		/*
> +		 * CQ notification is one-shot: Since the
> +		 * current CQE causes user notification,
> +		 * the CQ gets dis-aremd and must be re-aremd
> +		 * by the user for a new notification.
> +		 */
> +		WRITE_ONCE(cq->notify->flags, SIW_NOTIFY_NOT);
>  
>  		return true;
>  	}
> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
> index 32dc79d0e898..e7f3a2379d9d 100644
> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> @@ -1049,7 +1049,7 @@ int siw_create_cq(struct ib_cq *base_cq, const struct ib_cq_init_attr *attr,
>  
>  	spin_lock_init(&cq->lock);
>  
> -	cq->notify = &((struct siw_cq_ctrl *)&cq->queue[size])->notify;
> +	cq->notify = (struct siw_cq_ctrl *)&cq->queue[size];
>  
>  	if (udata) {
>  		struct siw_uresp_create_cq uresp = {};
> @@ -1141,11 +1141,17 @@ int siw_req_notify_cq(struct ib_cq *base_cq, enum ib_cq_notify_flags flags)
>  	siw_dbg_cq(cq, "flags: 0x%02x\n", flags);
>  
>  	if ((flags & IB_CQ_SOLICITED_MASK) == IB_CQ_SOLICITED)
> -		/* CQ event for next solicited completion */
> -		smp_store_mb(*cq->notify, SIW_NOTIFY_SOLICITED);
> +		/*
> +		 * Enable CQ event for next solicited completion.
> +		 * and make it visible to all associated producers.
> +		 */
> +		smp_store_mb(cq->notify->flags, SIW_NOTIFY_SOLICITED);
>  	else
> -		/* CQ event for any signalled completion */
> -		smp_store_mb(*cq->notify, SIW_NOTIFY_ALL);
> +		/*
> +		 * Enable CQ event for any signalled completion.
> +		 * and make it visible to all associated producers.
> +		 */
> +		smp_store_mb(cq->notify->flags, SIW_NOTIFY_ALL);

this isn't what we talked about, is it?

> index 7de68f1dc707..af735f55b291 100644
> --- a/include/uapi/rdma/siw-abi.h
> +++ b/include/uapi/rdma/siw-abi.h
> @@ -180,6 +180,7 @@ struct siw_cqe {
>   * to control CQ arming.
>   */
>  struct siw_cq_ctrl {
> -	__aligned_u64 notify;
> +	__u32 flags;
> +	__u32 pad;

The commit message needs to explain why this is compatible with
existing user space, if it is even is safe..

Jason