Re: [PATCH v3 for-next 1/2] RDMA/core: Provide rdma_user_mmap_disassociate() to disassociate mmap pages

[Leon Romanovsky <leon@kernel.org>]

On Mon, Sep 02, 2024 at 09:32:10PM +0800, Junxian Huang wrote:
> 
> 
> On 2024/9/2 14:57, Leon Romanovsky wrote:
> > On Wed, Aug 28, 2024 at 02:46:04PM +0800, Junxian Huang wrote:
> >> From: Chengchang Tang <tangchengchang@huawei.com>
> >>
> >> Provide a new api rdma_user_mmap_disassociate() for drivers to
> >> disassociate mmap pages for a device.
> >>
> >> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> >> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
> >> ---
> >>  drivers/infiniband/core/uverbs.h      |  3 ++
> >>  drivers/infiniband/core/uverbs_main.c | 45 +++++++++++++++++++++++++--
> >>  include/rdma/ib_verbs.h               |  8 +++++
> >>  3 files changed, 54 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
> >> index 821d93c8f712..0999d27cb1c9 100644
> >> --- a/drivers/infiniband/core/uverbs.h
> >> +++ b/drivers/infiniband/core/uverbs.h
> >> @@ -160,6 +160,9 @@ struct ib_uverbs_file {
> >>  	struct page *disassociate_page;
> >>  
> >>  	struct xarray		idr;
> >> +
> >> +	struct mutex disassociation_lock;
> >> +	atomic_t disassociated;
> >>  };
> >>  
> >>  struct ib_uverbs_event {
> >> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> >> index bc099287de9a..589f27c09a2e 100644
> >> --- a/drivers/infiniband/core/uverbs_main.c
> >> +++ b/drivers/infiniband/core/uverbs_main.c
> >> @@ -76,6 +76,7 @@ static dev_t dynamic_uverbs_dev;
> >>  static DEFINE_IDA(uverbs_ida);
> >>  static int ib_uverbs_add_one(struct ib_device *device);
> >>  static void ib_uverbs_remove_one(struct ib_device *device, void *client_data);
> >> +static struct ib_client uverbs_client;
> >>  
> >>  static char *uverbs_devnode(const struct device *dev, umode_t *mode)
> >>  {
> >> @@ -217,6 +218,7 @@ void ib_uverbs_release_file(struct kref *ref)
> >>  
> >>  	if (file->disassociate_page)
> >>  		__free_pages(file->disassociate_page, 0);
> >> +	mutex_destroy(&file->disassociation_lock);
> >>  	mutex_destroy(&file->umap_lock);
> >>  	mutex_destroy(&file->ucontext_lock);
> >>  	kfree(file);
> >> @@ -700,6 +702,12 @@ static int ib_uverbs_mmap(struct file *filp, struct vm_area_struct *vma)
> >>  		ret = PTR_ERR(ucontext);
> >>  		goto out;
> >>  	}
> >> +
> >> +	if (atomic_read(&file->disassociated)) {
> > 
> > I don't see any of the newly introduced locks here. If it is
> > intentional, it needs to be documented.
> > 
> 
> <...>
> 
> >> +		ret = -EPERM;
> >> +		goto out;
> >> +	}
> >> +
> >>  	vma->vm_ops = &rdma_umap_ops;
> >>  	ret = ucontext->device->ops.mmap(ucontext, vma);
> >>  out:
> >> @@ -726,7 +734,7 @@ static void rdma_umap_open(struct vm_area_struct *vma)
> >>  	/*
> >>  	 * Disassociation already completed, the VMA should already be zapped.
> >>  	 */
> >> -	if (!ufile->ucontext)
> >> +	if (!ufile->ucontext || atomic_read(&ufile->disassociated))
> >>  		goto out_unlock;
> >>  
> >>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> >> @@ -822,6 +830,8 @@ void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile)
> >>  	struct rdma_umap_priv *priv, *next_priv;
> >>  
> >>  	lockdep_assert_held(&ufile->hw_destroy_rwsem);
> >> +	mutex_lock(&ufile->disassociation_lock);
> >> +	atomic_set(&ufile->disassociated, 1);
> > 
> > Why do you use atomic_t and not regular bool?
> > 
> 
> The original thought was that ib_uverbs_mmap() reads ufile->disassociated while
> uverbs_user_mmap_disassociate() writes it, and there might be a racing. We tried
> to use atomic_t to avoid racing without adding locks.

atomic_t is never a replacement for locks. It is a way to provide
coherent view of the data between CPUs and makes sure that write/read is
not interrupted.

Thanks