On Sun, Oct 07, 2018 at 10:23:06AM +0300, Shamir Rabinovitch wrote:
> uobject pointers should not be used in ib core layer. they only
> belong to ib uverbs. as result, resource tracker should not use
> them as well.
I can't admit that I agree with this claim and the change below.
"uobject" is part of IB/core structures and has visibility and access to
all such members.
In addition, we had the following assumption for restrack:
1. "Reuse already existing information" - this is why we had res_is_user()
2. "Don't add fields without need to restrack struct" - it is created
for every object and increases memory footprint.
3, "Avoid direct access to fields to restrack" - no to "cq->res.user = false"
4. "Minimize human errors" - this is why annotating every caller to
rdma_restrack_add() is prone to errors. For example, you didn't change cma.c.
Thanks
>
> Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@xxxxxxxxxx>
> ---
> drivers/infiniband/core/core_priv.h | 1 +
> drivers/infiniband/core/cq.c | 1 +
> drivers/infiniband/core/restrack.c | 16 +---------------
> drivers/infiniband/core/uverbs_cmd.c | 3 +++
> drivers/infiniband/core/uverbs_std_types_cq.c | 1 +
> drivers/infiniband/core/verbs.c | 3 +++
> include/rdma/restrack.h | 4 ++++
> 7 files changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h
> index d7399d5b1cb6..4b98a967b821 100644
> --- a/drivers/infiniband/core/core_priv.h
> +++ b/drivers/infiniband/core/core_priv.h
> @@ -324,6 +324,7 @@ static inline struct ib_qp *_ib_create_qp(struct ib_device *dev,
> */
> if (attr->qp_type < IB_QPT_XRC_INI) {
> qp->res.type = RDMA_RESTRACK_QP;
> + qp->res.user = !!uobj;
> rdma_restrack_add(&qp->res);
> } else
> qp->res.valid = false;
> diff --git a/drivers/infiniband/core/cq.c b/drivers/infiniband/core/cq.c
> index 9271f7290005..456e8b41ce16 100644
> --- a/drivers/infiniband/core/cq.c
> +++ b/drivers/infiniband/core/cq.c
> @@ -161,6 +161,7 @@ struct ib_cq *__ib_alloc_cq(struct ib_device *dev, void *private,
> goto out_destroy_cq;
>
> cq->res.type = RDMA_RESTRACK_CQ;
> + cq->res.user = false;
> cq->res.kern_name = caller;
> rdma_restrack_add(&cq->res);
>
> diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> index bcc693fffd4c..fb26ee566b00 100644
> --- a/drivers/infiniband/core/restrack.c
> +++ b/drivers/infiniband/core/restrack.c
> @@ -138,21 +138,7 @@ static struct ib_device *res_to_dev(struct rdma_restrack_entry *res)
>
> static bool res_is_user(struct rdma_restrack_entry *res)
> {
> - switch (res->type) {
> - case RDMA_RESTRACK_PD:
> - return container_of(res, struct ib_pd, res)->uobject;
> - case RDMA_RESTRACK_CQ:
> - return container_of(res, struct ib_cq, res)->uobject;
> - case RDMA_RESTRACK_QP:
> - return container_of(res, struct ib_qp, res)->uobject;
> - case RDMA_RESTRACK_CM_ID:
> - return !res->kern_name;
> - case RDMA_RESTRACK_MR:
> - return container_of(res, struct ib_mr, res)->pd->uobject;
> - default:
> - WARN_ONCE(true, "Wrong resource tracking type %u\n", res->type);
> - return false;
> - }
> + return res->user;
> }
>
> void rdma_restrack_add(struct rdma_restrack_entry *res)
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 91d3e4029cd5..ef83813aefc2 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -379,6 +379,7 @@ ssize_t ib_uverbs_alloc_pd(struct ib_uverbs_file *file,
> memset(&resp, 0, sizeof resp);
> resp.pd_handle = uobj->id;
> pd->res.type = RDMA_RESTRACK_PD;
> + pd->res.user = true;
> rdma_restrack_add(&pd->res);
>
> if (copy_to_user(u64_to_user_ptr(cmd.response), &resp, sizeof resp)) {
> @@ -716,6 +717,7 @@ ssize_t ib_uverbs_reg_mr(struct ib_uverbs_file *file,
> mr->uobject = uobj;
> atomic_inc(&pd->usecnt);
> mr->res.type = RDMA_RESTRACK_MR;
> + mr->res.user = true;
> rdma_restrack_add(&mr->res);
>
> uobj->object = mr;
> @@ -1039,6 +1041,7 @@ static struct ib_ucq_object *create_cq(struct ib_uverbs_file *file,
> sizeof(resp.response_length);
>
> cq->res.type = RDMA_RESTRACK_CQ;
> + cq->res.user = true;
> rdma_restrack_add(&cq->res);
>
> ret = cb(file, obj, &resp, ucore, context);
> diff --git a/drivers/infiniband/core/uverbs_std_types_cq.c b/drivers/infiniband/core/uverbs_std_types_cq.c
> index 5b5f2052cd52..fbd442b276e3 100644
> --- a/drivers/infiniband/core/uverbs_std_types_cq.c
> +++ b/drivers/infiniband/core/uverbs_std_types_cq.c
> @@ -129,6 +129,7 @@ static int UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(
> obj->uobject.user_handle = user_handle;
> atomic_set(&cq->usecnt, 0);
> cq->res.type = RDMA_RESTRACK_CQ;
> + cq->res.user = true;
> rdma_restrack_add(&cq->res);
>
> ret = uverbs_copy_to(attrs, UVERBS_ATTR_CREATE_CQ_RESP_CQE, &cq->cqe,
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index ee5fc8408add..415b4297fc7e 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -264,6 +264,7 @@ struct ib_pd *__ib_alloc_pd(struct ib_device *device, unsigned int flags,
> }
>
> pd->res.type = RDMA_RESTRACK_PD;
> + pd->res.user = false;
> pd->res.kern_name = caller;
> rdma_restrack_add(&pd->res);
>
> @@ -1890,6 +1891,7 @@ struct ib_cq *__ib_create_cq(struct ib_device *device,
> cq->cq_context = cq_context;
> atomic_set(&cq->usecnt, 0);
> cq->res.type = RDMA_RESTRACK_CQ;
> + cq->res.user = false;
> cq->res.kern_name = caller;
> rdma_restrack_add(&cq->res);
> }
> @@ -1972,6 +1974,7 @@ struct ib_mr *ib_alloc_mr(struct ib_pd *pd,
> atomic_inc(&pd->usecnt);
> mr->need_inval = false;
> mr->res.type = RDMA_RESTRACK_MR;
> + mr->res.user = false;
> rdma_restrack_add(&mr->res);
> }
>
> diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
> index 9654d33edd98..18e0fa032b45 100644
> --- a/include/rdma/restrack.h
> +++ b/include/rdma/restrack.h
> @@ -112,6 +112,10 @@ struct rdma_restrack_entry {
> * @type: various objects in restrack database
> */
> enum rdma_restrack_type type;
> + /**
> + * @user: user resource
> + */
> + bool user;
> };
>
> /**
> --
> 2.17.1
>
Attachment:
signature.asc
Description: PGP signature
