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/
> > + 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 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..
Jason
--
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
