Message ID | 20181217151528.30743-1-shamir.rabinovitch@oracle.com (mailing list archive) |
---|---|
Headers | show |
Series | figure uverbs/kernel ib_pd w/o using ib_pd uobject | expand |
On Mon, Dec 17, 2018 at 05:15:15PM +0200, Shamir Rabinovitch wrote: > This change has 2 steps. First step is to change the resource tracker > so it will not use ib_x uobject pointer to figure if ib_x object was > created by uverbs/kernel. The second step is to use the resource > tracker ability to tell if ib_pd was created by uverbs/kernel and > replace every place in the code where the code test for valid ib_pd > uobject pointer just to tell if the ib_pd was created by uverbs/kernel. > > This series is the first step toward releasing the code from the dependency > in the uobject pointer in the ib_pd. This change is required before we can > move to shared ib_pd model. > > Changelog: > v2: > * Patch 1: Comments from Jason & Leon > - Fix bool assign > - Add rdma_restrack_kadd, rdma_restrack_uadd > - Remove res_is_user > - Fix rdma_is_kernel_res > * Patch 2: Comments from Jason > - Fix rdma_is_user_pd > * Patch 3: Comments from Jason to add cleanup patch > * Patch 4: Comments from Jason which include the missing patch 3 > v3: > * Patch 1: Add patch to fix mlx5_core issue where restrack was not > initialized for some ib_x objects allocated by the driver. > * Patch 5: Fixed hns build issue reported by kbuild > v4: > * Patch 3: Extend the use of udata follow comments from Jason > on patch 4 > * Patch 4: Follow the change in patch 3, this now became more > focused on the ib_x destroy where udata is not available. > > Shamir Rabinovitch (4): > RDMA/restrack: resource-tracker should not use uobject pointers > IB/hw: cleanup of incorrect pd->uobject usage I've applied these two to for-next, with the below modifications. The last patch became so short, I'd like to get further removing the large scale uobject uses before revisiting it. I made a quick patch showing some of the next steps, I will send it to you separately.. Thanks, Jason diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c index d743c68d7c301c..46a5c553c6244a 100644 --- a/drivers/infiniband/core/restrack.c +++ b/drivers/infiniband/core/restrack.c @@ -181,6 +181,10 @@ static void rdma_restrack_add(struct rdma_restrack_entry *res) up_write(&dev->res.rwsem); } +/** + * rdma_restrack_kadd() - add kernel object to the reource tracking database + * @res: resource entry + */ void rdma_restrack_kadd(struct rdma_restrack_entry *res) { res->user = false; @@ -188,6 +192,10 @@ void rdma_restrack_kadd(struct rdma_restrack_entry *res) } EXPORT_SYMBOL(rdma_restrack_kadd); +/** + * rdma_restrack_uadd() - add user object to the reource tracking database + * @res: resource entry + */ void rdma_restrack_uadd(struct rdma_restrack_entry *res) { res->user = true; diff --git a/drivers/infiniband/hw/cxgb3/iwch_provider.c b/drivers/infiniband/hw/cxgb3/iwch_provider.c index 7a1dc83ba588a3..b34b1a1bd94b1d 100644 --- a/drivers/infiniband/hw/cxgb3/iwch_provider.c +++ b/drivers/infiniband/hw/cxgb3/iwch_provider.c @@ -836,7 +836,7 @@ static struct ib_qp *iwch_create_qp(struct ib_pd *pd, * Kernel users need more wq space for fastreg WRs which can take * 2 WR fragments. */ - ucontext = pd->uobject ? to_iwch_ucontext(pd->uobject->context) : NULL; + ucontext = udata ? to_iwch_ucontext(pd->uobject->context) : NULL; if (!ucontext && wqsize < (rqsize + (2 * sqsize))) wqsize = roundup_pow_of_two(rqsize + roundup_pow_of_two(attrs->cap.max_send_wr * 2)); diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c index 5a8030bd420850..981ff5cfb5d1e6 100644 --- a/drivers/infiniband/hw/cxgb4/qp.c +++ b/drivers/infiniband/hw/cxgb4/qp.c @@ -2163,7 +2163,7 @@ struct ib_qp *c4iw_create_qp(struct ib_pd *pd, struct ib_qp_init_attr *attrs, if (sqsize < 8) sqsize = 8; - ucontext = pd->uobject ? to_c4iw_ucontext(pd->uobject->context) : NULL; + ucontext = udata ? to_c4iw_ucontext(pd->uobject->context) : NULL; qhp = kzalloc(sizeof(*qhp), GFP_KERNEL); if (!qhp) @@ -2712,7 +2712,7 @@ struct ib_srq *c4iw_create_srq(struct ib_pd *pd, struct ib_srq_init_attr *attrs, rqsize = attrs->attr.max_wr + 1; rqsize = roundup_pow_of_two(max_t(u16, rqsize, 16)); - ucontext = pd->uobject ? to_c4iw_ucontext(pd->uobject->context) : NULL; + ucontext = udata ? to_c4iw_ucontext(pd->uobject->context) : NULL; srq = kzalloc(sizeof(*srq), GFP_KERNEL); if (!srq) diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c index 471c77b37768ae..3a669451cf868d 100644 --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c @@ -4133,7 +4133,7 @@ static int hns_roce_v2_query_qp(struct ib_qp *ibqp, struct ib_qp_attr *qp_attr, static int hns_roce_v2_destroy_qp_common(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp, - int is_user) + bool is_user) { struct hns_roce_cq *send_cq, *recv_cq; struct device *dev = hr_dev->dev; @@ -4210,7 +4210,7 @@ static int hns_roce_v2_destroy_qp(struct ib_qp *ibqp) struct hns_roce_qp *hr_qp = to_hr_qp(ibqp); int ret; - ret = hns_roce_v2_destroy_qp_common(hr_dev, hr_qp, !!ibqp->uobject); + ret = hns_roce_v2_destroy_qp_common(hr_dev, hr_qp, ibqp->uobject); if (ret) { dev_err(hr_dev->dev, "Destroy qp failed(%d)\n", ret); return ret; diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c index 54f70ae68b65a9..54031c5b53fa9d 100644 --- a/drivers/infiniband/hw/hns/hns_roce_qp.c +++ b/drivers/infiniband/hw/hns/hns_roce_qp.c @@ -280,7 +280,7 @@ void hns_roce_release_range_qp(struct hns_roce_dev *hr_dev, int base_qpn, EXPORT_SYMBOL_GPL(hns_roce_release_range_qp); static int hns_roce_set_rq_size(struct hns_roce_dev *hr_dev, - struct ib_qp_cap *cap, int is_user, int has_rq, + struct ib_qp_cap *cap, bool is_user, int has_rq, struct hns_roce_qp *hr_qp) { struct device *dev = hr_dev->dev; @@ -560,7 +560,7 @@ static int hns_roce_create_qp_common(struct hns_roce_dev *hr_dev, else hr_qp->sq_signal_bits = cpu_to_le32(IB_SIGNAL_REQ_WR); - ret = hns_roce_set_rq_size(hr_dev, &init_attr->cap, !!udata, + ret = hns_roce_set_rq_size(hr_dev, &init_attr->cap, udata, hns_roce_qp_has_rq(init_attr), hr_qp); if (ret) { dev_err(dev, "hns_roce_set_rq_size failed\n"); diff --git a/drivers/infiniband/hw/hns/hns_roce_srq.c b/drivers/infiniband/hw/hns/hns_roce_srq.c index 6377e734e28eb6..960b1946c36503 100644 --- a/drivers/infiniband/hw/hns/hns_roce_srq.c +++ b/drivers/infiniband/hw/hns/hns_roce_srq.c @@ -379,7 +379,7 @@ struct ib_srq *hns_roce_create_srq(struct ib_pd *pd, srq->event = hns_roce_ib_srq_event; srq->ibsrq.ext.xrc.srq_num = srq->srqn; - if (pd->uobject) { + if (udata) { if (ib_copy_to_udata(udata, &srq->srqn, sizeof(__u32))) { ret = -EFAULT; goto err_wrid; diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c index 0bf66bc5b9d1ab..24ee30f1cb45b2 100644 --- a/drivers/infiniband/hw/mlx4/qp.c +++ b/drivers/infiniband/hw/mlx4/qp.c @@ -323,7 +323,7 @@ static int send_wqe_overhead(enum mlx4_ib_qp_type type, u32 flags) } static int set_rq_size(struct mlx4_ib_dev *dev, struct ib_qp_cap *cap, - int is_user, int has_rq, struct mlx4_ib_qp *qp, + bool is_user, int has_rq, struct mlx4_ib_qp *qp, u32 inl_recv_sz) { /* Sanity check RQ size before proceeding */ @@ -991,7 +991,7 @@ static int create_qp_common(struct mlx4_ib_dev *dev, struct ib_pd *pd, qp->flags |= MLX4_IB_QP_SCATTER_FCS; } - err = set_rq_size(dev, &init_attr->cap, !!udata, + err = set_rq_size(dev, &init_attr->cap, udata, qp_has_rq(init_attr), qp, qp->inl_recv_sz); if (err) goto err; @@ -1043,7 +1043,7 @@ static int create_qp_common(struct mlx4_ib_dev *dev, struct ib_pd *pd, } qp->mqp.usage = MLX4_RES_USAGE_USER_VERBS; } else { - err = set_rq_size(dev, &init_attr->cap, !!udata, + err = set_rq_size(dev, &init_attr->cap, udata, qp_has_rq(init_attr), qp, 0); if (err) goto err; @@ -1332,7 +1332,7 @@ static void destroy_qp_rss(struct mlx4_ib_dev *dev, struct mlx4_ib_qp *qp) } static void destroy_qp_common(struct mlx4_ib_dev *dev, struct mlx4_ib_qp *qp, - enum mlx4_ib_source_type src, int is_user) + enum mlx4_ib_source_type src, bool is_user) { struct mlx4_ib_cq *send_cq, *recv_cq; unsigned long flags; @@ -1612,7 +1612,7 @@ static int _mlx4_ib_destroy_qp(struct ib_qp *qp) struct mlx4_ib_pd *pd; pd = get_pd(mqp); - destroy_qp_common(dev, mqp, MLX4_IB_QP_SRC, !!qp->uobject); + destroy_qp_common(dev, mqp, MLX4_IB_QP_SRC, qp->uobject); } if (is_sqp(dev, mqp)) diff --git a/drivers/infiniband/hw/mthca/mthca_srq.c b/drivers/infiniband/hw/mthca/mthca_srq.c index 7befd40c3f4551..b8333c79e3fa74 100644 --- a/drivers/infiniband/hw/mthca/mthca_srq.c +++ b/drivers/infiniband/hw/mthca/mthca_srq.c @@ -95,7 +95,8 @@ static inline int *wqe_to_link(void *wqe) static void mthca_tavor_init_srq_context(struct mthca_dev *dev, struct mthca_pd *pd, struct mthca_srq *srq, - struct mthca_tavor_srq_context *context) + struct mthca_tavor_srq_context *context, + bool is_user) { memset(context, 0, sizeof *context); @@ -103,7 +104,7 @@ static void mthca_tavor_init_srq_context(struct mthca_dev *dev, context->state_pd = cpu_to_be32(pd->pd_num); context->lkey = cpu_to_be32(srq->mr.ibmr.lkey); - if (pd->ibpd.uobject) + if (is_user) context->uar = cpu_to_be32(to_mucontext(pd->ibpd.uobject->context)->uar.index); else @@ -113,7 +114,8 @@ static void mthca_tavor_init_srq_context(struct mthca_dev *dev, static void mthca_arbel_init_srq_context(struct mthca_dev *dev, struct mthca_pd *pd, struct mthca_srq *srq, - struct mthca_arbel_srq_context *context) + struct mthca_arbel_srq_context *context, + bool is_user) { int logsize, max; @@ -129,7 +131,7 @@ static void mthca_arbel_init_srq_context(struct mthca_dev *dev, context->lkey = cpu_to_be32(srq->mr.ibmr.lkey); context->db_index = cpu_to_be32(srq->db_index); context->logstride_usrpage = cpu_to_be32((srq->wqe_shift - 4) << 29); - if (pd->ibpd.uobject) + if (is_user) context->logstride_usrpage |= cpu_to_be32(to_mucontext(pd->ibpd.uobject->context)->uar.index); else @@ -262,9 +264,9 @@ int mthca_alloc_srq(struct mthca_dev *dev, struct mthca_pd *pd, mutex_init(&srq->mutex); if (mthca_is_memfree(dev)) - mthca_arbel_init_srq_context(dev, pd, srq, mailbox->buf); + mthca_arbel_init_srq_context(dev, pd, srq, mailbox->buf, udata); else - mthca_tavor_init_srq_context(dev, pd, srq, mailbox->buf); + mthca_tavor_init_srq_context(dev, pd, srq, mailbox->buf, udata); err = mthca_SW2HW_SRQ(dev, mailbox, srq->srqn); diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h index bf5f32c38072a4..8f179be9d9a9e6 100644 --- a/include/rdma/restrack.h +++ b/include/rdma/restrack.h @@ -144,16 +144,7 @@ int rdma_restrack_count(struct rdma_restrack_root *res, enum rdma_restrack_type type, struct pid_namespace *ns); -/** - * rdma_restrack_kadd() - add kernel object to the reource tracking database - * @res: resource entry - */ void rdma_restrack_kadd(struct rdma_restrack_entry *res); - -/** - * rdma_restrack_uadd() - add user object to the reource tracking database - * @res: resource entry - */ void rdma_restrack_uadd(struct rdma_restrack_entry *res); /**
On Tue, Dec 18, 2018 at 09:58:44PM -0700, Jason Gunthorpe wrote: > On Mon, Dec 17, 2018 at 05:15:15PM +0200, Shamir Rabinovitch wrote: > > This change has 2 steps. First step is to change the resource tracker > > so it will not use ib_x uobject pointer to figure if ib_x object was > > created by uverbs/kernel. The second step is to use the resource > > tracker ability to tell if ib_pd was created by uverbs/kernel and > > replace every place in the code where the code test for valid ib_pd > > uobject pointer just to tell if the ib_pd was created by uverbs/kernel. > > > > This series is the first step toward releasing the code from the dependency > > in the uobject pointer in the ib_pd. This change is required before we can > > move to shared ib_pd model. > > > > Changelog: > > v2: > > * Patch 1: Comments from Jason & Leon > > - Fix bool assign > > - Add rdma_restrack_kadd, rdma_restrack_uadd > > - Remove res_is_user > > - Fix rdma_is_kernel_res > > * Patch 2: Comments from Jason > > - Fix rdma_is_user_pd > > * Patch 3: Comments from Jason to add cleanup patch > > * Patch 4: Comments from Jason which include the missing patch 3 > > v3: > > * Patch 1: Add patch to fix mlx5_core issue where restrack was not > > initialized for some ib_x objects allocated by the driver. > > * Patch 5: Fixed hns build issue reported by kbuild > > v4: > > * Patch 3: Extend the use of udata follow comments from Jason > > on patch 4 > > * Patch 4: Follow the change in patch 3, this now became more > > focused on the ib_x destroy where udata is not available. > > > > Shamir Rabinovitch (4): > > RDMA/restrack: resource-tracker should not use uobject pointers > > IB/hw: cleanup of incorrect pd->uobject usage > > I've applied these two to for-next, with the below modifications. > Don't we learn lately that bool in struct is not cool anymore? Thanks,
On Wed, Dec 19, 2018 at 08:46:48AM +0200, Leon Romanovsky wrote: > On Tue, Dec 18, 2018 at 09:58:44PM -0700, Jason Gunthorpe wrote: > > On Mon, Dec 17, 2018 at 05:15:15PM +0200, Shamir Rabinovitch wrote: > > > This change has 2 steps. First step is to change the resource tracker > > > so it will not use ib_x uobject pointer to figure if ib_x object was > > > created by uverbs/kernel. The second step is to use the resource > > > tracker ability to tell if ib_pd was created by uverbs/kernel and > > > replace every place in the code where the code test for valid ib_pd > > > uobject pointer just to tell if the ib_pd was created by uverbs/kernel. > > > > > > This series is the first step toward releasing the code from the dependency > > > in the uobject pointer in the ib_pd. This change is required before we can > > > move to shared ib_pd model. > > > > > > Changelog: > > > v2: > > > * Patch 1: Comments from Jason & Leon > > > - Fix bool assign > > > - Add rdma_restrack_kadd, rdma_restrack_uadd > > > - Remove res_is_user > > > - Fix rdma_is_kernel_res > > > * Patch 2: Comments from Jason > > > - Fix rdma_is_user_pd > > > * Patch 3: Comments from Jason to add cleanup patch > > > * Patch 4: Comments from Jason which include the missing patch 3 > > > v3: > > > * Patch 1: Add patch to fix mlx5_core issue where restrack was not > > > initialized for some ib_x objects allocated by the driver. > > > * Patch 5: Fixed hns build issue reported by kbuild > > > v4: > > > * Patch 3: Extend the use of udata follow comments from Jason > > > on patch 4 > > > * Patch 4: Follow the change in patch 3, this now became more > > > focused on the ib_x destroy where udata is not available. > > > > > > Shamir Rabinovitch (4): > > > RDMA/restrack: resource-tracker should not use uobject pointers > > > IB/hw: cleanup of incorrect pd->uobject usage > > > > I've applied these two to for-next, with the below modifications. > > > > Don't we learn lately that bool in struct is not cool anymore? > > Thanks, Leon sorry for the dumb questions but can you elaborate a bit about the above statement you made ? Thanks
On Wed, Dec 19, 2018 at 01:58:46PM +0200, Shamir Rabinovitch wrote: > On Wed, Dec 19, 2018 at 08:46:48AM +0200, Leon Romanovsky wrote: > > On Tue, Dec 18, 2018 at 09:58:44PM -0700, Jason Gunthorpe wrote: > > > On Mon, Dec 17, 2018 at 05:15:15PM +0200, Shamir Rabinovitch wrote: > > > > This change has 2 steps. First step is to change the resource tracker > > > > so it will not use ib_x uobject pointer to figure if ib_x object was > > > > created by uverbs/kernel. The second step is to use the resource > > > > tracker ability to tell if ib_pd was created by uverbs/kernel and > > > > replace every place in the code where the code test for valid ib_pd > > > > uobject pointer just to tell if the ib_pd was created by uverbs/kernel. > > > > > > > > This series is the first step toward releasing the code from the dependency > > > > in the uobject pointer in the ib_pd. This change is required before we can > > > > move to shared ib_pd model. > > > > > > > > Changelog: > > > > v2: > > > > * Patch 1: Comments from Jason & Leon > > > > - Fix bool assign > > > > - Add rdma_restrack_kadd, rdma_restrack_uadd > > > > - Remove res_is_user > > > > - Fix rdma_is_kernel_res > > > > * Patch 2: Comments from Jason > > > > - Fix rdma_is_user_pd > > > > * Patch 3: Comments from Jason to add cleanup patch > > > > * Patch 4: Comments from Jason which include the missing patch 3 > > > > v3: > > > > * Patch 1: Add patch to fix mlx5_core issue where restrack was not > > > > initialized for some ib_x objects allocated by the driver. > > > > * Patch 5: Fixed hns build issue reported by kbuild > > > > v4: > > > > * Patch 3: Extend the use of udata follow comments from Jason > > > > on patch 4 > > > > * Patch 4: Follow the change in patch 3, this now became more > > > > focused on the ib_x destroy where udata is not available. > > > > > > > > Shamir Rabinovitch (4): > > > > RDMA/restrack: resource-tracker should not use uobject pointers > > > > IB/hw: cleanup of incorrect pd->uobject usage > > > > > > I've applied these two to for-next, with the below modifications. > > > > > > > Don't we learn lately that bool in struct is not cool anymore? > > > > Thanks, > > Leon sorry for the dumb questions but can you elaborate a bit about the > above statement you made ? Linus said no to bool in structs :) https://lkml.org/lkml/2017/11/21/384 > > Thanks
On Wed, Dec 19, 2018 at 05:45:35PM +0200, Leon Romanovsky wrote: > On Wed, Dec 19, 2018 at 01:58:46PM +0200, Shamir Rabinovitch wrote: > > On Wed, Dec 19, 2018 at 08:46:48AM +0200, Leon Romanovsky wrote: > > > On Tue, Dec 18, 2018 at 09:58:44PM -0700, Jason Gunthorpe wrote: > > > > On Mon, Dec 17, 2018 at 05:15:15PM +0200, Shamir Rabinovitch wrote: > > > > > This change has 2 steps. First step is to change the resource tracker > > > > > so it will not use ib_x uobject pointer to figure if ib_x object was > > > > > created by uverbs/kernel. The second step is to use the resource > > > > > tracker ability to tell if ib_pd was created by uverbs/kernel and > > > > > replace every place in the code where the code test for valid ib_pd > > > > > uobject pointer just to tell if the ib_pd was created by uverbs/kernel. > > > > > > > > > > This series is the first step toward releasing the code from the dependency > > > > > in the uobject pointer in the ib_pd. This change is required before we can > > > > > move to shared ib_pd model. > > > > > > > > > > Changelog: > > > > > v2: > > > > > * Patch 1: Comments from Jason & Leon > > > > > - Fix bool assign > > > > > - Add rdma_restrack_kadd, rdma_restrack_uadd > > > > > - Remove res_is_user > > > > > - Fix rdma_is_kernel_res > > > > > * Patch 2: Comments from Jason > > > > > - Fix rdma_is_user_pd > > > > > * Patch 3: Comments from Jason to add cleanup patch > > > > > * Patch 4: Comments from Jason which include the missing patch 3 > > > > > v3: > > > > > * Patch 1: Add patch to fix mlx5_core issue where restrack was not > > > > > initialized for some ib_x objects allocated by the driver. > > > > > * Patch 5: Fixed hns build issue reported by kbuild > > > > > v4: > > > > > * Patch 3: Extend the use of udata follow comments from Jason > > > > > on patch 4 > > > > > * Patch 4: Follow the change in patch 3, this now became more > > > > > focused on the ib_x destroy where udata is not available. > > > > > > > > > > Shamir Rabinovitch (4): > > > > > RDMA/restrack: resource-tracker should not use uobject pointers > > > > > IB/hw: cleanup of incorrect pd->uobject usage > > > > > > > > I've applied these two to for-next, with the below modifications. > > > > > > > > > > Don't we learn lately that bool in struct is not cool anymore? > > > > > > Thanks, > > > > Leon sorry for the dumb questions but can you elaborate a bit about the > > above statement you made ? > > Linus said no to bool in structs :) > https://lkml.org/lkml/2017/11/21/384 I still think this is primarily about bitfields & bool. The patch which sparked the comment was about converting :1 to bool which greatly increased the struct size. In this case swapping bool for int isn't going to make things smaller or clearer.. But if we had multiple flags then it should go to an unsigned int bitfield rather than a long list of bool.. Jason