Re: [PATCH v5] IB: Improve uverbs_cleanup_ucontext algorithm

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux