Re: [PATCH for-next v2 2/2] RDMA/rxe: Enable ODP in ATOMIC WRITE operation

[Leon Romanovsky <leon@kernel.org>]

On Tue, Mar 18, 2025 at 06:49:32PM +0900, Daisuke Matsuda wrote:
> Add rxe_odp_do_atomic_write() so that ODP specific steps are applied to
> ATOMIC WRITE requests.
> 
> Signed-off-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com>
> ---
>  drivers/infiniband/sw/rxe/rxe.c      |  1 +
>  drivers/infiniband/sw/rxe/rxe_loc.h  |  5 +++
>  drivers/infiniband/sw/rxe/rxe_mr.c   | 12 -------
>  drivers/infiniband/sw/rxe/rxe_odp.c  | 53 ++++++++++++++++++++++++++++
>  drivers/infiniband/sw/rxe/rxe_resp.c | 11 +++++-
>  include/rdma/ib_verbs.h              |  1 +
>  6 files changed, 70 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c
> index df66f8f9efa1..21ce2d876b42 100644
> --- a/drivers/infiniband/sw/rxe/rxe.c
> +++ b/drivers/infiniband/sw/rxe/rxe.c
> @@ -110,6 +110,7 @@ static void rxe_init_device_param(struct rxe_dev *rxe)
>  		rxe->attr.odp_caps.per_transport_caps.rc_odp_caps |= IB_ODP_SUPPORT_ATOMIC;
>  		rxe->attr.odp_caps.per_transport_caps.rc_odp_caps |= IB_ODP_SUPPORT_SRQ_RECV;
>  		rxe->attr.odp_caps.per_transport_caps.rc_odp_caps |= IB_ODP_SUPPORT_FLUSH;
> +		rxe->attr.odp_caps.per_transport_caps.rc_odp_caps |= IB_ODP_SUPPORT_ATOMIC_WRITE;
>  	}
>  }

<...>

> +static inline int rxe_odp_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
> +{
> +	return RESPST_ERR_UNSUPPORTED_OPCODE;
> +}
>  #endif /* CONFIG_INFINIBAND_ON_DEMAND_PAGING */

You are returning "enum resp_states", while function expects to return "int". You should return -EOPNOTSUPP.

>  
>  #endif /* RXE_LOC_H */
> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> index 93e4b5acd3ac..d40fbe10633f 100644
> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> @@ -547,16 +547,6 @@ int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
>  	struct page *page;
>  	u64 *va;
>  
> -	/* ODP is not supported right now. WIP. */
> -	if (mr->umem->is_odp)
> -		return RESPST_ERR_UNSUPPORTED_OPCODE;
> -
> -	/* See IBA oA19-28 */
> -	if (unlikely(mr->state != RXE_MR_STATE_VALID)) {
> -		rxe_dbg_mr(mr, "mr not in valid state\n");
> -		return RESPST_ERR_RKEY_VIOLATION;
> -	}
> -
>  	if (mr->ibmr.type == IB_MR_TYPE_DMA) {
>  		page_offset = iova & (PAGE_SIZE - 1);
>  		page = ib_virt_dma_to_page(iova);
> @@ -584,10 +574,8 @@ int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
>  	}
>  
>  	va = kmap_local_page(page);
> -
>  	/* Do atomic write after all prior operations have completed */
>  	smp_store_release(&va[page_offset >> 3], value);
> -
>  	kunmap_local(va);
>  
>  	return 0;
> diff --git a/drivers/infiniband/sw/rxe/rxe_odp.c b/drivers/infiniband/sw/rxe/rxe_odp.c
> index 9a9aae967486..f3443c604a7f 100644
> --- a/drivers/infiniband/sw/rxe/rxe_odp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_odp.c
> @@ -378,3 +378,56 @@ int rxe_odp_flush_pmem_iova(struct rxe_mr *mr, u64 iova,
>  
>  	return 0;
>  }
> +
> +#if defined CONFIG_64BIT
> +/* only implemented or called for 64 bit architectures */
> +int rxe_odp_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
> +{
> +	struct ib_umem_odp *umem_odp = to_ib_umem_odp(mr->umem);
> +	unsigned int page_offset;
> +	unsigned long index;
> +	struct page *page;
> +	int err;
> +	u64 *va;
> +
> +	/* See IBA oA19-28 */
> +	err = mr_check_range(mr, iova, sizeof(value));
> +	if (unlikely(err)) {
> +		rxe_dbg_mr(mr, "iova out of range\n");
> +		return RESPST_ERR_RKEY_VIOLATION;

Please don't redefine returned errors.

> +	}

<...>

> +#else
> +int rxe_odp_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
> +{
> +	return RESPST_ERR_UNSUPPORTED_OPCODE;
> +}
> +#endif

You already have empty declaration in rxe_loc.h, use it.

Thanks