Re: [RFC PATCH rdma-next 05/10] RDMA/rxe: Allow registering persistent flag for pmem MR only

[Tom Talpey]

On 12/28/2021 3:07 AM, Li Zhijian wrote:
> Memory region should support 2 placement types: IB_ACCESS_FLUSH_PERSISTENT
> and IB_ACCESS_FLUSH_GLOBAL_VISIBILITY, and only pmem/nvdimm has ability to
> persist data(IB_ACCESS_FLUSH_PERSISTENT).
>
> Signed-off-by: Li Zhijian <lizhijian@xxxxxxxxxxxxxx>
> ---
>   drivers/infiniband/sw/rxe/rxe_mr.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> index bcd5e7afa475..21616d058f29 100644
> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> @@ -206,6 +206,11 @@ static bool iova_in_pmem(struct rxe_mr *mr, u64 iova, int length)
>   	return page_in_dev_pagemap(page);
>   }
>   
> +static bool ib_check_flush_access_flags(struct ib_mr *mr, u32 flags)
> +{
> +	return mr->is_pmem || !(flags & IB_ACCESS_FLUSH_PERSISTENT);
> +}

It is perfectly allowed to flush ordinary memory, persistence is
another matter entirely. Is this subroutine checking for flush,
or persistence? Its name is confusing and needs to be clarified.

> +
>   int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova,
>   		     int access, struct rxe_mr *mr)
>   {
> @@ -282,6 +287,13 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova,
>   
>   	// iova_in_pmem must be called after set is updated
>   	mr->ibmr.is_pmem = iova_in_pmem(mr, iova, length);
> +	if (!ib_check_flush_access_flags(&mr->ibmr, access)) {
> +		pr_err("Cannot set IB_ACCESS_FLUSH_PERSISTENT for non-pmem memory\n");
> +		mr->state = RXE_MR_STATE_INVALID;
> +		mr->umem = NULL;
> +		err = -EINVAL;
> +		goto err_release_umem;
> +	}

Setting is_pmem is reasonable, but again, this is confusing with respect
to the region being flushable. In general, all memory is flushable,
provided the platform supports any kind of cache flush (i.e. all of them).

Tom.