Mostly questions.
> @@ -1628,7 +1148,7 @@ ssize_t ib_uverbs_resize_cq(struct
> ib_uverbs_file *file,
> (unsigned long) cmd.response + sizeof resp,
> in_len - sizeof cmd, out_len - sizeof resp);
>
> - cq = idr_read_cq(cmd.cq_handle, file->ucontext, 0);
> + cq = uobj_get_obj_read(cq, cmd.cq_handle, file->ucontext);
(I noticed rereg_mr used a write lock.)
Is a read lock sufficient here?
> if (!cq)
> return -EINVAL;
>
> @@ -2399,7 +1896,7 @@ static int modify_qp(struct ib_uverbs_file
> *file,
> if (!attr)
> return -ENOMEM;
>
> - qp = idr_read_qp(cmd->base.qp_handle, file->ucontext);
> + qp = uobj_get_obj_read(qp, cmd->base.qp_handle, file->ucontext);
And here? (another below)
> if (!qp) {
> ret = -EINVAL;
> goto out;
> @@ -2471,7 +1968,7 @@ static int modify_qp(struct ib_uverbs_file
> *file,
> }
>
> release_qp:
> - put_qp_read(qp);
> + uobj_put_obj_read(qp);
>
> out:
> kfree(attr);
> @@ -2558,42 +2055,27 @@ ssize_t ib_uverbs_destroy_qp(struct
> ib_uverbs_file *file,
>
> memset(&resp, 0, sizeof resp);
>
> - uobj = idr_write_uobj(cmd.qp_handle, file->ucontext);
> - if (!uobj)
> - return -EINVAL;
> + uobj = uobj_get_write(uobj_get_type(qp), cmd.qp_handle,
> + file->ucontext);
> + if (IS_ERR(uobj))
> + return PTR_ERR(uobj);
> +
> qp = uobj->object;
> obj = container_of(uobj, struct ib_uqp_object, uevent.uobject);
> + /*
> + * Make sure we don't free the memory in remove_commit as we
> still
> + * needs the uobject memory to create the response.
> + */
> + uverbs_uobject_get(uobj);
As an alternative, you could pass a parameter into the destroy calls, which could carry the information needed to write out the results. (There are 3-4 other places with a similar structure.)
>
> - if (!list_empty(&obj->mcast_list)) {
> - put_uobj_write(uobj);
> - return -EBUSY;
> - }
> -
> - ret = ib_destroy_qp(qp);
> - if (!ret)
> - uobj->live = 0;
> -
> - put_uobj_write(uobj);
> -
> - if (ret)
> + ret = uobj_remove_commit(uobj);
> + if (ret) {
> + uverbs_uobject_put(uobj);
> return ret;
> -
> - ib_rdmacg_uncharge(&uobj->cg_obj, ib_dev,
> RDMACG_RESOURCE_HCA_OBJECT);
> -
> - if (obj->uxrcd)
> - atomic_dec(&obj->uxrcd->refcnt);
> -
> - idr_remove_uobj(uobj);
> -
> - mutex_lock(&file->mutex);
> - list_del(&uobj->list);
> - mutex_unlock(&file->mutex);
> -
> - ib_uverbs_release_uevent(file, &obj->uevent);
> + }
>
> resp.events_reported = obj->uevent.events_reported;
> -
> - put_uobj(uobj);
> + uverbs_uobject_put(uobj);
>
> if (copy_to_user((void __user *) (unsigned long) cmd.response,
> &resp, sizeof resp))
> @@ -3142,7 +2583,7 @@ ssize_t ib_uverbs_attach_mcast(struct
> ib_uverbs_file *file,
> if (copy_from_user(&cmd, buf, sizeof cmd))
> return -EFAULT;
>
> - qp = idr_write_qp(cmd.qp_handle, file->ucontext);
> + qp = uobj_get_obj_read(qp, cmd.qp_handle, file->ucontext);
This converts an idr_write to get_obj_read...
> if (!qp)
> return -EINVAL;
>
> @@ -3171,7 +2612,7 @@ ssize_t ib_uverbs_attach_mcast(struct
> ib_uverbs_file *file,
> kfree(mcast);
>
> out_put:
> - put_qp_write(qp);
> + uobj_put_obj_read(qp);
>
> return ret ? ret : in_len;
> }
> @@ -3190,16 +2631,16 @@ ssize_t ib_uverbs_detach_mcast(struct
> ib_uverbs_file *file,
> if (copy_from_user(&cmd, buf, sizeof cmd))
> return -EFAULT;
>
> - qp = idr_write_qp(cmd.qp_handle, file->ucontext);
> + qp = uobj_get_obj_read(qp, cmd.qp_handle, file->ucontext);
Same here. Are these changes correct?
> if (!qp)
> return -EINVAL;
>
> + obj = container_of(qp->uobject, struct ib_uqp_object,
> uevent.uobject);
> +
> ret = ib_detach_mcast(qp, (union ib_gid *) cmd.gid, cmd.mlid);
> if (ret)
> goto out_put;
>
> - obj = container_of(qp->uobject, struct ib_uqp_object,
> uevent.uobject);
> -
> list_for_each_entry(mcast, &obj->mcast_list, list)
> if (cmd.mlid == mcast->lid &&
> !memcmp(cmd.gid, mcast->gid.raw, sizeof mcast-
> >gid.raw)) {
> @@ -3209,8 +2650,7 @@ ssize_t ib_uverbs_detach_mcast(struct
> ib_uverbs_file *file,
> }
>
> out_put:
> - put_qp_write(qp);
> -
> + uobj_put_obj_read(qp);
> return ret ? ret : in_len;
> }
> @@ -3526,31 +2953,27 @@ int ib_uverbs_ex_destroy_wq(struct
> ib_uverbs_file *file,
> return -EOPNOTSUPP;
>
> resp.response_length = required_resp_len;
> - uobj = idr_write_uobj(cmd.wq_handle,
> - file->ucontext);
> - if (!uobj)
> - return -EINVAL;
> + uobj = uobj_get_write(uobj_get_type(wq), cmd.wq_handle,
> + file->ucontext);
> + if (IS_ERR(uobj))
> + return PTR_ERR(uobj);
>
> wq = uobj->object;
> obj = container_of(uobj, struct ib_uwq_object, uevent.uobject);
> - ret = ib_destroy_wq(wq);
> - if (!ret)
> - uobj->live = 0;
> + /*
> + * Make sure we don't free the memory in remove_commit as we
> still
> + * needs the uobject memory to create the response.
> + */
> + uverbs_uobject_get(uobj);
>
> - put_uobj_write(uobj);
> - if (ret)
> + ret = uobj_remove_commit(uobj);
> + if (ret) {
> + uverbs_uobject_put(uobj);
> return ret;
> + }
>
> - idr_remove_uobj(uobj);
> -
> - mutex_lock(&file->mutex);
> - list_del(&uobj->list);
> - mutex_unlock(&file->mutex);
> -
> - ib_uverbs_release_uevent(file, &obj->uevent);
> resp.events_reported = obj->uevent.events_reported;
> - put_uobj(uobj);
> -
> + uverbs_uobject_put(uobj);
Nit: This call can move above the if (ret) check above, with the duplicate call removed.
> ret = ib_copy_to_udata(ucore, &resp, resp.response_length);
> if (ret)
> return ret;
> @@ -3588,7 +3011,7 @@ int ib_uverbs_ex_modify_wq(struct ib_uverbs_file
> *file,
> if (cmd.attr_mask > (IB_WQ_STATE | IB_WQ_CUR_STATE |
> IB_WQ_FLAGS))
> return -EINVAL;
>
> - wq = idr_read_wq(cmd.wq_handle, file->ucontext);
> + wq = uobj_get_obj_read(wq, cmd.wq_handle, file->ucontext);
> if (!wq)
> return -EINVAL;
>
> @@ -4254,7 +3591,7 @@ ssize_t ib_uverbs_modify_srq(struct
> ib_uverbs_file *file,
> INIT_UDATA(&udata, buf + sizeof cmd, NULL, in_len - sizeof cmd,
> out_len);
>
> - srq = idr_read_srq(cmd.srq_handle, file->ucontext);
> + srq = uobj_get_obj_read(srq, cmd.srq_handle, file->ucontext);
Use write lock here?
> if (!srq)
> return -EINVAL;
>
> @@ -4263,7 +3600,7 @@ ssize_t ib_uverbs_modify_srq(struct
> ib_uverbs_file *file,
>
> ret = srq->device->modify_srq(srq, &attr, cmd.attr_mask,
> &udata);
>
> - put_srq_read(srq);
> + uobj_put_obj_read(srq);
>
> return ret ? ret : in_len;
> }
- Sean
--
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
