Re: [PATCH for-next v5 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply()

From: Jason Gunthorpe <jgg@nvidia.com>
To: Bob Pearson <rpearsonhpe@gmail.com>
Cc: zyjzyj2000@gmail.com, leonro@nvidia.com, yangx.jy@fujitsu.com,
	linux-rdma@vger.kernel.org
Subject: Re: [PATCH for-next v5 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply()
Date: Tue, 17 Jan 2023 11:11:20 -0400	[thread overview]
Message-ID: <Y8a6mILrIxIwq4/m@nvidia.com> (raw)
In-Reply-To: <20230116235602.22899-5-rpearsonhpe@gmail.com>

On Mon, Jan 16, 2023 at 05:56:01PM -0600, Bob Pearson wrote:

> +/* only implemented for 64 bit architectures */
> +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
> +{
> +#if defined CONFIG_64BIT

#ifdef

It is a little more typical style to provide an alternate version of
the function when #ifdefing

> +	u64 *va;
> +
> +	/* See IBA oA19-28 */
> +	if (unlikely(mr->state != RXE_MR_STATE_VALID)) {
> +		rxe_dbg_mr(mr, "mr not in valid state");
> +		return -EINVAL;
> +	}
> +
> +	va = iova_to_vaddr(mr, iova, sizeof(value));
> +	if (unlikely(!va)) {
> +		rxe_dbg_mr(mr, "iova out of range");
> +		return -ERANGE;
> +	}
> +
> +	/* See IBA A19.4.2 */
> +	if (unlikely((uintptr_t)va & 0x7 || iova & 0x7)) {
> +		rxe_dbg_mr(mr, "misaligned address");
> +		return -RXE_ERR_NOT_ALIGNED;
> +	}
> +
> +	/* Do atomic write after all prior operations have completed */
> +	smp_store_release(va, value);
> +
> +	return 0;
> +#else
> +	WARN_ON(1);
> +	return -EINVAL;
> +#endif
> +}
> +
>  int advance_dma_data(struct rxe_dma_info *dma, unsigned int length)
>  {
>  	struct rxe_sge		*sge	= &dma->sge[dma->cur_sge];
> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index 1e38e5da1f4c..49298ff88d25 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -764,30 +764,40 @@ static enum resp_states atomic_reply(struct rxe_qp *qp,
>  	return RESPST_ACKNOWLEDGE;
>  }
>  
> -#ifdef CONFIG_64BIT
> -static enum resp_states do_atomic_write(struct rxe_qp *qp,
> -					struct rxe_pkt_info *pkt)
> +static enum resp_states atomic_write_reply(struct rxe_qp *qp,
> +					   struct rxe_pkt_info *pkt)
>  {
> -	struct rxe_mr *mr = qp->resp.mr;
> -	int payload = payload_size(pkt);
> -	u64 src, *dst;
> -
> -	if (mr->state != RXE_MR_STATE_VALID)
> -		return RESPST_ERR_RKEY_VIOLATION;
> +	struct resp_res *res = qp->resp.res;
> +	struct rxe_mr *mr;
> +	u64 value;
> +	u64 iova;
> +	int err;
>  
> -	memcpy(&src, payload_addr(pkt), payload);
> +	if (!res) {
> +		res = rxe_prepare_res(qp, pkt, RXE_ATOMIC_WRITE_MASK);
> +		qp->resp.res = res;
> +	}
>  
> -	dst = iova_to_vaddr(mr, qp->resp.va + qp->resp.offset, payload);
> -	/* check vaddr is 8 bytes aligned. */
> -	if (!dst || (uintptr_t)dst & 7)
> -		return RESPST_ERR_MISALIGNED_ATOMIC;
> +	if (res->replay)
> +		return RESPST_ACKNOWLEDGE;
>  
> -	/* Do atomic write after all prior operations have completed */
> -	smp_store_release(dst, src);
> +	mr = qp->resp.mr;
> +	value = *(u64 *)payload_addr(pkt);
> +	iova = qp->resp.va + qp->resp.offset;
>  
> -	/* decrease resp.resid to zero */
> -	qp->resp.resid -= sizeof(payload);
> +#if defined CONFIG_64BIT

Shouldn't need a #ifdef here

> +	err = rxe_mr_do_atomic_write(mr, iova, value);
> +	if (unlikely(err)) {
> +		if (err == -RXE_ERR_NOT_ALIGNED)
> +			return RESPST_ERR_MISALIGNED_ATOMIC;
> +		else
> +			return RESPST_ERR_RKEY_VIOLATION;

Again why not return the RESPST directly then the stub can return
RESPST_ERR_UNSUPPORTED_OPCODE?

Jason