> -----Original Message-----
> From: Jason Gunthorpe
> Sent: Friday, July 13, 2018 11:57 AM
> To: Parav Pandit <parav@xxxxxxxxxxxx>
> Cc: linux-rdma@xxxxxxxxxxxxxxx; Yishai Hadas <yishaih@xxxxxxxxxxxx>; Leon
> Romanovsky <leonro@xxxxxxxxxxxx>
> Subject: Re: [PATCH v5] IB: Improve uverbs_cleanup_ucontext algorithm
>
> On Fri, Jul 13, 2018 at 10:47:07AM -0600, Parav Pandit wrote:
> > > -void uverbs_cleanup_ucontext(struct ib_ucontext *ucontext, bool
> > > device_removed)
> > > +static int __uverbs_cleanup_ucontext(struct ib_ucontext *ucontext,
> > > + enum rdma_remove_reason reason)
> > > {
> > > - enum rdma_remove_reason reason = device_removed ?
> > > - RDMA_REMOVE_DRIVER_REMOVE : RDMA_REMOVE_CLOSE;
> > > - unsigned int cur_order = 0;
> > > + struct ib_uobject *obj, *next_obj;
> > > + int ret = -EINVAL;
> > > + int err = 0;
> > >
> > > + /*
> > > + * This shouldn't run while executing other commands on this
> > > + * context. Thus, the only thing we should take care of is
> > > + * releasing a FD while traversing this list. The FD could be
> > > + * closed and released from the _release fop of this FD.
> > > + * In order to mitigate this, we add a lock.
> > > + * We take and release the lock per traversal in order to let
> > > + * other threads (which might still use the FDs) chance to run.
> > > + */
> > > + mutex_lock(&ucontext->uobjects_lock);
> > > ucontext->cleanup_reason = reason;
> > Any reason to protect writing cleanup_reason?
> > Couldn't find a reader that also holds uobjects_lock while reading.
>
> cleanup_reason must be written when cleanup_rwsem is held. I have a patch
> already on the list that narrows the scope of uobjects_lock to only cover what it
> is actually locking:
>
> https://patchwork.kernel.org/patch/10518713/
Spin lock replacement and not holding it while calling remove_commit() make sense now to me.
>
> > > + list_for_each_entry_safe(obj, next_obj, &ucontext->uobjects, list) {
> > > + /*
> > > + * if we hit this WARN_ON, that means we are
> > > + * racing with a lookup_get.
> > > + */
> > > + WARN_ON(uverbs_try_lock_object(obj, true));
> > > + err = obj->type->type_class->remove_commit(obj, reason);
> > > +
> > > + if (ib_is_destroy_retryable(err, reason, obj)) {
> > > + pr_debug("ib_uverbs: failed to remove uobject id %d err
> > > %d\n",
> > > + obj->id, err);
> > > + atomic_set(&obj->usecnt, 0);
> > > + continue;
> > > + }
> > > +
> > > + if (err)
> > > + pr_err("ib_uverbs: unable to remove uobject id %d err
> > > %d\n",
> > > + obj->id, err);
> > > +
> > > + list_del(&obj->list);
> > > + /* put the ref we took when we created the object */
> > > + uverbs_uobject_put(obj);
> > > + ret = 0;
> > > + }
> > > + mutex_unlock(&ucontext->uobjects_lock);
> > > + return ret;
> >
> > If there are no objects present, won't this function return error
> > because at beginning ret = -EINVAL?
>
> Yes, that is the definition of the function, if it doesn't do a cleanup it returns a
> failure.
>
> The algorithm never calls it if the list is empty.. and the locking is tricky at this
Got it, missed out the while loop.
> point because cleanup_rwsem also protects uobjects_list, so doing the
> list_empty out side the lock it should obviously be linked with is actually safe..
>
Right.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
