Re: [PATCH] rdma/siw: avoid smp_store_mb() on a u64

[Peter Zijlstra]

On Fri, Jul 12, 2019 at 11:33:46AM +0000, Bernard Metzler wrote:
> Many thanks for pointing that out! Indeed, this CQ notification
> mechanism does not take 32 bit architectures into account.
> Since we have only three flags to hold here, it's probably better
> to make it a 32bit value. That would remove the issue w/o
> introducing extra smp_wmb(). I'd prefer smp_store_mb(),
> since on some architectures it shall be more efficient.
> That would also make it sufficient to use READ_ONCE. 
> 

The below fails review due to a distinct lack of comments describing the
memory ordering.

Describe which variables (at least two) are ordered how and what
guarantees that provides and how that helps.

> From c7c3e2dbc3555581be52cb5d76c15726dced0331 Mon Sep 17 00:00:00 2001
> From: Bernard Metzler <bmt@xxxxxxxxxxxxxx>
> Date: Fri, 12 Jul 2019 13:19:27 +0200
> Subject: [PATCH] Make shared CQ notification flags 32bit to respect 32bit
>  architectures
> 
> Signed-off-by: Bernard Metzler <bmt@xxxxxxxxxxxxxx>
> ---
>  drivers/infiniband/sw/siw/siw.h       | 2 +-
>  drivers/infiniband/sw/siw/siw_qp.c    | 6 +++---
>  drivers/infiniband/sw/siw/siw_verbs.c | 6 +++---
>  include/uapi/rdma/siw-abi.h           | 3 ++-
>  4 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h
> index 409e2987cd45..d59d81f4d86b 100644
> --- a/drivers/infiniband/sw/siw/siw.h
> +++ b/drivers/infiniband/sw/siw/siw.h
> @@ -216,7 +216,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 83e50fe8e48b..0fcc5002d2da 100644
> --- a/drivers/infiniband/sw/siw/siw_qp.c
> +++ b/drivers/infiniband/sw/siw/siw_qp.c
> @@ -1011,18 +1011,18 @@ 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);
> +	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);
> +		smp_store_mb(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 d4fb78780765..bc6892229af0 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 = {};
> @@ -1142,10 +1142,10 @@ int siw_req_notify_cq(struct ib_cq *base_cq, enum ib_cq_notify_flags flags)
>  
>  	if ((flags & IB_CQ_SOLICITED_MASK) == IB_CQ_SOLICITED)
>  		/* CQ event for next solicited completion */
> -		smp_store_mb(*cq->notify, SIW_NOTIFY_SOLICITED);
> +		smp_store_mb(cq->notify->flags, SIW_NOTIFY_SOLICITED);
>  	else
>  		/* CQ event for any signalled completion */
> -		smp_store_mb(*cq->notify, SIW_NOTIFY_ALL);
> +	smp_store_mb(cq->notify->flags, SIW_NOTIFY_ALL);
>  
>  	if (flags & IB_CQ_REPORT_MISSED_EVENTS)
>  		return cq->cq_put - cq->cq_get;
> diff --git a/include/uapi/rdma/siw-abi.h b/include/uapi/rdma/siw-abi.h
> index ba4d5315cb76..93298980d3a7 100644
> --- a/include/uapi/rdma/siw-abi.h
> +++ b/include/uapi/rdma/siw-abi.h
> @@ -178,6 +178,7 @@ struct siw_cqe {
>   * to control CQ arming.
>   */
>  struct siw_cq_ctrl {
> -	__aligned_u64 notify;
> +	__u32 flags;
> +	__u32 pad;
>  };
>  #endif
> -- 
> 2.17.2
> 
>