Date:   Wed, 5 Jan 2022 20:21:30 -0400
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Li Zhijian <lizhijian@cn.fujitsu.com>
Cc:     linux-rdma@vger.kernel.org, zyjzyj2000@gmail.com,
        aharonl@nvidia.com, leon@kernel.org, linux-kernel@vger.kernel.org,
        mbloch@nvidia.com, liweihang@huawei.com, liangwenpeng@huawei.com,
        yangx.jy@cn.fujitsu.com, rpearsonhpe@gmail.com, y-goto@fujitsu.com
Subject: Re: [RFC PATCH rdma-next 01/10] RDMA: mr: Introduce is_pmem
Message-ID: <20220106002130.GP6467@ziepe.ca>
References: <20211228080717.10666-1-lizhijian@cn.fujitsu.com>
 <20211228080717.10666-2-lizhijian@cn.fujitsu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211228080717.10666-2-lizhijian@cn.fujitsu.com>
Precedence: bulk

On Tue, Dec 28, 2021 at 04:07:08PM +0800, Li Zhijian wrote:
> We can use it to indicate whether the registering mr is associated with
> a pmem/nvdimm or not.
> 
> Currently, we only assign it in rxe driver, for other device/drivers,
> they should implement it if needed.
> 
> RDMA FLUSH will support the persistence feature for a pmem/nvdimm.
> 
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>  drivers/infiniband/sw/rxe/rxe_mr.c | 47 ++++++++++++++++++++++++++++++
>  include/rdma/ib_verbs.h            |  1 +
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> index 7c4cd19a9db2..bcd5e7afa475 100644
> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> @@ -162,6 +162,50 @@ void rxe_mr_init_dma(struct rxe_pd *pd, int access, struct rxe_mr *mr)
>  	mr->type = IB_MR_TYPE_DMA;
>  }
>  
> +// XXX: the logic is similar with mm/memory-failure.c
> +static bool page_in_dev_pagemap(struct page *page)
> +{
> +	unsigned long pfn;
> +	struct page *p;
> +	struct dev_pagemap *pgmap = NULL;
> +
> +	pfn = page_to_pfn(page);
> +	if (!pfn) {
> +		pr_err("no such pfn for page %p\n", page);
> +		return false;
> +	}
> +
> +	p = pfn_to_online_page(pfn);
> +	if (!p) {
> +		if (pfn_valid(pfn)) {
> +			pgmap = get_dev_pagemap(pfn, NULL);
> +			if (pgmap)
> +				put_dev_pagemap(pgmap);
> +		}
> +	}
> +
> +	return !!pgmap;

You need to get Dan to check this out, but I'm pretty sure this should
be more like this:

if (is_zone_device_page(page) && page->pgmap->type == MEMORY_DEVICE_FS_DAX)


> +static bool iova_in_pmem(struct rxe_mr *mr, u64 iova, int length)
> +{
> +	struct page *page = NULL;
> +	char *vaddr = iova_to_vaddr(mr, iova, length);
> +
> +	if (!vaddr) {
> +		pr_err("not a valid iova %llu\n", iova);
> +		return false;
> +	}
> +
> +	page = virt_to_page(vaddr);

And obviously this isn't uniform for the entire umem, so I don't even
know what this is supposed to mean.

> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 6e9ad656ecb7..822ebb3425dc 100644
> +++ b/include/rdma/ib_verbs.h
> @@ -1807,6 +1807,7 @@ struct ib_mr {
>  	unsigned int	   page_size;
>  	enum ib_mr_type	   type;
>  	bool		   need_inval;
> +	bool		   is_pmem;

Or why it is being stored in the global struct?

Jason