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@xxxxxxxxxx>
> >>
> >> Provide a new api rdma_user_mmap_disassociate() for drivers to
> >> disassociate mmap pages for a device.
> >>
> >> Signed-off-by: Chengchang Tang <tangchengchang@xxxxxxxxxx>
> >> Signed-off-by: Junxian Huang <huangjunxian6@xxxxxxxxxxxxx>
> >> ---
> >> 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
