* [PATCH rdma-next 1/5] RDMA/core: Create and destroy counters in the ib_core
2020-06-24 10:54 [PATCH rdma-next 0/5] ib_core allocation patches Leon Romanovsky
@ 2020-06-24 10:54 ` Leon Romanovsky
2020-06-24 10:54 ` [PATCH rdma-next 2/5] RDMA: Clean MW allocation and free flows Leon Romanovsky
` (3 subsequent siblings)
4 siblings, 0 replies; 21+ messages in thread
From: Leon Romanovsky @ 2020-06-24 10:54 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe; +Cc: Leon Romanovsky, linux-rdma
From: Leon Romanovsky <leonro@mellanox.com>
Move allocation and destruction of counters under ib_core responsibility
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/device.c | 1 +
.../core/uverbs_std_types_counters.c | 17 ++++++++--------
drivers/infiniband/hw/mlx5/main.c | 20 ++++++-------------
include/rdma/ib_verbs.h | 7 ++++---
4 files changed, 20 insertions(+), 25 deletions(-)
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 1da1b0731f25..088559d72d57 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2682,6 +2682,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
SET_DEVICE_OP(dev_ops, set_vf_link_state);
SET_OBJ_SIZE(dev_ops, ib_ah);
+ SET_OBJ_SIZE(dev_ops, ib_counters);
SET_OBJ_SIZE(dev_ops, ib_cq);
SET_OBJ_SIZE(dev_ops, ib_pd);
SET_OBJ_SIZE(dev_ops, ib_srq);
diff --git a/drivers/infiniband/core/uverbs_std_types_counters.c b/drivers/infiniband/core/uverbs_std_types_counters.c
index 9f013304e677..c7e7438752bc 100644
--- a/drivers/infiniband/core/uverbs_std_types_counters.c
+++ b/drivers/infiniband/core/uverbs_std_types_counters.c
@@ -46,7 +46,9 @@ static int uverbs_free_counters(struct ib_uobject *uobject,
if (ret)
return ret;
- return counters->device->ops.destroy_counters(counters);
+ counters->device->ops.destroy_counters(counters);
+ kfree(counters);
+ return 0;
}
static int UVERBS_HANDLER(UVERBS_METHOD_COUNTERS_CREATE)(
@@ -66,20 +68,19 @@ static int UVERBS_HANDLER(UVERBS_METHOD_COUNTERS_CREATE)(
if (!ib_dev->ops.create_counters)
return -EOPNOTSUPP;
- counters = ib_dev->ops.create_counters(ib_dev, attrs);
- if (IS_ERR(counters)) {
- ret = PTR_ERR(counters);
- goto err_create_counters;
- }
+ counters = rdma_zalloc_drv_obj(ib_dev, ib_counters);
+ if (!counters)
+ return -ENOMEM;
counters->device = ib_dev;
counters->uobject = uobj;
uobj->object = counters;
atomic_set(&counters->usecnt, 0);
- return 0;
+ ret = ib_dev->ops.create_counters(counters, attrs);
+ if (ret)
+ kfree(counters);
-err_create_counters:
return ret;
}
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 46c596a855e7..ca35394a181a 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -6458,7 +6458,7 @@ static int mlx5_ib_read_counters(struct ib_counters *counters,
return ret;
}
-static int mlx5_ib_destroy_counters(struct ib_counters *counters)
+static void mlx5_ib_destroy_counters(struct ib_counters *counters)
{
struct mlx5_ib_mcounters *mcounters = to_mcounters(counters);
@@ -6466,24 +6466,15 @@ static int mlx5_ib_destroy_counters(struct ib_counters *counters)
if (mcounters->hw_cntrs_hndl)
mlx5_fc_destroy(to_mdev(counters->device)->mdev,
mcounters->hw_cntrs_hndl);
-
- kfree(mcounters);
-
- return 0;
}
-static struct ib_counters *mlx5_ib_create_counters(struct ib_device *device,
- struct uverbs_attr_bundle *attrs)
+static int mlx5_ib_create_counters(struct ib_counters *counters,
+ struct uverbs_attr_bundle *attrs)
{
- struct mlx5_ib_mcounters *mcounters;
-
- mcounters = kzalloc(sizeof(*mcounters), GFP_KERNEL);
- if (!mcounters)
- return ERR_PTR(-ENOMEM);
+ struct mlx5_ib_mcounters *mcounters = to_mcounters(counters);
mutex_init(&mcounters->mcntrs_mutex);
-
- return &mcounters->ibcntrs;
+ return 0;
}
static void mlx5_ib_stage_init_cleanup(struct mlx5_ib_dev *dev)
@@ -6651,6 +6642,7 @@ static const struct ib_device_ops mlx5_ib_dev_ops = {
.resize_cq = mlx5_ib_resize_cq,
INIT_RDMA_OBJ_SIZE(ib_ah, mlx5_ib_ah, ibah),
+ INIT_RDMA_OBJ_SIZE(ib_counters, mlx5_ib_mcounters, ibcntrs),
INIT_RDMA_OBJ_SIZE(ib_cq, mlx5_ib_cq, ibcq),
INIT_RDMA_OBJ_SIZE(ib_pd, mlx5_ib_pd, ibpd),
INIT_RDMA_OBJ_SIZE(ib_srq, mlx5_ib_srq, ibsrq),
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 1691c10f6e8a..3aa20ce23556 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2541,9 +2541,9 @@ struct ib_device_ops {
struct ib_mr *(*reg_dm_mr)(struct ib_pd *pd, struct ib_dm *dm,
struct ib_dm_mr_attr *attr,
struct uverbs_attr_bundle *attrs);
- struct ib_counters *(*create_counters)(
- struct ib_device *device, struct uverbs_attr_bundle *attrs);
- int (*destroy_counters)(struct ib_counters *counters);
+ int (*create_counters)(struct ib_counters *counters,
+ struct uverbs_attr_bundle *attrs);
+ void (*destroy_counters)(struct ib_counters *counters);
int (*read_counters)(struct ib_counters *counters,
struct ib_counters_read_attr *counters_read_attr,
struct uverbs_attr_bundle *attrs);
@@ -2651,6 +2651,7 @@ struct ib_device_ops {
struct uverbs_attr_bundle *attrs);
DECLARE_RDMA_OBJ_SIZE(ib_ah);
+ DECLARE_RDMA_OBJ_SIZE(ib_counters);
DECLARE_RDMA_OBJ_SIZE(ib_cq);
DECLARE_RDMA_OBJ_SIZE(ib_pd);
DECLARE_RDMA_OBJ_SIZE(ib_srq);
--
2.26.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH rdma-next 2/5] RDMA: Clean MW allocation and free flows
2020-06-24 10:54 [PATCH rdma-next 0/5] ib_core allocation patches Leon Romanovsky
2020-06-24 10:54 ` [PATCH rdma-next 1/5] RDMA/core: Create and destroy counters in the ib_core Leon Romanovsky
@ 2020-06-24 10:54 ` Leon Romanovsky
2020-06-28 9:18 ` Yishai Hadas
2020-06-24 10:54 ` [PATCH rdma-next 3/5] RDMA: Move XRCD to be under ib_core responsibility Leon Romanovsky
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Leon Romanovsky @ 2020-06-24 10:54 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Lijun Ou, linux-rdma, Potnuri Bharat Teja,
Weihang Li, Wei Hu(Xavier),
Yishai Hadas
From: Leon Romanovsky <leonro@mellanox.com>
Move allocation and destruction of memory windows under ib_core
responsibility and clean drivers to ensure that no updates to MW
ib_core structures are done in driver layer.
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/device.c | 1 +
drivers/infiniband/core/uverbs.h | 2 +-
drivers/infiniband/core/uverbs_cmd.c | 26 ++++++++-----
drivers/infiniband/core/uverbs_main.c | 10 ++---
drivers/infiniband/core/uverbs_std_types.c | 3 +-
drivers/infiniband/hw/cxgb4/iw_cxgb4.h | 5 +--
drivers/infiniband/hw/cxgb4/mem.c | 35 ++++++-----------
drivers/infiniband/hw/cxgb4/provider.c | 4 +-
drivers/infiniband/hw/hns/hns_roce_device.h | 5 +--
drivers/infiniband/hw/hns/hns_roce_main.c | 2 +
drivers/infiniband/hw/hns/hns_roce_mr.c | 31 +++++----------
drivers/infiniband/hw/mlx4/main.c | 2 +
drivers/infiniband/hw/mlx4/mlx4_ib.h | 5 +--
drivers/infiniband/hw/mlx4/mr.c | 32 +++++-----------
drivers/infiniband/hw/mlx5/main.c | 2 +
drivers/infiniband/hw/mlx5/mlx5_ib.h | 5 +--
drivers/infiniband/hw/mlx5/mr.c | 42 ++++++++-------------
include/rdma/ib_verbs.h | 6 +--
18 files changed, 91 insertions(+), 127 deletions(-)
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 088559d72d57..257e8a667977 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2684,6 +2684,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
SET_OBJ_SIZE(dev_ops, ib_ah);
SET_OBJ_SIZE(dev_ops, ib_counters);
SET_OBJ_SIZE(dev_ops, ib_cq);
+ SET_OBJ_SIZE(dev_ops, ib_mw);
SET_OBJ_SIZE(dev_ops, ib_pd);
SET_OBJ_SIZE(dev_ops, ib_srq);
SET_OBJ_SIZE(dev_ops, ib_ucontext);
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 53a10479958b..072bfe4e1b5b 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -242,7 +242,7 @@ int ib_uverbs_dealloc_xrcd(struct ib_uobject *uobject, struct ib_xrcd *xrcd,
enum rdma_remove_reason why,
struct uverbs_attr_bundle *attrs);
-int uverbs_dealloc_mw(struct ib_mw *mw);
+void uverbs_dealloc_mw(struct ib_mw *mw);
void ib_uverbs_detach_umcast(struct ib_qp *qp,
struct ib_uqp_object *uobj);
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 68c9a0210220..a5301f3d3059 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -890,7 +890,7 @@ static int ib_uverbs_dereg_mr(struct uverbs_attr_bundle *attrs)
static int ib_uverbs_alloc_mw(struct uverbs_attr_bundle *attrs)
{
struct ib_uverbs_alloc_mw cmd;
- struct ib_uverbs_alloc_mw_resp resp;
+ struct ib_uverbs_alloc_mw_resp resp = {};
struct ib_uobject *uobj;
struct ib_pd *pd;
struct ib_mw *mw;
@@ -916,21 +916,24 @@ static int ib_uverbs_alloc_mw(struct uverbs_attr_bundle *attrs)
goto err_put;
}
- mw = pd->device->ops.alloc_mw(pd, cmd.mw_type, &attrs->driver_udata);
- if (IS_ERR(mw)) {
- ret = PTR_ERR(mw);
+ mw = rdma_zalloc_drv_obj(ib_dev, ib_mw);
+ if (!mw) {
+ ret = -ENOMEM;
goto err_put;
}
- mw->device = pd->device;
- mw->pd = pd;
+ mw->device = ib_dev;
+ mw->pd = pd;
mw->uobject = uobj;
- atomic_inc(&pd->usecnt);
-
uobj->object = mw;
+ mw->type = cmd.mw_type;
- memset(&resp, 0, sizeof(resp));
- resp.rkey = mw->rkey;
+ ret = pd->device->ops.alloc_mw(mw, &mw->rkey, &attrs->driver_udata);
+ if (ret)
+ goto err_alloc;
+
+ atomic_inc(&pd->usecnt);
+ resp.rkey = mw->rkey;
resp.mw_handle = uobj->id;
ret = uverbs_response(attrs, &resp, sizeof(resp));
@@ -943,6 +946,9 @@ static int ib_uverbs_alloc_mw(struct uverbs_attr_bundle *attrs)
err_copy:
uverbs_dealloc_mw(mw);
+ mw = NULL;
+err_alloc:
+ kfree(mw);
err_put:
uobj_put_obj_read(pd);
err_free:
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 69e4755cc04b..706c972ea3a1 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -102,15 +102,13 @@ struct ib_ucontext *ib_uverbs_get_ucontext_file(struct ib_uverbs_file *ufile)
}
EXPORT_SYMBOL(ib_uverbs_get_ucontext_file);
-int uverbs_dealloc_mw(struct ib_mw *mw)
+void uverbs_dealloc_mw(struct ib_mw *mw)
{
struct ib_pd *pd = mw->pd;
- int ret;
- ret = mw->device->ops.dealloc_mw(mw);
- if (!ret)
- atomic_dec(&pd->usecnt);
- return ret;
+ mw->device->ops.dealloc_mw(mw);
+ atomic_dec(&pd->usecnt);
+ kfree(mw);
}
static void ib_uverbs_release_dev(struct device *device)
diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
index 08c39cfb1bd9..e4994cc4cc51 100644
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -72,7 +72,8 @@ static int uverbs_free_mw(struct ib_uobject *uobject,
enum rdma_remove_reason why,
struct uverbs_attr_bundle *attrs)
{
- return uverbs_dealloc_mw((struct ib_mw *)uobject->object);
+ uverbs_dealloc_mw((struct ib_mw *)uobject->object);
+ return 0;
}
static int uverbs_free_rwq_ind_tbl(struct ib_uobject *uobject,
diff --git a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
index 27da0705c88a..5545bcd0043e 100644
--- a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
+++ b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
@@ -983,10 +983,9 @@ struct ib_mr *c4iw_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type,
u32 max_num_sg, struct ib_udata *udata);
int c4iw_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg, int sg_nents,
unsigned int *sg_offset);
-int c4iw_dealloc_mw(struct ib_mw *mw);
+void c4iw_dealloc_mw(struct ib_mw *mw);
void c4iw_dealloc(struct uld_ctx *ctx);
-struct ib_mw *c4iw_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
- struct ib_udata *udata);
+int c4iw_alloc_mw(struct ib_mw *mw, u32 *rkey, struct ib_udata *udata);
struct ib_mr *c4iw_reg_user_mr(struct ib_pd *pd, u64 start,
u64 length, u64 virt, int acc,
struct ib_udata *udata);
diff --git a/drivers/infiniband/hw/cxgb4/mem.c b/drivers/infiniband/hw/cxgb4/mem.c
index 1e4f4e525598..edb62d9cf404 100644
--- a/drivers/infiniband/hw/cxgb4/mem.c
+++ b/drivers/infiniband/hw/cxgb4/mem.c
@@ -611,30 +611,23 @@ struct ib_mr *c4iw_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
return ERR_PTR(err);
}
-struct ib_mw *c4iw_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
- struct ib_udata *udata)
+int c4iw_alloc_mw(struct ib_mw *ibmw, u32 *rkey, struct ib_udata *udata)
{
+ struct c4iw_mw *mhp = to_c4iw_mw(ibmw);
struct c4iw_dev *rhp;
struct c4iw_pd *php;
- struct c4iw_mw *mhp;
u32 mmid;
u32 stag = 0;
int ret;
- if (type != IB_MW_TYPE_1)
- return ERR_PTR(-EINVAL);
+ if (ibmw->type != IB_MW_TYPE_1)
+ return -EINVAL;
- php = to_c4iw_pd(pd);
+ php = to_c4iw_pd(ibmw->pd);
rhp = php->rhp;
- mhp = kzalloc(sizeof(*mhp), GFP_KERNEL);
- if (!mhp)
- return ERR_PTR(-ENOMEM);
-
mhp->wr_waitp = c4iw_alloc_wr_wait(GFP_KERNEL);
- if (!mhp->wr_waitp) {
- ret = -ENOMEM;
- goto free_mhp;
- }
+ if (!mhp->wr_waitp)
+ return -ENOMEM;
mhp->dereg_skb = alloc_skb(SGE_MAX_WR_LEN, GFP_KERNEL);
if (!mhp->dereg_skb) {
@@ -645,18 +638,19 @@ struct ib_mw *c4iw_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
ret = allocate_window(&rhp->rdev, &stag, php->pdid, mhp->wr_waitp);
if (ret)
goto free_skb;
+
mhp->rhp = rhp;
mhp->attr.pdid = php->pdid;
mhp->attr.type = FW_RI_STAG_MW;
mhp->attr.stag = stag;
mmid = (stag) >> 8;
- mhp->ibmw.rkey = stag;
+ *rkey = stag;
if (xa_insert_irq(&rhp->mrs, mmid, mhp, GFP_KERNEL)) {
ret = -ENOMEM;
goto dealloc_win;
}
pr_debug("mmid 0x%x mhp %p stag 0x%x\n", mmid, mhp, stag);
- return &(mhp->ibmw);
+ return 0;
dealloc_win:
deallocate_window(&rhp->rdev, mhp->attr.stag, mhp->dereg_skb,
@@ -665,12 +659,10 @@ struct ib_mw *c4iw_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
kfree_skb(mhp->dereg_skb);
free_wr_wait:
c4iw_put_wr_wait(mhp->wr_waitp);
-free_mhp:
- kfree(mhp);
- return ERR_PTR(ret);
+ return ret;
}
-int c4iw_dealloc_mw(struct ib_mw *mw)
+void c4iw_dealloc_mw(struct ib_mw *mw)
{
struct c4iw_dev *rhp;
struct c4iw_mw *mhp;
@@ -684,9 +676,6 @@ int c4iw_dealloc_mw(struct ib_mw *mw)
mhp->wr_waitp);
kfree_skb(mhp->dereg_skb);
c4iw_put_wr_wait(mhp->wr_waitp);
- pr_debug("ib_mw %p mmid 0x%x ptr %p\n", mw, mmid, mhp);
- kfree(mhp);
- return 0;
}
struct ib_mr *c4iw_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type,
diff --git a/drivers/infiniband/hw/cxgb4/provider.c b/drivers/infiniband/hw/cxgb4/provider.c
index 1d3ff59e4060..7ec74e2fa885 100644
--- a/drivers/infiniband/hw/cxgb4/provider.c
+++ b/drivers/infiniband/hw/cxgb4/provider.c
@@ -508,8 +508,10 @@ static const struct ib_device_ops c4iw_dev_ops = {
.query_qp = c4iw_ib_query_qp,
.reg_user_mr = c4iw_reg_user_mr,
.req_notify_cq = c4iw_arm_cq,
- INIT_RDMA_OBJ_SIZE(ib_pd, c4iw_pd, ibpd),
+
INIT_RDMA_OBJ_SIZE(ib_cq, c4iw_cq, ibcq),
+ INIT_RDMA_OBJ_SIZE(ib_mw, c4iw_mw, ibmw),
+ INIT_RDMA_OBJ_SIZE(ib_pd, c4iw_pd, ibpd),
INIT_RDMA_OBJ_SIZE(ib_srq, c4iw_srq, ibsrq),
INIT_RDMA_OBJ_SIZE(ib_ucontext, c4iw_ucontext, ibucontext),
};
diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
index b890ff63f24d..e745e5c41770 100644
--- a/drivers/infiniband/hw/hns/hns_roce_device.h
+++ b/drivers/infiniband/hw/hns/hns_roce_device.h
@@ -1201,9 +1201,8 @@ int hns_roce_hw_destroy_mpt(struct hns_roce_dev *hr_dev,
unsigned long mpt_index);
unsigned long key_to_hw_index(u32 key);
-struct ib_mw *hns_roce_alloc_mw(struct ib_pd *pd, enum ib_mw_type,
- struct ib_udata *udata);
-int hns_roce_dealloc_mw(struct ib_mw *ibmw);
+int hns_roce_alloc_mw(struct ib_mw *pd, u32 *rkey, struct ib_udata *udata);
+void hns_roce_dealloc_mw(struct ib_mw *ibmw);
void hns_roce_buf_free(struct hns_roce_dev *hr_dev, struct hns_roce_buf *buf);
int hns_roce_buf_alloc(struct hns_roce_dev *hr_dev, u32 size, u32 max_direct,
diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c b/drivers/infiniband/hw/hns/hns_roce_main.c
index 5907cfd878a6..45f7353bd348 100644
--- a/drivers/infiniband/hw/hns/hns_roce_main.c
+++ b/drivers/infiniband/hw/hns/hns_roce_main.c
@@ -454,6 +454,8 @@ static const struct ib_device_ops hns_roce_dev_mr_ops = {
static const struct ib_device_ops hns_roce_dev_mw_ops = {
.alloc_mw = hns_roce_alloc_mw,
.dealloc_mw = hns_roce_dealloc_mw,
+
+ INIT_RDMA_OBJ_SIZE(ib_mw, hns_roce_mw, ibmw),
};
static const struct ib_device_ops hns_roce_dev_frmr_ops = {
diff --git a/drivers/infiniband/hw/hns/hns_roce_mr.c b/drivers/infiniband/hw/hns/hns_roce_mr.c
index 0e71ebee9e52..4af2797c3b95 100644
--- a/drivers/infiniband/hw/hns/hns_roce_mr.c
+++ b/drivers/infiniband/hw/hns/hns_roce_mr.c
@@ -589,28 +589,22 @@ static int hns_roce_mw_enable(struct hns_roce_dev *hr_dev,
return ret;
}
-struct ib_mw *hns_roce_alloc_mw(struct ib_pd *ib_pd, enum ib_mw_type type,
- struct ib_udata *udata)
+int hns_roce_alloc_mw(struct ib_mw *ibmw, u32 *rkey, struct ib_udata *udata)
{
- struct hns_roce_dev *hr_dev = to_hr_dev(ib_pd->device);
- struct hns_roce_mw *mw;
+ struct hns_roce_dev *hr_dev = to_hr_dev(ibmw->device);
+ struct hns_roce_mw *mw = to_hr_mw(ibmw);
unsigned long index = 0;
int ret;
- mw = kmalloc(sizeof(*mw), GFP_KERNEL);
- if (!mw)
- return ERR_PTR(-ENOMEM);
-
/* Allocate a key for mw from bitmap */
ret = hns_roce_bitmap_alloc(&hr_dev->mr_table.mtpt_bitmap, &index);
if (ret)
- goto err_bitmap;
+ return ret;
mw->rkey = hw_index_to_key(index);
- mw->ibmw.rkey = mw->rkey;
- mw->ibmw.type = type;
- mw->pdn = to_hr_pd(ib_pd)->pdn;
+ *rkey = mw->rkey;
+ mw->pdn = to_hr_pd(ibmw->pd)->pdn;
mw->pbl_hop_num = hr_dev->caps.pbl_hop_num;
mw->pbl_ba_pg_sz = hr_dev->caps.pbl_ba_pg_sz;
mw->pbl_buf_pg_sz = hr_dev->caps.pbl_buf_pg_sz;
@@ -619,26 +613,19 @@ struct ib_mw *hns_roce_alloc_mw(struct ib_pd *ib_pd, enum ib_mw_type type,
if (ret)
goto err_mw;
- return &mw->ibmw;
+ return 0;
err_mw:
hns_roce_mw_free(hr_dev, mw);
-
-err_bitmap:
- kfree(mw);
-
- return ERR_PTR(ret);
+ return ret;
}
-int hns_roce_dealloc_mw(struct ib_mw *ibmw)
+void hns_roce_dealloc_mw(struct ib_mw *ibmw)
{
struct hns_roce_dev *hr_dev = to_hr_dev(ibmw->device);
struct hns_roce_mw *mw = to_hr_mw(ibmw);
hns_roce_mw_free(hr_dev, mw);
- kfree(mw);
-
- return 0;
}
static int mtr_map_region(struct hns_roce_dev *hr_dev, struct hns_roce_mtr *mtr,
diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 816d28854a8e..6471f47bd365 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -2602,6 +2602,8 @@ static const struct ib_device_ops mlx4_ib_dev_wq_ops = {
static const struct ib_device_ops mlx4_ib_dev_mw_ops = {
.alloc_mw = mlx4_ib_alloc_mw,
.dealloc_mw = mlx4_ib_dealloc_mw,
+
+ INIT_RDMA_OBJ_SIZE(ib_mw, mlx4_ib_mw, ibmw),
};
static const struct ib_device_ops mlx4_ib_dev_xrc_ops = {
diff --git a/drivers/infiniband/hw/mlx4/mlx4_ib.h b/drivers/infiniband/hw/mlx4/mlx4_ib.h
index 6f4ea1067095..5fbe766aef6b 100644
--- a/drivers/infiniband/hw/mlx4/mlx4_ib.h
+++ b/drivers/infiniband/hw/mlx4/mlx4_ib.h
@@ -725,9 +725,8 @@ struct ib_mr *mlx4_ib_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
u64 virt_addr, int access_flags,
struct ib_udata *udata);
int mlx4_ib_dereg_mr(struct ib_mr *mr, struct ib_udata *udata);
-struct ib_mw *mlx4_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
- struct ib_udata *udata);
-int mlx4_ib_dealloc_mw(struct ib_mw *mw);
+int mlx4_ib_alloc_mw(struct ib_mw *mw, u32 *rkey, struct ib_udata *udata);
+void mlx4_ib_dealloc_mw(struct ib_mw *mw);
struct ib_mr *mlx4_ib_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type,
u32 max_num_sg, struct ib_udata *udata);
int mlx4_ib_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg, int sg_nents,
diff --git a/drivers/infiniband/hw/mlx4/mr.c b/drivers/infiniband/hw/mlx4/mr.c
index d7c78f841d2f..82d7d8dcc9d4 100644
--- a/drivers/infiniband/hw/mlx4/mr.c
+++ b/drivers/infiniband/hw/mlx4/mr.c
@@ -610,47 +610,35 @@ int mlx4_ib_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
return 0;
}
-struct ib_mw *mlx4_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
- struct ib_udata *udata)
+int mlx4_ib_alloc_mw(struct ib_mw *ibmw, u32 *rkey, struct ib_udata *udata)
{
- struct mlx4_ib_dev *dev = to_mdev(pd->device);
- struct mlx4_ib_mw *mw;
+ struct mlx4_ib_dev *dev = to_mdev(ibmw->device);
+ struct mlx4_ib_mw *mw = to_mmw(ibmw);
int err;
- mw = kmalloc(sizeof(*mw), GFP_KERNEL);
- if (!mw)
- return ERR_PTR(-ENOMEM);
-
- err = mlx4_mw_alloc(dev->dev, to_mpd(pd)->pdn,
- to_mlx4_type(type), &mw->mmw);
+ err = mlx4_mw_alloc(dev->dev, to_mpd(ibmw->pd)->pdn,
+ to_mlx4_type(ibmw->type), &mw->mmw);
if (err)
- goto err_free;
+ return err;
err = mlx4_mw_enable(dev->dev, &mw->mmw);
if (err)
goto err_mw;
- mw->ibmw.rkey = mw->mmw.key;
+ *rkey = mw->mmw.key;
- return &mw->ibmw;
+ return 0;
err_mw:
mlx4_mw_free(dev->dev, &mw->mmw);
-
-err_free:
- kfree(mw);
-
- return ERR_PTR(err);
+ return err;
}
-int mlx4_ib_dealloc_mw(struct ib_mw *ibmw)
+void mlx4_ib_dealloc_mw(struct ib_mw *ibmw)
{
struct mlx4_ib_mw *mw = to_mmw(ibmw);
mlx4_mw_free(to_mdev(ibmw->device)->dev, &mw->mmw);
- kfree(mw);
-
- return 0;
}
struct ib_mr *mlx4_ib_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type,
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index ca35394a181a..8b0b1f95af2f 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -6669,6 +6669,8 @@ static const struct ib_device_ops mlx5_ib_dev_sriov_ops = {
static const struct ib_device_ops mlx5_ib_dev_mw_ops = {
.alloc_mw = mlx5_ib_alloc_mw,
.dealloc_mw = mlx5_ib_dealloc_mw,
+
+ INIT_RDMA_OBJ_SIZE(ib_mw, mlx5_ib_mw, ibmw),
};
static const struct ib_device_ops mlx5_ib_dev_xrc_ops = {
diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index 26545e88709d..d84f56517caa 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -1195,9 +1195,8 @@ int mlx5_ib_advise_mr(struct ib_pd *pd,
struct ib_sge *sg_list,
u32 num_sge,
struct uverbs_attr_bundle *attrs);
-struct ib_mw *mlx5_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
- struct ib_udata *udata);
-int mlx5_ib_dealloc_mw(struct ib_mw *mw);
+int mlx5_ib_alloc_mw(struct ib_mw *mw, u32 *rkey, struct ib_udata *udata);
+void mlx5_ib_dealloc_mw(struct ib_mw *mw);
int mlx5_ib_update_xlt(struct mlx5_ib_mr *mr, u64 idx, int npages,
int page_shift, int flags);
struct mlx5_ib_mr *mlx5_ib_alloc_implicit_mr(struct mlx5_ib_pd *pd,
diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
index 44683073be0c..58a3d62937a1 100644
--- a/drivers/infiniband/hw/mlx5/mr.c
+++ b/drivers/infiniband/hw/mlx5/mr.c
@@ -1973,12 +1973,11 @@ struct ib_mr *mlx5_ib_alloc_mr_integrity(struct ib_pd *pd,
max_num_meta_sg);
}
-struct ib_mw *mlx5_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
- struct ib_udata *udata)
+int mlx5_ib_alloc_mw(struct ib_mw *ibmw, u32 *rkey, struct ib_udata *udata)
{
- struct mlx5_ib_dev *dev = to_mdev(pd->device);
+ struct mlx5_ib_dev *dev = to_mdev(ibmw->device);
int inlen = MLX5_ST_SZ_BYTES(create_mkey_in);
- struct mlx5_ib_mw *mw = NULL;
+ struct mlx5_ib_mw *mw = to_mmw(ibmw);
u32 *in = NULL;
void *mkc;
int ndescs;
@@ -1991,21 +1990,20 @@ struct ib_mw *mlx5_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
err = ib_copy_from_udata(&req, udata, min(udata->inlen, sizeof(req)));
if (err)
- return ERR_PTR(err);
+ return err;
if (req.comp_mask || req.reserved1 || req.reserved2)
- return ERR_PTR(-EOPNOTSUPP);
+ return -EOPNOTSUPP;
if (udata->inlen > sizeof(req) &&
!ib_is_udata_cleared(udata, sizeof(req),
udata->inlen - sizeof(req)))
- return ERR_PTR(-EOPNOTSUPP);
+ return -EOPNOTSUPP;
ndescs = req.num_klms ? roundup(req.num_klms, 4) : roundup(1, 4);
- mw = kzalloc(sizeof(*mw), GFP_KERNEL);
in = kzalloc(inlen, GFP_KERNEL);
- if (!mw || !in) {
+ if (!in) {
err = -ENOMEM;
goto free;
}
@@ -2014,11 +2012,11 @@ struct ib_mw *mlx5_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
MLX5_SET(mkc, mkc, free, 1);
MLX5_SET(mkc, mkc, translations_octword_size, ndescs);
- MLX5_SET(mkc, mkc, pd, to_mpd(pd)->pdn);
+ MLX5_SET(mkc, mkc, pd, to_mpd(ibmw->pd)->pdn);
MLX5_SET(mkc, mkc, umr_en, 1);
MLX5_SET(mkc, mkc, lr, 1);
MLX5_SET(mkc, mkc, access_mode_1_0, MLX5_MKC_ACCESS_MODE_KLMS);
- MLX5_SET(mkc, mkc, en_rinval, !!((type == IB_MW_TYPE_2)));
+ MLX5_SET(mkc, mkc, en_rinval, !!((ibmw->type == IB_MW_TYPE_2)));
MLX5_SET(mkc, mkc, qpn, 0xffffff);
err = mlx5_ib_create_mkey(dev, &mw->mmkey, in, inlen);
@@ -2026,17 +2024,15 @@ struct ib_mw *mlx5_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
goto free;
mw->mmkey.type = MLX5_MKEY_MW;
- mw->ibmw.rkey = mw->mmkey.key;
+ *rkey = mw->mmkey.key;
mw->ndescs = ndescs;
resp.response_length = min(offsetof(typeof(resp), response_length) +
sizeof(resp.response_length), udata->outlen);
if (resp.response_length) {
err = ib_copy_to_udata(udata, &resp, resp.response_length);
- if (err) {
- mlx5_core_destroy_mkey(dev->mdev, &mw->mmkey);
- goto free;
- }
+ if (err)
+ goto free_mkey;
}
if (IS_ENABLED(CONFIG_INFINIBAND_ON_DEMAND_PAGING)) {
@@ -2048,21 +2044,19 @@ struct ib_mw *mlx5_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
}
kfree(in);
- return &mw->ibmw;
+ return 0;
free_mkey:
mlx5_core_destroy_mkey(dev->mdev, &mw->mmkey);
free:
- kfree(mw);
kfree(in);
- return ERR_PTR(err);
+ return err;
}
-int mlx5_ib_dealloc_mw(struct ib_mw *mw)
+void mlx5_ib_dealloc_mw(struct ib_mw *mw)
{
struct mlx5_ib_dev *dev = to_mdev(mw->device);
struct mlx5_ib_mw *mmw = to_mmw(mw);
- int err;
if (IS_ENABLED(CONFIG_INFINIBAND_ON_DEMAND_PAGING)) {
xa_erase(&dev->odp_mkeys, mlx5_base_mkey(mmw->mmkey.key));
@@ -2073,11 +2067,7 @@ int mlx5_ib_dealloc_mw(struct ib_mw *mw)
synchronize_srcu(&dev->odp_srcu);
}
- err = mlx5_core_destroy_mkey(dev->mdev, &mmw->mmkey);
- if (err)
- return err;
- kfree(mmw);
- return 0;
+ mlx5_core_destroy_mkey(dev->mdev, &mmw->mmkey);
}
int mlx5_ib_check_mr_status(struct ib_mr *ibmr, u32 check_mask,
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 3aa20ce23556..7160662899bb 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2490,9 +2490,8 @@ struct ib_device_ops {
unsigned int *sg_offset);
int (*check_mr_status)(struct ib_mr *mr, u32 check_mask,
struct ib_mr_status *mr_status);
- struct ib_mw *(*alloc_mw)(struct ib_pd *pd, enum ib_mw_type type,
- struct ib_udata *udata);
- int (*dealloc_mw)(struct ib_mw *mw);
+ int (*alloc_mw)(struct ib_mw *mw, u32 *rkey, struct ib_udata *udata);
+ void (*dealloc_mw)(struct ib_mw *mw);
int (*attach_mcast)(struct ib_qp *qp, union ib_gid *gid, u16 lid);
int (*detach_mcast)(struct ib_qp *qp, union ib_gid *gid, u16 lid);
struct ib_xrcd *(*alloc_xrcd)(struct ib_device *device,
@@ -2653,6 +2652,7 @@ struct ib_device_ops {
DECLARE_RDMA_OBJ_SIZE(ib_ah);
DECLARE_RDMA_OBJ_SIZE(ib_counters);
DECLARE_RDMA_OBJ_SIZE(ib_cq);
+ DECLARE_RDMA_OBJ_SIZE(ib_mw);
DECLARE_RDMA_OBJ_SIZE(ib_pd);
DECLARE_RDMA_OBJ_SIZE(ib_srq);
DECLARE_RDMA_OBJ_SIZE(ib_ucontext);
--
2.26.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 2/5] RDMA: Clean MW allocation and free flows
2020-06-24 10:54 ` [PATCH rdma-next 2/5] RDMA: Clean MW allocation and free flows Leon Romanovsky
@ 2020-06-28 9:18 ` Yishai Hadas
2020-06-28 9:55 ` Leon Romanovsky
0 siblings, 1 reply; 21+ messages in thread
From: Yishai Hadas @ 2020-06-28 9:18 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Jason Gunthorpe, Leon Romanovsky, Lijun Ou,
linux-rdma, Potnuri Bharat Teja, Weihang Li, Wei Hu(Xavier),
Yishai Hadas, Maor Gottlieb
On 6/24/2020 1:54 PM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> Move allocation and destruction of memory windows under ib_core
> responsibility and clean drivers to ensure that no updates to MW
> ib_core structures are done in driver layer.
>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
> drivers/infiniband/core/device.c | 1 +
> drivers/infiniband/core/uverbs.h | 2 +-
> drivers/infiniband/core/uverbs_cmd.c | 26 ++++++++-----
> drivers/infiniband/core/uverbs_main.c | 10 ++---
> drivers/infiniband/core/uverbs_std_types.c | 3 +-
> drivers/infiniband/hw/cxgb4/iw_cxgb4.h | 5 +--
> drivers/infiniband/hw/cxgb4/mem.c | 35 ++++++-----------
> drivers/infiniband/hw/cxgb4/provider.c | 4 +-
> drivers/infiniband/hw/hns/hns_roce_device.h | 5 +--
> drivers/infiniband/hw/hns/hns_roce_main.c | 2 +
> drivers/infiniband/hw/hns/hns_roce_mr.c | 31 +++++----------
> drivers/infiniband/hw/mlx4/main.c | 2 +
> drivers/infiniband/hw/mlx4/mlx4_ib.h | 5 +--
> drivers/infiniband/hw/mlx4/mr.c | 32 +++++-----------
> drivers/infiniband/hw/mlx5/main.c | 2 +
> drivers/infiniband/hw/mlx5/mlx5_ib.h | 5 +--
> drivers/infiniband/hw/mlx5/mr.c | 42 ++++++++-------------
> include/rdma/ib_verbs.h | 6 +--
> 18 files changed, 91 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 088559d72d57..257e8a667977 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -2684,6 +2684,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
> SET_OBJ_SIZE(dev_ops, ib_ah);
> SET_OBJ_SIZE(dev_ops, ib_counters);
> SET_OBJ_SIZE(dev_ops, ib_cq);
> + SET_OBJ_SIZE(dev_ops, ib_mw);
> SET_OBJ_SIZE(dev_ops, ib_pd);
> SET_OBJ_SIZE(dev_ops, ib_srq);
> SET_OBJ_SIZE(dev_ops, ib_ucontext);
> diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
> index 53a10479958b..072bfe4e1b5b 100644
> --- a/drivers/infiniband/core/uverbs.h
> +++ b/drivers/infiniband/core/uverbs.h
> @@ -242,7 +242,7 @@ int ib_uverbs_dealloc_xrcd(struct ib_uobject *uobject, struct ib_xrcd *xrcd,
> enum rdma_remove_reason why,
> struct uverbs_attr_bundle *attrs);
>
> -int uverbs_dealloc_mw(struct ib_mw *mw);
> +void uverbs_dealloc_mw(struct ib_mw *mw);
> void ib_uverbs_detach_umcast(struct ib_qp *qp,
> struct ib_uqp_object *uobj);
>
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 68c9a0210220..a5301f3d3059 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -890,7 +890,7 @@ static int ib_uverbs_dereg_mr(struct uverbs_attr_bundle *attrs)
> static int ib_uverbs_alloc_mw(struct uverbs_attr_bundle *attrs)
> {
> struct ib_uverbs_alloc_mw cmd;
> - struct ib_uverbs_alloc_mw_resp resp;
> + struct ib_uverbs_alloc_mw_resp resp = {};
> struct ib_uobject *uobj;
> struct ib_pd *pd;
> struct ib_mw *mw;
> @@ -916,21 +916,24 @@ static int ib_uverbs_alloc_mw(struct uverbs_attr_bundle *attrs)
> goto err_put;
> }
>
> - mw = pd->device->ops.alloc_mw(pd, cmd.mw_type, &attrs->driver_udata);
> - if (IS_ERR(mw)) {
> - ret = PTR_ERR(mw);
> + mw = rdma_zalloc_drv_obj(ib_dev, ib_mw);
> + if (!mw) {
> + ret = -ENOMEM;
> goto err_put;
> }
>
> - mw->device = pd->device;
> - mw->pd = pd;
> + mw->device = ib_dev;
> + mw->pd = pd;
> mw->uobject = uobj;
> - atomic_inc(&pd->usecnt);
> -
> uobj->object = mw;
> + mw->type = cmd.mw_type;
>
> - memset(&resp, 0, sizeof(resp));
> - resp.rkey = mw->rkey;
> + ret = pd->device->ops.alloc_mw(mw, &mw->rkey, &attrs->driver_udata);
> + if (ret)
> + goto err_alloc;
> +
> + atomic_inc(&pd->usecnt);
> + resp.rkey = mw->rkey;
> resp.mw_handle = uobj->id;
>
> ret = uverbs_response(attrs, &resp, sizeof(resp));
> @@ -943,6 +946,9 @@ static int ib_uverbs_alloc_mw(struct uverbs_attr_bundle *attrs)
>
> err_copy:
> uverbs_dealloc_mw(mw);
> + mw = NULL;
> +err_alloc:
> + kfree(mw);
> err_put:
> uobj_put_obj_read(pd);
> err_free:
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index 69e4755cc04b..706c972ea3a1 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -102,15 +102,13 @@ struct ib_ucontext *ib_uverbs_get_ucontext_file(struct ib_uverbs_file *ufile)
> }
> EXPORT_SYMBOL(ib_uverbs_get_ucontext_file);
>
> -int uverbs_dealloc_mw(struct ib_mw *mw)
> +void uverbs_dealloc_mw(struct ib_mw *mw)
> {
> struct ib_pd *pd = mw->pd;
> - int ret;
>
> - ret = mw->device->ops.dealloc_mw(mw);
> - if (!ret)
> - atomic_dec(&pd->usecnt);
> - return ret;
> + mw->device->ops.dealloc_mw(mw);
> + atomic_dec(&pd->usecnt);
> + kfree(mw);
> }
>
> static void ib_uverbs_release_dev(struct device *device)
> diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
> index 08c39cfb1bd9..e4994cc4cc51 100644
> --- a/drivers/infiniband/core/uverbs_std_types.c
> +++ b/drivers/infiniband/core/uverbs_std_types.c
> @@ -72,7 +72,8 @@ static int uverbs_free_mw(struct ib_uobject *uobject,
> enum rdma_remove_reason why,
> struct uverbs_attr_bundle *attrs)
> {
> - return uverbs_dealloc_mw((struct ib_mw *)uobject->object);
> + uverbs_dealloc_mw((struct ib_mw *)uobject->object);
> + return 0;
> }
>
> static int uverbs_free_rwq_ind_tbl(struct ib_uobject *uobject,
> diff --git a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
> index 27da0705c88a..5545bcd0043e 100644
> --- a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
> +++ b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
> @@ -983,10 +983,9 @@ struct ib_mr *c4iw_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type,
> u32 max_num_sg, struct ib_udata *udata);
> int c4iw_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg, int sg_nents,
> unsigned int *sg_offset);
> -int c4iw_dealloc_mw(struct ib_mw *mw);
> +void c4iw_dealloc_mw(struct ib_mw *mw);
> void c4iw_dealloc(struct uld_ctx *ctx);
> -struct ib_mw *c4iw_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
> - struct ib_udata *udata);
> +int c4iw_alloc_mw(struct ib_mw *mw, u32 *rkey, struct ib_udata *udata);
> struct ib_mr *c4iw_reg_user_mr(struct ib_pd *pd, u64 start,
> u64 length, u64 virt, int acc,
> struct ib_udata *udata);
> diff --git a/drivers/infiniband/hw/cxgb4/mem.c b/drivers/infiniband/hw/cxgb4/mem.c
> index 1e4f4e525598..edb62d9cf404 100644
> --- a/drivers/infiniband/hw/cxgb4/mem.c
> +++ b/drivers/infiniband/hw/cxgb4/mem.c
> @@ -611,30 +611,23 @@ struct ib_mr *c4iw_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
> return ERR_PTR(err);
> }
>
> -struct ib_mw *c4iw_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
> - struct ib_udata *udata)
> +int c4iw_alloc_mw(struct ib_mw *ibmw, u32 *rkey, struct ib_udata *udata)
> {
> + struct c4iw_mw *mhp = to_c4iw_mw(ibmw);
> struct c4iw_dev *rhp;
> struct c4iw_pd *php;
> - struct c4iw_mw *mhp;
> u32 mmid;
> u32 stag = 0;
> int ret;
>
> - if (type != IB_MW_TYPE_1)
> - return ERR_PTR(-EINVAL);
> + if (ibmw->type != IB_MW_TYPE_1)
> + return -EINVAL;
>
> - php = to_c4iw_pd(pd);
> + php = to_c4iw_pd(ibmw->pd);
> rhp = php->rhp;
> - mhp = kzalloc(sizeof(*mhp), GFP_KERNEL);
> - if (!mhp)
> - return ERR_PTR(-ENOMEM);
> -
> mhp->wr_waitp = c4iw_alloc_wr_wait(GFP_KERNEL);
> - if (!mhp->wr_waitp) {
> - ret = -ENOMEM;
> - goto free_mhp;
> - }
> + if (!mhp->wr_waitp)
> + return -ENOMEM;
>
> mhp->dereg_skb = alloc_skb(SGE_MAX_WR_LEN, GFP_KERNEL);
> if (!mhp->dereg_skb) {
> @@ -645,18 +638,19 @@ struct ib_mw *c4iw_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
> ret = allocate_window(&rhp->rdev, &stag, php->pdid, mhp->wr_waitp);
> if (ret)
> goto free_skb;
> +
> mhp->rhp = rhp;
> mhp->attr.pdid = php->pdid;
> mhp->attr.type = FW_RI_STAG_MW;
> mhp->attr.stag = stag;
> mmid = (stag) >> 8;
> - mhp->ibmw.rkey = stag;
> + *rkey = stag;
> if (xa_insert_irq(&rhp->mrs, mmid, mhp, GFP_KERNEL)) {
> ret = -ENOMEM;
> goto dealloc_win;
> }
> pr_debug("mmid 0x%x mhp %p stag 0x%x\n", mmid, mhp, stag);
> - return &(mhp->ibmw);
> + return 0;
>
> dealloc_win:
> deallocate_window(&rhp->rdev, mhp->attr.stag, mhp->dereg_skb,
> @@ -665,12 +659,10 @@ struct ib_mw *c4iw_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
> kfree_skb(mhp->dereg_skb);
> free_wr_wait:
> c4iw_put_wr_wait(mhp->wr_waitp);
> -free_mhp:
> - kfree(mhp);
> - return ERR_PTR(ret);
> + return ret;
> }
>
> -int c4iw_dealloc_mw(struct ib_mw *mw)
> +void c4iw_dealloc_mw(struct ib_mw *mw)
> {
> struct c4iw_dev *rhp;
> struct c4iw_mw *mhp;
> @@ -684,9 +676,6 @@ int c4iw_dealloc_mw(struct ib_mw *mw)
> mhp->wr_waitp);
> kfree_skb(mhp->dereg_skb);
> c4iw_put_wr_wait(mhp->wr_waitp);
> - pr_debug("ib_mw %p mmid 0x%x ptr %p\n", mw, mmid, mhp);
> - kfree(mhp);
> - return 0;
> }
>
> struct ib_mr *c4iw_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type,
> diff --git a/drivers/infiniband/hw/cxgb4/provider.c b/drivers/infiniband/hw/cxgb4/provider.c
> index 1d3ff59e4060..7ec74e2fa885 100644
> --- a/drivers/infiniband/hw/cxgb4/provider.c
> +++ b/drivers/infiniband/hw/cxgb4/provider.c
> @@ -508,8 +508,10 @@ static const struct ib_device_ops c4iw_dev_ops = {
> .query_qp = c4iw_ib_query_qp,
> .reg_user_mr = c4iw_reg_user_mr,
> .req_notify_cq = c4iw_arm_cq,
> - INIT_RDMA_OBJ_SIZE(ib_pd, c4iw_pd, ibpd),
> +
> INIT_RDMA_OBJ_SIZE(ib_cq, c4iw_cq, ibcq),
> + INIT_RDMA_OBJ_SIZE(ib_mw, c4iw_mw, ibmw),
> + INIT_RDMA_OBJ_SIZE(ib_pd, c4iw_pd, ibpd),
> INIT_RDMA_OBJ_SIZE(ib_srq, c4iw_srq, ibsrq),
> INIT_RDMA_OBJ_SIZE(ib_ucontext, c4iw_ucontext, ibucontext),
> };
> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
> index b890ff63f24d..e745e5c41770 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_device.h
> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
> @@ -1201,9 +1201,8 @@ int hns_roce_hw_destroy_mpt(struct hns_roce_dev *hr_dev,
> unsigned long mpt_index);
> unsigned long key_to_hw_index(u32 key);
>
> -struct ib_mw *hns_roce_alloc_mw(struct ib_pd *pd, enum ib_mw_type,
> - struct ib_udata *udata);
> -int hns_roce_dealloc_mw(struct ib_mw *ibmw);
> +int hns_roce_alloc_mw(struct ib_mw *pd, u32 *rkey, struct ib_udata *udata);
> +void hns_roce_dealloc_mw(struct ib_mw *ibmw);
>
> void hns_roce_buf_free(struct hns_roce_dev *hr_dev, struct hns_roce_buf *buf);
> int hns_roce_buf_alloc(struct hns_roce_dev *hr_dev, u32 size, u32 max_direct,
> diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c b/drivers/infiniband/hw/hns/hns_roce_main.c
> index 5907cfd878a6..45f7353bd348 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_main.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_main.c
> @@ -454,6 +454,8 @@ static const struct ib_device_ops hns_roce_dev_mr_ops = {
> static const struct ib_device_ops hns_roce_dev_mw_ops = {
> .alloc_mw = hns_roce_alloc_mw,
> .dealloc_mw = hns_roce_dealloc_mw,
> +
> + INIT_RDMA_OBJ_SIZE(ib_mw, hns_roce_mw, ibmw),
> };
>
> static const struct ib_device_ops hns_roce_dev_frmr_ops = {
> diff --git a/drivers/infiniband/hw/hns/hns_roce_mr.c b/drivers/infiniband/hw/hns/hns_roce_mr.c
> index 0e71ebee9e52..4af2797c3b95 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_mr.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_mr.c
> @@ -589,28 +589,22 @@ static int hns_roce_mw_enable(struct hns_roce_dev *hr_dev,
> return ret;
> }
>
> -struct ib_mw *hns_roce_alloc_mw(struct ib_pd *ib_pd, enum ib_mw_type type,
> - struct ib_udata *udata)
> +int hns_roce_alloc_mw(struct ib_mw *ibmw, u32 *rkey, struct ib_udata *udata)
> {
> - struct hns_roce_dev *hr_dev = to_hr_dev(ib_pd->device);
> - struct hns_roce_mw *mw;
> + struct hns_roce_dev *hr_dev = to_hr_dev(ibmw->device);
> + struct hns_roce_mw *mw = to_hr_mw(ibmw);
> unsigned long index = 0;
> int ret;
>
> - mw = kmalloc(sizeof(*mw), GFP_KERNEL);
> - if (!mw)
> - return ERR_PTR(-ENOMEM);
> -
> /* Allocate a key for mw from bitmap */
> ret = hns_roce_bitmap_alloc(&hr_dev->mr_table.mtpt_bitmap, &index);
> if (ret)
> - goto err_bitmap;
> + return ret;
>
> mw->rkey = hw_index_to_key(index);
>
> - mw->ibmw.rkey = mw->rkey;
> - mw->ibmw.type = type;
> - mw->pdn = to_hr_pd(ib_pd)->pdn;
> + *rkey = mw->rkey;
> + mw->pdn = to_hr_pd(ibmw->pd)->pdn;
> mw->pbl_hop_num = hr_dev->caps.pbl_hop_num;
> mw->pbl_ba_pg_sz = hr_dev->caps.pbl_ba_pg_sz;
> mw->pbl_buf_pg_sz = hr_dev->caps.pbl_buf_pg_sz;
> @@ -619,26 +613,19 @@ struct ib_mw *hns_roce_alloc_mw(struct ib_pd *ib_pd, enum ib_mw_type type,
> if (ret)
> goto err_mw;
>
> - return &mw->ibmw;
> + return 0;
>
> err_mw:
> hns_roce_mw_free(hr_dev, mw);
> -
> -err_bitmap:
> - kfree(mw);
> -
> - return ERR_PTR(ret);
> + return ret;
> }
>
> -int hns_roce_dealloc_mw(struct ib_mw *ibmw)
> +void hns_roce_dealloc_mw(struct ib_mw *ibmw)
> {
> struct hns_roce_dev *hr_dev = to_hr_dev(ibmw->device);
> struct hns_roce_mw *mw = to_hr_mw(ibmw);
>
> hns_roce_mw_free(hr_dev, mw);
> - kfree(mw);
> -
> - return 0;
> }
>
> static int mtr_map_region(struct hns_roce_dev *hr_dev, struct hns_roce_mtr *mtr,
> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> index 816d28854a8e..6471f47bd365 100644
> --- a/drivers/infiniband/hw/mlx4/main.c
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -2602,6 +2602,8 @@ static const struct ib_device_ops mlx4_ib_dev_wq_ops = {
> static const struct ib_device_ops mlx4_ib_dev_mw_ops = {
> .alloc_mw = mlx4_ib_alloc_mw,
> .dealloc_mw = mlx4_ib_dealloc_mw,
> +
> + INIT_RDMA_OBJ_SIZE(ib_mw, mlx4_ib_mw, ibmw),
> };
>
> static const struct ib_device_ops mlx4_ib_dev_xrc_ops = {
> diff --git a/drivers/infiniband/hw/mlx4/mlx4_ib.h b/drivers/infiniband/hw/mlx4/mlx4_ib.h
> index 6f4ea1067095..5fbe766aef6b 100644
> --- a/drivers/infiniband/hw/mlx4/mlx4_ib.h
> +++ b/drivers/infiniband/hw/mlx4/mlx4_ib.h
> @@ -725,9 +725,8 @@ struct ib_mr *mlx4_ib_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
> u64 virt_addr, int access_flags,
> struct ib_udata *udata);
> int mlx4_ib_dereg_mr(struct ib_mr *mr, struct ib_udata *udata);
> -struct ib_mw *mlx4_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
> - struct ib_udata *udata);
> -int mlx4_ib_dealloc_mw(struct ib_mw *mw);
> +int mlx4_ib_alloc_mw(struct ib_mw *mw, u32 *rkey, struct ib_udata *udata);
> +void mlx4_ib_dealloc_mw(struct ib_mw *mw);
> struct ib_mr *mlx4_ib_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type,
> u32 max_num_sg, struct ib_udata *udata);
> int mlx4_ib_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg, int sg_nents,
> diff --git a/drivers/infiniband/hw/mlx4/mr.c b/drivers/infiniband/hw/mlx4/mr.c
> index d7c78f841d2f..82d7d8dcc9d4 100644
> --- a/drivers/infiniband/hw/mlx4/mr.c
> +++ b/drivers/infiniband/hw/mlx4/mr.c
> @@ -610,47 +610,35 @@ int mlx4_ib_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
> return 0;
> }
>
> -struct ib_mw *mlx4_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
> - struct ib_udata *udata)
> +int mlx4_ib_alloc_mw(struct ib_mw *ibmw, u32 *rkey, struct ib_udata *udata)
> {
> - struct mlx4_ib_dev *dev = to_mdev(pd->device);
> - struct mlx4_ib_mw *mw;
> + struct mlx4_ib_dev *dev = to_mdev(ibmw->device);
> + struct mlx4_ib_mw *mw = to_mmw(ibmw);
> int err;
>
> - mw = kmalloc(sizeof(*mw), GFP_KERNEL);
> - if (!mw)
> - return ERR_PTR(-ENOMEM);
> -
> - err = mlx4_mw_alloc(dev->dev, to_mpd(pd)->pdn,
> - to_mlx4_type(type), &mw->mmw);
> + err = mlx4_mw_alloc(dev->dev, to_mpd(ibmw->pd)->pdn,
> + to_mlx4_type(ibmw->type), &mw->mmw);
> if (err)
> - goto err_free;
> + return err;
>
> err = mlx4_mw_enable(dev->dev, &mw->mmw);
> if (err)
> goto err_mw;
>
> - mw->ibmw.rkey = mw->mmw.key;
> + *rkey = mw->mmw.key;
>
> - return &mw->ibmw;
> + return 0;
>
> err_mw:
> mlx4_mw_free(dev->dev, &mw->mmw);
> -
> -err_free:
> - kfree(mw);
> -
> - return ERR_PTR(err);
> + return err;
> }
>
> -int mlx4_ib_dealloc_mw(struct ib_mw *ibmw)
> +void mlx4_ib_dealloc_mw(struct ib_mw *ibmw)
> {
> struct mlx4_ib_mw *mw = to_mmw(ibmw);
>
> mlx4_mw_free(to_mdev(ibmw->device)->dev, &mw->mmw);
> - kfree(mw);
> -
> - return 0;
> }
>
> struct ib_mr *mlx4_ib_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type,
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index ca35394a181a..8b0b1f95af2f 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -6669,6 +6669,8 @@ static const struct ib_device_ops mlx5_ib_dev_sriov_ops = {
> static const struct ib_device_ops mlx5_ib_dev_mw_ops = {
> .alloc_mw = mlx5_ib_alloc_mw,
> .dealloc_mw = mlx5_ib_dealloc_mw,
> +
> + INIT_RDMA_OBJ_SIZE(ib_mw, mlx5_ib_mw, ibmw),
> };
>
> static const struct ib_device_ops mlx5_ib_dev_xrc_ops = {
> diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> index 26545e88709d..d84f56517caa 100644
> --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
> +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> @@ -1195,9 +1195,8 @@ int mlx5_ib_advise_mr(struct ib_pd *pd,
> struct ib_sge *sg_list,
> u32 num_sge,
> struct uverbs_attr_bundle *attrs);
> -struct ib_mw *mlx5_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
> - struct ib_udata *udata);
> -int mlx5_ib_dealloc_mw(struct ib_mw *mw);
> +int mlx5_ib_alloc_mw(struct ib_mw *mw, u32 *rkey, struct ib_udata *udata);
> +void mlx5_ib_dealloc_mw(struct ib_mw *mw);
> int mlx5_ib_update_xlt(struct mlx5_ib_mr *mr, u64 idx, int npages,
> int page_shift, int flags);
> struct mlx5_ib_mr *mlx5_ib_alloc_implicit_mr(struct mlx5_ib_pd *pd,
> diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
> index 44683073be0c..58a3d62937a1 100644
> --- a/drivers/infiniband/hw/mlx5/mr.c
> +++ b/drivers/infiniband/hw/mlx5/mr.c
> @@ -1973,12 +1973,11 @@ struct ib_mr *mlx5_ib_alloc_mr_integrity(struct ib_pd *pd,
> max_num_meta_sg);
> }
>
> -struct ib_mw *mlx5_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
> - struct ib_udata *udata)
> +int mlx5_ib_alloc_mw(struct ib_mw *ibmw, u32 *rkey, struct ib_udata *udata)
> {
> - struct mlx5_ib_dev *dev = to_mdev(pd->device);
> + struct mlx5_ib_dev *dev = to_mdev(ibmw->device);
> int inlen = MLX5_ST_SZ_BYTES(create_mkey_in);
> - struct mlx5_ib_mw *mw = NULL;
> + struct mlx5_ib_mw *mw = to_mmw(ibmw);
> u32 *in = NULL;
> void *mkc;
> int ndescs;
> @@ -1991,21 +1990,20 @@ struct ib_mw *mlx5_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
>
> err = ib_copy_from_udata(&req, udata, min(udata->inlen, sizeof(req)));
> if (err)
> - return ERR_PTR(err);
> + return err;
>
> if (req.comp_mask || req.reserved1 || req.reserved2)
> - return ERR_PTR(-EOPNOTSUPP);
> + return -EOPNOTSUPP;
>
> if (udata->inlen > sizeof(req) &&
> !ib_is_udata_cleared(udata, sizeof(req),
> udata->inlen - sizeof(req)))
> - return ERR_PTR(-EOPNOTSUPP);
> + return -EOPNOTSUPP;
>
> ndescs = req.num_klms ? roundup(req.num_klms, 4) : roundup(1, 4);
>
> - mw = kzalloc(sizeof(*mw), GFP_KERNEL);
> in = kzalloc(inlen, GFP_KERNEL);
> - if (!mw || !in) {
> + if (!in) {
> err = -ENOMEM;
> goto free;
> }
> @@ -2014,11 +2012,11 @@ struct ib_mw *mlx5_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
>
> MLX5_SET(mkc, mkc, free, 1);
> MLX5_SET(mkc, mkc, translations_octword_size, ndescs);
> - MLX5_SET(mkc, mkc, pd, to_mpd(pd)->pdn);
> + MLX5_SET(mkc, mkc, pd, to_mpd(ibmw->pd)->pdn);
> MLX5_SET(mkc, mkc, umr_en, 1);
> MLX5_SET(mkc, mkc, lr, 1);
> MLX5_SET(mkc, mkc, access_mode_1_0, MLX5_MKC_ACCESS_MODE_KLMS);
> - MLX5_SET(mkc, mkc, en_rinval, !!((type == IB_MW_TYPE_2)));
> + MLX5_SET(mkc, mkc, en_rinval, !!((ibmw->type == IB_MW_TYPE_2)));
> MLX5_SET(mkc, mkc, qpn, 0xffffff);
>
> err = mlx5_ib_create_mkey(dev, &mw->mmkey, in, inlen);
> @@ -2026,17 +2024,15 @@ struct ib_mw *mlx5_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
> goto free;
>
> mw->mmkey.type = MLX5_MKEY_MW;
> - mw->ibmw.rkey = mw->mmkey.key;
> + *rkey = mw->mmkey.key;
> mw->ndescs = ndescs;
>
> resp.response_length = min(offsetof(typeof(resp), response_length) +
> sizeof(resp.response_length), udata->outlen);
> if (resp.response_length) {
> err = ib_copy_to_udata(udata, &resp, resp.response_length);
> - if (err) {
> - mlx5_core_destroy_mkey(dev->mdev, &mw->mmkey);
> - goto free;
> - }
> + if (err)
> + goto free_mkey;
> }
>
> if (IS_ENABLED(CONFIG_INFINIBAND_ON_DEMAND_PAGING)) {
> @@ -2048,21 +2044,19 @@ struct ib_mw *mlx5_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type,
> }
>
> kfree(in);
> - return &mw->ibmw;
> + return 0;
>
> free_mkey:
> mlx5_core_destroy_mkey(dev->mdev, &mw->mmkey);
> free:
> - kfree(mw);
> kfree(in);
> - return ERR_PTR(err);
> + return err;
> }
>
> -int mlx5_ib_dealloc_mw(struct ib_mw *mw)
> +void mlx5_ib_dealloc_mw(struct ib_mw *mw)
> {
> struct mlx5_ib_dev *dev = to_mdev(mw->device);
> struct mlx5_ib_mw *mmw = to_mmw(mw);
> - int err;
>
> if (IS_ENABLED(CONFIG_INFINIBAND_ON_DEMAND_PAGING)) {
> xa_erase(&dev->odp_mkeys, mlx5_base_mkey(mmw->mmkey.key));
> @@ -2073,11 +2067,7 @@ int mlx5_ib_dealloc_mw(struct ib_mw *mw)
> synchronize_srcu(&dev->odp_srcu);
> }
>
> - err = mlx5_core_destroy_mkey(dev->mdev, &mmw->mmkey);
> - if (err)
> - return err;
> - kfree(mmw);
> - return 0;
> + mlx5_core_destroy_mkey(dev->mdev, &mmw->mmkey);
Are we fully sure that this can never be triggered by user space to fail
as of the property of the MW that can be binded with bypassing kernel ?
the new code just ignored the err.
> }
>
> int mlx5_ib_check_mr_status(struct ib_mr *ibmr, u32 check_mask,
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 3aa20ce23556..7160662899bb 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -2490,9 +2490,8 @@ struct ib_device_ops {
> unsigned int *sg_offset);
> int (*check_mr_status)(struct ib_mr *mr, u32 check_mask,
> struct ib_mr_status *mr_status);
> - struct ib_mw *(*alloc_mw)(struct ib_pd *pd, enum ib_mw_type type,
> - struct ib_udata *udata);
> - int (*dealloc_mw)(struct ib_mw *mw);
> + int (*alloc_mw)(struct ib_mw *mw, u32 *rkey, struct ib_udata *udata);
> + void (*dealloc_mw)(struct ib_mw *mw);
> int (*attach_mcast)(struct ib_qp *qp, union ib_gid *gid, u16 lid);
> int (*detach_mcast)(struct ib_qp *qp, union ib_gid *gid, u16 lid);
> struct ib_xrcd *(*alloc_xrcd)(struct ib_device *device,
> @@ -2653,6 +2652,7 @@ struct ib_device_ops {
> DECLARE_RDMA_OBJ_SIZE(ib_ah);
> DECLARE_RDMA_OBJ_SIZE(ib_counters);
> DECLARE_RDMA_OBJ_SIZE(ib_cq);
> + DECLARE_RDMA_OBJ_SIZE(ib_mw);
> DECLARE_RDMA_OBJ_SIZE(ib_pd);
> DECLARE_RDMA_OBJ_SIZE(ib_srq);
> DECLARE_RDMA_OBJ_SIZE(ib_ucontext);
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 2/5] RDMA: Clean MW allocation and free flows
2020-06-28 9:18 ` Yishai Hadas
@ 2020-06-28 9:55 ` Leon Romanovsky
0 siblings, 0 replies; 21+ messages in thread
From: Leon Romanovsky @ 2020-06-28 9:55 UTC (permalink / raw)
To: Yishai Hadas
Cc: Doug Ledford, Jason Gunthorpe, Lijun Ou, linux-rdma,
Potnuri Bharat Teja, Weihang Li, Wei Hu(Xavier),
Yishai Hadas, Maor Gottlieb
On Sun, Jun 28, 2020 at 12:18:38PM +0300, Yishai Hadas wrote:
> On 6/24/2020 1:54 PM, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > Move allocation and destruction of memory windows under ib_core
> > responsibility and clean drivers to ensure that no updates to MW
> > ib_core structures are done in driver layer.
> >
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > ---
<...>
> > +void mlx5_ib_dealloc_mw(struct ib_mw *mw)
> > {
> > struct mlx5_ib_dev *dev = to_mdev(mw->device);
> > struct mlx5_ib_mw *mmw = to_mmw(mw);
> > - int err;
> > if (IS_ENABLED(CONFIG_INFINIBAND_ON_DEMAND_PAGING)) {
> > xa_erase(&dev->odp_mkeys, mlx5_base_mkey(mmw->mmkey.key));
> > @@ -2073,11 +2067,7 @@ int mlx5_ib_dealloc_mw(struct ib_mw *mw)
> > synchronize_srcu(&dev->odp_srcu);
> > }
> > - err = mlx5_core_destroy_mkey(dev->mdev, &mmw->mmkey);
> > - if (err)
> > - return err;
> > - kfree(mmw);
> > - return 0;
> > + mlx5_core_destroy_mkey(dev->mdev, &mmw->mmkey);
>
> Are we fully sure that this can never be triggered by user space to fail as
> of the property of the MW that can be binded with bypassing kernel ? the new
> code just ignored the err.
Why is it different from any other HW object?
The failure to destroy will leave leaked kernel resources.
We already removed this mmkey from xarray above and if we don't finish,
the MW will be leaked.
Thanks
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH rdma-next 3/5] RDMA: Move XRCD to be under ib_core responsibility
2020-06-24 10:54 [PATCH rdma-next 0/5] ib_core allocation patches Leon Romanovsky
2020-06-24 10:54 ` [PATCH rdma-next 1/5] RDMA/core: Create and destroy counters in the ib_core Leon Romanovsky
2020-06-24 10:54 ` [PATCH rdma-next 2/5] RDMA: Clean MW allocation and free flows Leon Romanovsky
@ 2020-06-24 10:54 ` Leon Romanovsky
2020-06-24 10:54 ` [PATCH rdma-next 4/5] RDMA/core: Delete not-used create RWQ table function Leon Romanovsky
2020-06-24 10:54 ` [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme Leon Romanovsky
4 siblings, 0 replies; 21+ messages in thread
From: Leon Romanovsky @ 2020-06-24 10:54 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe; +Cc: Leon Romanovsky, linux-rdma, Yishai Hadas
From: Leon Romanovsky <leonro@mellanox.com>
Update the code to allocate and free ib_xrcd structure in the
ib_core instead of inside drivers.
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/device.c | 1 +
drivers/infiniband/core/verbs.c | 30 ++++++++++++++--------
drivers/infiniband/hw/mlx4/main.c | 37 +++++++++++-----------------
drivers/infiniband/hw/mlx5/main.c | 2 ++
drivers/infiniband/hw/mlx5/mlx5_ib.h | 5 ++--
drivers/infiniband/hw/mlx5/qp.c | 34 +++++++------------------
include/rdma/ib_verbs.h | 6 ++---
7 files changed, 52 insertions(+), 63 deletions(-)
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 257e8a667977..b15fa3fa09ac 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2688,6 +2688,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
SET_OBJ_SIZE(dev_ops, ib_pd);
SET_OBJ_SIZE(dev_ops, ib_srq);
SET_OBJ_SIZE(dev_ops, ib_ucontext);
+ SET_OBJ_SIZE(dev_ops, ib_xrcd);
}
EXPORT_SYMBOL(ib_set_device_ops);
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 61debe056912..6a2becd624c3 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -2294,20 +2294,28 @@ struct ib_xrcd *ib_alloc_xrcd_user(struct ib_device *device,
struct inode *inode, struct ib_udata *udata)
{
struct ib_xrcd *xrcd;
+ int ret;
if (!device->ops.alloc_xrcd)
return ERR_PTR(-EOPNOTSUPP);
- xrcd = device->ops.alloc_xrcd(device, udata);
- if (!IS_ERR(xrcd)) {
- xrcd->device = device;
- xrcd->inode = inode;
- atomic_set(&xrcd->usecnt, 0);
- init_rwsem(&xrcd->tgt_qps_rwsem);
- xa_init(&xrcd->tgt_qps);
- }
+ xrcd = rdma_zalloc_drv_obj(device, ib_xrcd);
+ if (!xrcd)
+ return ERR_PTR(-ENOMEM);
+ xrcd->device = device;
+ xrcd->inode = inode;
+ atomic_set(&xrcd->usecnt, 0);
+ init_rwsem(&xrcd->tgt_qps_rwsem);
+ xa_init(&xrcd->tgt_qps);
+
+ ret = device->ops.alloc_xrcd(xrcd, udata);
+ if (ret)
+ goto err;
return xrcd;
+err:
+ kfree(xrcd);
+ return ERR_PTR(ret);
}
EXPORT_SYMBOL(ib_alloc_xrcd_user);
@@ -2317,8 +2325,10 @@ int ib_dealloc_xrcd_user(struct ib_xrcd *xrcd, struct ib_udata *udata)
return -EBUSY;
WARN_ON(!xa_empty(&xrcd->tgt_qps));
- return xrcd->device->ops.dealloc_xrcd(xrcd, udata);
-}
+ xrcd->device->ops.dealloc_xrcd(xrcd, udata);
+ kfree(xrcd);
+ return 0;
+ }
EXPORT_SYMBOL(ib_dealloc_xrcd_user);
/**
diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 6471f47bd365..0ad584a3b8d6 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -1219,56 +1219,47 @@ static void mlx4_ib_dealloc_pd(struct ib_pd *pd, struct ib_udata *udata)
mlx4_pd_free(to_mdev(pd->device)->dev, to_mpd(pd)->pdn);
}
-static struct ib_xrcd *mlx4_ib_alloc_xrcd(struct ib_device *ibdev,
- struct ib_udata *udata)
+static int mlx4_ib_alloc_xrcd(struct ib_xrcd *ibxrcd, struct ib_udata *udata)
{
- struct mlx4_ib_xrcd *xrcd;
+ struct mlx4_ib_dev *dev = to_mdev(ibxrcd->device);
+ struct mlx4_ib_xrcd *xrcd = to_mxrcd(ibxrcd);
struct ib_cq_init_attr cq_attr = {};
int err;
- if (!(to_mdev(ibdev)->dev->caps.flags & MLX4_DEV_CAP_FLAG_XRC))
- return ERR_PTR(-ENOSYS);
-
- xrcd = kmalloc(sizeof *xrcd, GFP_KERNEL);
- if (!xrcd)
- return ERR_PTR(-ENOMEM);
+ if (!(dev->dev->caps.flags & MLX4_DEV_CAP_FLAG_XRC))
+ return -EOPNOTSUPP;
- err = mlx4_xrcd_alloc(to_mdev(ibdev)->dev, &xrcd->xrcdn);
+ err = mlx4_xrcd_alloc(dev->dev, &xrcd->xrcdn);
if (err)
- goto err1;
+ return err;
- xrcd->pd = ib_alloc_pd(ibdev, 0);
+ xrcd->pd = ib_alloc_pd(ibxrcd->device, 0);
if (IS_ERR(xrcd->pd)) {
err = PTR_ERR(xrcd->pd);
goto err2;
}
cq_attr.cqe = 1;
- xrcd->cq = ib_create_cq(ibdev, NULL, NULL, xrcd, &cq_attr);
+ xrcd->cq = ib_create_cq(ibxrcd->device, NULL, NULL, xrcd, &cq_attr);
if (IS_ERR(xrcd->cq)) {
err = PTR_ERR(xrcd->cq);
goto err3;
}
- return &xrcd->ibxrcd;
+ return 0;
err3:
ib_dealloc_pd(xrcd->pd);
err2:
- mlx4_xrcd_free(to_mdev(ibdev)->dev, xrcd->xrcdn);
-err1:
- kfree(xrcd);
- return ERR_PTR(err);
+ mlx4_xrcd_free(dev->dev, xrcd->xrcdn);
+ return err;
}
-static int mlx4_ib_dealloc_xrcd(struct ib_xrcd *xrcd, struct ib_udata *udata)
+static void mlx4_ib_dealloc_xrcd(struct ib_xrcd *xrcd, struct ib_udata *udata)
{
ib_destroy_cq(to_mxrcd(xrcd)->cq);
ib_dealloc_pd(to_mxrcd(xrcd)->pd);
mlx4_xrcd_free(to_mdev(xrcd->device)->dev, to_mxrcd(xrcd)->xrcdn);
- kfree(xrcd);
-
- return 0;
}
static int add_gid_entry(struct ib_qp *ibqp, union ib_gid *gid)
@@ -2609,6 +2600,8 @@ static const struct ib_device_ops mlx4_ib_dev_mw_ops = {
static const struct ib_device_ops mlx4_ib_dev_xrc_ops = {
.alloc_xrcd = mlx4_ib_alloc_xrcd,
.dealloc_xrcd = mlx4_ib_dealloc_xrcd,
+
+ INIT_RDMA_OBJ_SIZE(ib_xrcd, mlx4_ib_xrcd, ibxrcd),
};
static const struct ib_device_ops mlx4_ib_dev_fs_ops = {
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 8b0b1f95af2f..a6d5c35a2845 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -6676,6 +6676,8 @@ static const struct ib_device_ops mlx5_ib_dev_mw_ops = {
static const struct ib_device_ops mlx5_ib_dev_xrc_ops = {
.alloc_xrcd = mlx5_ib_alloc_xrcd,
.dealloc_xrcd = mlx5_ib_dealloc_xrcd,
+
+ INIT_RDMA_OBJ_SIZE(ib_xrcd, mlx5_ib_xrcd, ibxrcd),
};
static const struct ib_device_ops mlx5_ib_dev_dm_ops = {
diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index d84f56517caa..726386fe4440 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -1223,9 +1223,8 @@ int mlx5_ib_process_mad(struct ib_device *ibdev, int mad_flags, u8 port_num,
const struct ib_wc *in_wc, const struct ib_grh *in_grh,
const struct ib_mad *in, struct ib_mad *out,
size_t *out_mad_size, u16 *out_mad_pkey_index);
-struct ib_xrcd *mlx5_ib_alloc_xrcd(struct ib_device *ibdev,
- struct ib_udata *udata);
-int mlx5_ib_dealloc_xrcd(struct ib_xrcd *xrcd, struct ib_udata *udata);
+int mlx5_ib_alloc_xrcd(struct ib_xrcd *xrcd, struct ib_udata *udata);
+void mlx5_ib_dealloc_xrcd(struct ib_xrcd *xrcd, struct ib_udata *udata);
int mlx5_ib_get_buf_offset(u64 addr, int page_shift, u32 *offset);
int mlx5_query_ext_port_caps(struct mlx5_ib_dev *dev, u8 port);
int mlx5_query_mad_ifc_smp_attr_node_info(struct ib_device *ibdev,
diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index f939c9b769f0..0ae73f4e28a3 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -4700,41 +4700,25 @@ int mlx5_ib_query_qp(struct ib_qp *ibqp, struct ib_qp_attr *qp_attr,
return err;
}
-struct ib_xrcd *mlx5_ib_alloc_xrcd(struct ib_device *ibdev,
- struct ib_udata *udata)
+int mlx5_ib_alloc_xrcd(struct ib_xrcd *ibxrcd, struct ib_udata *udata)
{
- struct mlx5_ib_dev *dev = to_mdev(ibdev);
- struct mlx5_ib_xrcd *xrcd;
- int err;
+ struct mlx5_ib_dev *dev = to_mdev(ibxrcd->device);
+ struct mlx5_ib_xrcd *xrcd = to_mxrcd(ibxrcd);
+ int ret;
if (!MLX5_CAP_GEN(dev->mdev, xrc))
- return ERR_PTR(-ENOSYS);
-
- xrcd = kmalloc(sizeof(*xrcd), GFP_KERNEL);
- if (!xrcd)
- return ERR_PTR(-ENOMEM);
-
- err = mlx5_cmd_xrcd_alloc(dev->mdev, &xrcd->xrcdn, 0);
- if (err) {
- kfree(xrcd);
- return ERR_PTR(-ENOMEM);
- }
+ return -EOPNOTSUPP;
- return &xrcd->ibxrcd;
+ ret = mlx5_cmd_xrcd_alloc(dev->mdev, &xrcd->xrcdn, 0);
+ return ret;
}
-int mlx5_ib_dealloc_xrcd(struct ib_xrcd *xrcd, struct ib_udata *udata)
+void mlx5_ib_dealloc_xrcd(struct ib_xrcd *xrcd, struct ib_udata *udata)
{
struct mlx5_ib_dev *dev = to_mdev(xrcd->device);
u32 xrcdn = to_mxrcd(xrcd)->xrcdn;
- int err;
- err = mlx5_cmd_xrcd_dealloc(dev->mdev, xrcdn, 0);
- if (err)
- mlx5_ib_warn(dev, "failed to dealloc xrcdn 0x%x\n", xrcdn);
-
- kfree(xrcd);
- return 0;
+ mlx5_cmd_xrcd_dealloc(dev->mdev, xrcdn, 0);
}
static void mlx5_ib_wq_event(struct mlx5_core_qp *core_qp, int type)
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 7160662899bb..f1a5c89466aa 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2494,9 +2494,8 @@ struct ib_device_ops {
void (*dealloc_mw)(struct ib_mw *mw);
int (*attach_mcast)(struct ib_qp *qp, union ib_gid *gid, u16 lid);
int (*detach_mcast)(struct ib_qp *qp, union ib_gid *gid, u16 lid);
- struct ib_xrcd *(*alloc_xrcd)(struct ib_device *device,
- struct ib_udata *udata);
- int (*dealloc_xrcd)(struct ib_xrcd *xrcd, struct ib_udata *udata);
+ int (*alloc_xrcd)(struct ib_xrcd *xrcd, struct ib_udata *udata);
+ void (*dealloc_xrcd)(struct ib_xrcd *xrcd, struct ib_udata *udata);
struct ib_flow *(*create_flow)(struct ib_qp *qp,
struct ib_flow_attr *flow_attr,
int domain, struct ib_udata *udata);
@@ -2656,6 +2655,7 @@ struct ib_device_ops {
DECLARE_RDMA_OBJ_SIZE(ib_pd);
DECLARE_RDMA_OBJ_SIZE(ib_srq);
DECLARE_RDMA_OBJ_SIZE(ib_ucontext);
+ DECLARE_RDMA_OBJ_SIZE(ib_xrcd);
};
struct ib_core_device {
--
2.26.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH rdma-next 4/5] RDMA/core: Delete not-used create RWQ table function
2020-06-24 10:54 [PATCH rdma-next 0/5] ib_core allocation patches Leon Romanovsky
` (2 preceding siblings ...)
2020-06-24 10:54 ` [PATCH rdma-next 3/5] RDMA: Move XRCD to be under ib_core responsibility Leon Romanovsky
@ 2020-06-24 10:54 ` Leon Romanovsky
2020-06-24 19:46 ` Jason Gunthorpe
2020-06-24 10:54 ` [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme Leon Romanovsky
4 siblings, 1 reply; 21+ messages in thread
From: Leon Romanovsky @ 2020-06-24 10:54 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe; +Cc: Leon Romanovsky, linux-rdma
From: Leon Romanovsky <leonro@mellanox.com>
The RWQ table is used for RSS uverbs and not in used for the kernel
consumers, delete ib_create_rwq_ind_table() routine that is not
called at all.
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/verbs.c | 39 ---------------------------------
include/rdma/ib_verbs.h | 3 ---
2 files changed, 42 deletions(-)
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 6a2becd624c3..65c9118a931c 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -2412,45 +2412,6 @@ int ib_modify_wq(struct ib_wq *wq, struct ib_wq_attr *wq_attr,
}
EXPORT_SYMBOL(ib_modify_wq);
-/*
- * ib_create_rwq_ind_table - Creates a RQ Indirection Table.
- * @device: The device on which to create the rwq indirection table.
- * @ib_rwq_ind_table_init_attr: A list of initial attributes required to
- * create the Indirection Table.
- *
- * Note: The life time of ib_rwq_ind_table_init_attr->ind_tbl is not less
- * than the created ib_rwq_ind_table object and the caller is responsible
- * for its memory allocation/free.
- */
-struct ib_rwq_ind_table *ib_create_rwq_ind_table(struct ib_device *device,
- struct ib_rwq_ind_table_init_attr *init_attr)
-{
- struct ib_rwq_ind_table *rwq_ind_table;
- int i;
- u32 table_size;
-
- if (!device->ops.create_rwq_ind_table)
- return ERR_PTR(-EOPNOTSUPP);
-
- table_size = (1 << init_attr->log_ind_tbl_size);
- rwq_ind_table = device->ops.create_rwq_ind_table(device,
- init_attr, NULL);
- if (IS_ERR(rwq_ind_table))
- return rwq_ind_table;
-
- rwq_ind_table->ind_tbl = init_attr->ind_tbl;
- rwq_ind_table->log_ind_tbl_size = init_attr->log_ind_tbl_size;
- rwq_ind_table->device = device;
- rwq_ind_table->uobject = NULL;
- atomic_set(&rwq_ind_table->usecnt, 0);
-
- for (i = 0; i < table_size; i++)
- atomic_inc(&rwq_ind_table->ind_tbl[i]->usecnt);
-
- return rwq_ind_table;
-}
-EXPORT_SYMBOL(ib_create_rwq_ind_table);
-
/*
* ib_destroy_rwq_ind_table - Destroys the specified Indirection Table.
* @wq_ind_table: The Indirection Table to destroy.
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index f1a5c89466aa..6e2cb69fe90b 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -4430,9 +4430,6 @@ struct ib_wq *ib_create_wq(struct ib_pd *pd,
int ib_destroy_wq(struct ib_wq *wq, struct ib_udata *udata);
int ib_modify_wq(struct ib_wq *wq, struct ib_wq_attr *attr,
u32 wq_attr_mask);
-struct ib_rwq_ind_table *ib_create_rwq_ind_table(struct ib_device *device,
- struct ib_rwq_ind_table_init_attr*
- wq_ind_table_init_attr);
int ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *wq_ind_table);
int ib_map_mr_sg(struct ib_mr *mr, struct scatterlist *sg, int sg_nents,
--
2.26.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 4/5] RDMA/core: Delete not-used create RWQ table function
2020-06-24 10:54 ` [PATCH rdma-next 4/5] RDMA/core: Delete not-used create RWQ table function Leon Romanovsky
@ 2020-06-24 19:46 ` Jason Gunthorpe
0 siblings, 0 replies; 21+ messages in thread
From: Jason Gunthorpe @ 2020-06-24 19:46 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Doug Ledford, Leon Romanovsky, linux-rdma
On Wed, Jun 24, 2020 at 01:54:21PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> The RWQ table is used for RSS uverbs and not in used for the kernel
> consumers, delete ib_create_rwq_ind_table() routine that is not
> called at all.
>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
> drivers/infiniband/core/verbs.c | 39 ---------------------------------
> include/rdma/ib_verbs.h | 3 ---
> 2 files changed, 42 deletions(-)
Applied to for-next thanks
Jason
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme
2020-06-24 10:54 [PATCH rdma-next 0/5] ib_core allocation patches Leon Romanovsky
` (3 preceding siblings ...)
2020-06-24 10:54 ` [PATCH rdma-next 4/5] RDMA/core: Delete not-used create RWQ table function Leon Romanovsky
@ 2020-06-24 10:54 ` Leon Romanovsky
2020-06-28 9:11 ` Yishai Hadas
2020-06-29 15:39 ` Jason Gunthorpe
4 siblings, 2 replies; 21+ messages in thread
From: Leon Romanovsky @ 2020-06-24 10:54 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe; +Cc: Leon Romanovsky, linux-rdma, Yishai Hadas
From: Leon Romanovsky <leonro@mellanox.com>
Move struct ib_rwq_ind_table allocation to ib_core.
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/device.c | 1 +
drivers/infiniband/core/uverbs_cmd.c | 37 ++++++++++++-------
drivers/infiniband/core/uverbs_std_types.c | 13 ++++++-
drivers/infiniband/core/verbs.c | 25 +------------
drivers/infiniband/hw/mlx4/main.c | 4 +-
drivers/infiniband/hw/mlx4/mlx4_ib.h | 12 +++---
drivers/infiniband/hw/mlx4/qp.c | 40 ++++++--------------
drivers/infiniband/hw/mlx5/main.c | 3 ++
drivers/infiniband/hw/mlx5/mlx5_ib.h | 8 ++--
drivers/infiniband/hw/mlx5/qp.c | 43 +++++++++-------------
include/rdma/ib_verbs.h | 11 +++---
11 files changed, 86 insertions(+), 111 deletions(-)
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index b15fa3fa09ac..85d921c8e2b5 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2686,6 +2686,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
SET_OBJ_SIZE(dev_ops, ib_cq);
SET_OBJ_SIZE(dev_ops, ib_mw);
SET_OBJ_SIZE(dev_ops, ib_pd);
+ SET_OBJ_SIZE(dev_ops, ib_rwq_ind_table);
SET_OBJ_SIZE(dev_ops, ib_srq);
SET_OBJ_SIZE(dev_ops, ib_ucontext);
SET_OBJ_SIZE(dev_ops, ib_xrcd);
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index a5301f3d3059..a83d11d1e3ee 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -3090,13 +3090,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
{
struct ib_uverbs_ex_create_rwq_ind_table cmd;
struct ib_uverbs_ex_create_rwq_ind_table_resp resp = {};
- struct ib_uobject *uobj;
+ struct ib_uobject *uobj;
int err;
struct ib_rwq_ind_table_init_attr init_attr = {};
struct ib_rwq_ind_table *rwq_ind_tbl;
- struct ib_wq **wqs = NULL;
+ struct ib_wq **wqs = NULL;
u32 *wqs_handles = NULL;
- struct ib_wq *wq = NULL;
+ struct ib_wq *wq = NULL;
int i, j, num_read_wqs;
u32 num_wq_handles;
struct uverbs_req_iter iter;
@@ -3151,17 +3151,15 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
goto put_wqs;
}
- init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
- init_attr.ind_tbl = wqs;
-
- rwq_ind_tbl = ib_dev->ops.create_rwq_ind_table(ib_dev, &init_attr,
- &attrs->driver_udata);
-
- if (IS_ERR(rwq_ind_tbl)) {
- err = PTR_ERR(rwq_ind_tbl);
+ rwq_ind_tbl = rdma_zalloc_drv_obj(ib_dev, ib_rwq_ind_table);
+ if (!rwq_ind_tbl) {
+ err = -ENOMEM;
goto err_uobj;
}
+ init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
+ init_attr.ind_tbl = wqs;
+
rwq_ind_tbl->ind_tbl = wqs;
rwq_ind_tbl->log_ind_tbl_size = init_attr.log_ind_tbl_size;
rwq_ind_tbl->uobject = uobj;
@@ -3169,6 +3167,12 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
rwq_ind_tbl->device = ib_dev;
atomic_set(&rwq_ind_tbl->usecnt, 0);
+ err = ib_dev->ops.create_rwq_ind_table(rwq_ind_tbl, &init_attr,
+ &rwq_ind_tbl->ind_tbl_num,
+ &attrs->driver_udata);
+ if (err)
+ goto err_create;
+
for (i = 0; i < num_wq_handles; i++)
atomic_inc(&wqs[i]->usecnt);
@@ -3190,7 +3194,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
return 0;
err_copy:
- ib_destroy_rwq_ind_table(rwq_ind_tbl);
+ for (i = 0; i < num_wq_handles; i++)
+ atomic_dec(&wqs[i]->usecnt);
+
+ if (ib_dev->ops.destroy_rwq_ind_table)
+ ib_dev->ops.destroy_rwq_ind_table(rwq_ind_tbl);
+err_create:
+ kfree(rwq_ind_tbl);
err_uobj:
uobj_alloc_abort(uobj, attrs);
put_wqs:
@@ -4018,8 +4028,7 @@ const struct uapi_definition uverbs_def_write_intf[] = {
IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
ib_uverbs_ex_destroy_rwq_ind_table,
UAPI_DEF_WRITE_I(
- struct ib_uverbs_ex_destroy_rwq_ind_table),
- UAPI_DEF_METHOD_NEEDS_FN(destroy_rwq_ind_table))),
+ struct ib_uverbs_ex_destroy_rwq_ind_table))),
DECLARE_UVERBS_OBJECT(
UVERBS_OBJECT_WQ,
diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
index e4994cc4cc51..cd82a5a80bdf 100644
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -82,12 +82,21 @@ static int uverbs_free_rwq_ind_tbl(struct ib_uobject *uobject,
{
struct ib_rwq_ind_table *rwq_ind_tbl = uobject->object;
struct ib_wq **ind_tbl = rwq_ind_tbl->ind_tbl;
- int ret;
+ u32 table_size = (1 << rwq_ind_tbl->log_ind_tbl_size);
+ int ret = 0, i;
+
+ if (atomic_read(&rwq_ind_tbl->usecnt))
+ ret = -EBUSY;
- ret = ib_destroy_rwq_ind_table(rwq_ind_tbl);
if (ib_is_destroy_retryable(ret, why, uobject))
return ret;
+ for (i = 0; i < table_size; i++)
+ atomic_dec(&ind_tbl[i]->usecnt);
+
+ if (rwq_ind_tbl->device->ops.destroy_rwq_ind_table)
+ rwq_ind_tbl->device->ops.destroy_rwq_ind_table(rwq_ind_tbl);
+ kfree(rwq_ind_tbl);
kfree(ind_tbl);
return ret;
}
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 65c9118a931c..4210f0842bc6 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1703,7 +1703,7 @@ static int _ib_modify_qp(struct ib_qp *qp, struct ib_qp_attr *attr,
&old_sgid_attr_alt_av);
if (ret)
goto out_av;
-
+//
/*
* Today the core code can only handle alternate paths and APM
* for IB. Ban them in roce mode.
@@ -2412,29 +2412,6 @@ int ib_modify_wq(struct ib_wq *wq, struct ib_wq_attr *wq_attr,
}
EXPORT_SYMBOL(ib_modify_wq);
-/*
- * ib_destroy_rwq_ind_table - Destroys the specified Indirection Table.
- * @wq_ind_table: The Indirection Table to destroy.
-*/
-int ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *rwq_ind_table)
-{
- int err, i;
- u32 table_size = (1 << rwq_ind_table->log_ind_tbl_size);
- struct ib_wq **ind_tbl = rwq_ind_table->ind_tbl;
-
- if (atomic_read(&rwq_ind_table->usecnt))
- return -EBUSY;
-
- err = rwq_ind_table->device->ops.destroy_rwq_ind_table(rwq_ind_table);
- if (!err) {
- for (i = 0; i < table_size; i++)
- atomic_dec(&ind_tbl[i]->usecnt);
- }
-
- return err;
-}
-EXPORT_SYMBOL(ib_destroy_rwq_ind_table);
-
int ib_check_mr_status(struct ib_mr *mr, u32 check_mask,
struct ib_mr_status *mr_status)
{
diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 0ad584a3b8d6..88a3a9a88bee 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -2585,9 +2585,11 @@ static const struct ib_device_ops mlx4_ib_dev_ops = {
static const struct ib_device_ops mlx4_ib_dev_wq_ops = {
.create_rwq_ind_table = mlx4_ib_create_rwq_ind_table,
.create_wq = mlx4_ib_create_wq,
- .destroy_rwq_ind_table = mlx4_ib_destroy_rwq_ind_table,
.destroy_wq = mlx4_ib_destroy_wq,
.modify_wq = mlx4_ib_modify_wq,
+
+ INIT_RDMA_OBJ_SIZE(ib_rwq_ind_table, mlx4_ib_rwq_ind_table,
+ ib_rwq_ind_tbl),
};
static const struct ib_device_ops mlx4_ib_dev_mw_ops = {
diff --git a/drivers/infiniband/hw/mlx4/mlx4_ib.h b/drivers/infiniband/hw/mlx4/mlx4_ib.h
index 5fbe766aef6b..2730a0f65f47 100644
--- a/drivers/infiniband/hw/mlx4/mlx4_ib.h
+++ b/drivers/infiniband/hw/mlx4/mlx4_ib.h
@@ -366,6 +366,10 @@ struct mlx4_ib_ah {
union mlx4_ext_av av;
};
+struct mlx4_ib_rwq_ind_table {
+ struct ib_rwq_ind_table ib_rwq_ind_tbl;
+};
+
/****************************************/
/* alias guid support */
/****************************************/
@@ -893,11 +897,9 @@ void mlx4_ib_destroy_wq(struct ib_wq *wq, struct ib_udata *udata);
int mlx4_ib_modify_wq(struct ib_wq *wq, struct ib_wq_attr *wq_attr,
u32 wq_attr_mask, struct ib_udata *udata);
-struct ib_rwq_ind_table
-*mlx4_ib_create_rwq_ind_table(struct ib_device *device,
- struct ib_rwq_ind_table_init_attr *init_attr,
- struct ib_udata *udata);
-int mlx4_ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *wq_ind_table);
+int mlx4_ib_create_rwq_ind_table(struct ib_rwq_ind_table *ib_rwq_ind_tbl,
+ struct ib_rwq_ind_table_init_attr *init_attr,
+ u32 *ind_tbl_num, struct ib_udata *udata);
int mlx4_ib_umem_calc_optimal_mtt_size(struct ib_umem *umem, u64 start_va,
int *num_of_mtts);
diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
index cf51e3cbd969..5795bfc1a512 100644
--- a/drivers/infiniband/hw/mlx4/qp.c
+++ b/drivers/infiniband/hw/mlx4/qp.c
@@ -4340,34 +4340,32 @@ void mlx4_ib_destroy_wq(struct ib_wq *ibwq, struct ib_udata *udata)
kfree(qp);
}
-struct ib_rwq_ind_table
-*mlx4_ib_create_rwq_ind_table(struct ib_device *device,
- struct ib_rwq_ind_table_init_attr *init_attr,
- struct ib_udata *udata)
+int mlx4_ib_create_rwq_ind_table(struct ib_rwq_ind_table *rwq_ind_table,
+ struct ib_rwq_ind_table_init_attr *init_attr,
+ u32 *ind_tbl_num, struct ib_udata *udata)
{
- struct ib_rwq_ind_table *rwq_ind_table;
struct mlx4_ib_create_rwq_ind_tbl_resp resp = {};
unsigned int ind_tbl_size = 1 << init_attr->log_ind_tbl_size;
+ struct ib_device *device = rwq_ind_table->device;
unsigned int base_wqn;
size_t min_resp_len;
- int i;
- int err;
+ int i, err = 0;
if (udata->inlen > 0 &&
!ib_is_udata_cleared(udata, 0,
udata->inlen))
- return ERR_PTR(-EOPNOTSUPP);
+ return -EOPNOTSUPP;
min_resp_len = offsetof(typeof(resp), reserved) + sizeof(resp.reserved);
if (udata->outlen && udata->outlen < min_resp_len)
- return ERR_PTR(-EINVAL);
+ return -EINVAL;
if (ind_tbl_size >
device->attrs.rss_caps.max_rwq_indirection_table_size) {
pr_debug("log_ind_tbl_size = %d is bigger than supported = %d\n",
ind_tbl_size,
device->attrs.rss_caps.max_rwq_indirection_table_size);
- return ERR_PTR(-EINVAL);
+ return -EINVAL;
}
base_wqn = init_attr->ind_tbl[0]->wq_num;
@@ -4375,39 +4373,23 @@ struct ib_rwq_ind_table
if (base_wqn % ind_tbl_size) {
pr_debug("WQN=0x%x isn't aligned with indirection table size\n",
base_wqn);
- return ERR_PTR(-EINVAL);
+ return -EINVAL;
}
for (i = 1; i < ind_tbl_size; i++) {
if (++base_wqn != init_attr->ind_tbl[i]->wq_num) {
pr_debug("indirection table's WQNs aren't consecutive\n");
- return ERR_PTR(-EINVAL);
+ return -EINVAL;
}
}
- rwq_ind_table = kzalloc(sizeof(*rwq_ind_table), GFP_KERNEL);
- if (!rwq_ind_table)
- return ERR_PTR(-ENOMEM);
-
if (udata->outlen) {
resp.response_length = offsetof(typeof(resp), response_length) +
sizeof(resp.response_length);
err = ib_copy_to_udata(udata, &resp, resp.response_length);
- if (err)
- goto err;
}
- return rwq_ind_table;
-
-err:
- kfree(rwq_ind_table);
- return ERR_PTR(err);
-}
-
-int mlx4_ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *ib_rwq_ind_tbl)
-{
- kfree(ib_rwq_ind_tbl);
- return 0;
+ return err;
}
struct mlx4_ib_drain_cqe {
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index a6d5c35a2845..682ba1b6f34a 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -6844,6 +6844,9 @@ static const struct ib_device_ops mlx5_ib_dev_common_roce_ops = {
.destroy_wq = mlx5_ib_destroy_wq,
.get_netdev = mlx5_ib_get_netdev,
.modify_wq = mlx5_ib_modify_wq,
+
+ INIT_RDMA_OBJ_SIZE(ib_rwq_ind_table, mlx5_ib_rwq_ind_table,
+ ib_rwq_ind_tbl),
};
static int mlx5_ib_stage_common_roce_init(struct mlx5_ib_dev *dev)
diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index 726386fe4440..c4a285e1950d 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -1272,10 +1272,10 @@ struct ib_wq *mlx5_ib_create_wq(struct ib_pd *pd,
void mlx5_ib_destroy_wq(struct ib_wq *wq, struct ib_udata *udata);
int mlx5_ib_modify_wq(struct ib_wq *wq, struct ib_wq_attr *wq_attr,
u32 wq_attr_mask, struct ib_udata *udata);
-struct ib_rwq_ind_table *mlx5_ib_create_rwq_ind_table(struct ib_device *device,
- struct ib_rwq_ind_table_init_attr *init_attr,
- struct ib_udata *udata);
-int mlx5_ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *wq_ind_table);
+int mlx5_ib_create_rwq_ind_table(struct ib_rwq_ind_table *ib_rwq_ind_table,
+ struct ib_rwq_ind_table_init_attr *init_attr,
+ u32 *ind_tbl_num, struct ib_udata *udata);
+void mlx5_ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *wq_ind_table);
struct ib_dm *mlx5_ib_alloc_dm(struct ib_device *ibdev,
struct ib_ucontext *context,
struct ib_dm_alloc_attr *attr,
diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index 0ae73f4e28a3..3124c80169fb 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -5063,12 +5063,13 @@ void mlx5_ib_destroy_wq(struct ib_wq *wq, struct ib_udata *udata)
kfree(rwq);
}
-struct ib_rwq_ind_table *mlx5_ib_create_rwq_ind_table(struct ib_device *device,
- struct ib_rwq_ind_table_init_attr *init_attr,
- struct ib_udata *udata)
+int mlx5_ib_create_rwq_ind_table(struct ib_rwq_ind_table *ib_rwq_ind_table,
+ struct ib_rwq_ind_table_init_attr *init_attr,
+ u32 *ind_tbl_num, struct ib_udata *udata)
{
- struct mlx5_ib_dev *dev = to_mdev(device);
- struct mlx5_ib_rwq_ind_table *rwq_ind_tbl;
+ struct mlx5_ib_rwq_ind_table *rwq_ind_tbl =
+ to_mrwq_ind_table(ib_rwq_ind_table);
+ struct mlx5_ib_dev *dev = to_mdev(ib_rwq_ind_table->device);
int sz = 1 << init_attr->log_ind_tbl_size;
struct mlx5_ib_create_rwq_ind_tbl_resp resp = {};
size_t min_resp_len;
@@ -5081,30 +5082,24 @@ struct ib_rwq_ind_table *mlx5_ib_create_rwq_ind_table(struct ib_device *device,
if (udata->inlen > 0 &&
!ib_is_udata_cleared(udata, 0,
udata->inlen))
- return ERR_PTR(-EOPNOTSUPP);
+ return -EOPNOTSUPP;
if (init_attr->log_ind_tbl_size >
MLX5_CAP_GEN(dev->mdev, log_max_rqt_size)) {
mlx5_ib_dbg(dev, "log_ind_tbl_size = %d is bigger than supported = %d\n",
init_attr->log_ind_tbl_size,
MLX5_CAP_GEN(dev->mdev, log_max_rqt_size));
- return ERR_PTR(-EINVAL);
+ return -EINVAL;
}
min_resp_len = offsetof(typeof(resp), reserved) + sizeof(resp.reserved);
if (udata->outlen && udata->outlen < min_resp_len)
- return ERR_PTR(-EINVAL);
-
- rwq_ind_tbl = kzalloc(sizeof(*rwq_ind_tbl), GFP_KERNEL);
- if (!rwq_ind_tbl)
- return ERR_PTR(-ENOMEM);
+ return -EINVAL;
inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + sizeof(u32) * sz;
in = kvzalloc(inlen, GFP_KERNEL);
- if (!in) {
- err = -ENOMEM;
- goto err;
- }
+ if (!in)
+ return -ENOMEM;
rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
@@ -5118,12 +5113,10 @@ struct ib_rwq_ind_table *mlx5_ib_create_rwq_ind_table(struct ib_device *device,
MLX5_SET(create_rqt_in, in, uid, rwq_ind_tbl->uid);
err = mlx5_core_create_rqt(dev->mdev, in, inlen, &rwq_ind_tbl->rqtn);
- kvfree(in);
-
if (err)
goto err;
- rwq_ind_tbl->ib_rwq_ind_tbl.ind_tbl_num = rwq_ind_tbl->rqtn;
+ *ind_tbl_num = rwq_ind_tbl->rqtn;
if (udata->outlen) {
resp.response_length = offsetof(typeof(resp), response_length) +
sizeof(resp.response_length);
@@ -5132,24 +5125,22 @@ struct ib_rwq_ind_table *mlx5_ib_create_rwq_ind_table(struct ib_device *device,
goto err_copy;
}
- return &rwq_ind_tbl->ib_rwq_ind_tbl;
+ kvfree(in);
+ return 0;
err_copy:
mlx5_cmd_destroy_rqt(dev->mdev, rwq_ind_tbl->rqtn, rwq_ind_tbl->uid);
err:
- kfree(rwq_ind_tbl);
- return ERR_PTR(err);
+ kvfree(in);
+ return err;
}
-int mlx5_ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *ib_rwq_ind_tbl)
+void mlx5_ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *ib_rwq_ind_tbl)
{
struct mlx5_ib_rwq_ind_table *rwq_ind_tbl = to_mrwq_ind_table(ib_rwq_ind_tbl);
struct mlx5_ib_dev *dev = to_mdev(ib_rwq_ind_tbl->device);
mlx5_cmd_destroy_rqt(dev->mdev, rwq_ind_tbl->rqtn, rwq_ind_tbl->uid);
-
- kfree(rwq_ind_tbl);
- return 0;
}
int mlx5_ib_modify_wq(struct ib_wq *wq, struct ib_wq_attr *wq_attr,
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 6e2cb69fe90b..ed948946e443 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2526,11 +2526,10 @@ struct ib_device_ops {
void (*destroy_wq)(struct ib_wq *wq, struct ib_udata *udata);
int (*modify_wq)(struct ib_wq *wq, struct ib_wq_attr *attr,
u32 wq_attr_mask, struct ib_udata *udata);
- struct ib_rwq_ind_table *(*create_rwq_ind_table)(
- struct ib_device *device,
- struct ib_rwq_ind_table_init_attr *init_attr,
- struct ib_udata *udata);
- int (*destroy_rwq_ind_table)(struct ib_rwq_ind_table *wq_ind_table);
+ int (*create_rwq_ind_table)(struct ib_rwq_ind_table *ib_rwq_ind_table,
+ struct ib_rwq_ind_table_init_attr *init_attr,
+ u32 *ind_tbl_num, struct ib_udata *udata);
+ void (*destroy_rwq_ind_table)(struct ib_rwq_ind_table *wq_ind_table);
struct ib_dm *(*alloc_dm)(struct ib_device *device,
struct ib_ucontext *context,
struct ib_dm_alloc_attr *attr,
@@ -2653,6 +2652,7 @@ struct ib_device_ops {
DECLARE_RDMA_OBJ_SIZE(ib_cq);
DECLARE_RDMA_OBJ_SIZE(ib_mw);
DECLARE_RDMA_OBJ_SIZE(ib_pd);
+ DECLARE_RDMA_OBJ_SIZE(ib_rwq_ind_table);
DECLARE_RDMA_OBJ_SIZE(ib_srq);
DECLARE_RDMA_OBJ_SIZE(ib_ucontext);
DECLARE_RDMA_OBJ_SIZE(ib_xrcd);
@@ -4430,7 +4430,6 @@ struct ib_wq *ib_create_wq(struct ib_pd *pd,
int ib_destroy_wq(struct ib_wq *wq, struct ib_udata *udata);
int ib_modify_wq(struct ib_wq *wq, struct ib_wq_attr *attr,
u32 wq_attr_mask);
-int ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *wq_ind_table);
int ib_map_mr_sg(struct ib_mr *mr, struct scatterlist *sg, int sg_nents,
unsigned int *sg_offset, unsigned int page_size);
--
2.26.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme
2020-06-24 10:54 ` [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme Leon Romanovsky
@ 2020-06-28 9:11 ` Yishai Hadas
2020-06-28 9:41 ` Leon Romanovsky
2020-06-29 15:39 ` Jason Gunthorpe
1 sibling, 1 reply; 21+ messages in thread
From: Yishai Hadas @ 2020-06-28 9:11 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Jason Gunthorpe, Leon Romanovsky, linux-rdma,
Yishai Hadas, Maor Gottlieb
On 6/24/2020 1:54 PM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> Move struct ib_rwq_ind_table allocation to ib_core.
>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
> drivers/infiniband/core/device.c | 1 +
> drivers/infiniband/core/uverbs_cmd.c | 37 ++++++++++++-------
> drivers/infiniband/core/uverbs_std_types.c | 13 ++++++-
> drivers/infiniband/core/verbs.c | 25 +------------
> drivers/infiniband/hw/mlx4/main.c | 4 +-
> drivers/infiniband/hw/mlx4/mlx4_ib.h | 12 +++---
> drivers/infiniband/hw/mlx4/qp.c | 40 ++++++--------------
> drivers/infiniband/hw/mlx5/main.c | 3 ++
> drivers/infiniband/hw/mlx5/mlx5_ib.h | 8 ++--
> drivers/infiniband/hw/mlx5/qp.c | 43 +++++++++-------------
> include/rdma/ib_verbs.h | 11 +++---
> 11 files changed, 86 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index b15fa3fa09ac..85d921c8e2b5 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -2686,6 +2686,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
> SET_OBJ_SIZE(dev_ops, ib_cq);
> SET_OBJ_SIZE(dev_ops, ib_mw);
> SET_OBJ_SIZE(dev_ops, ib_pd);
> + SET_OBJ_SIZE(dev_ops, ib_rwq_ind_table);
> SET_OBJ_SIZE(dev_ops, ib_srq);
> SET_OBJ_SIZE(dev_ops, ib_ucontext);
> SET_OBJ_SIZE(dev_ops, ib_xrcd);
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index a5301f3d3059..a83d11d1e3ee 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -3090,13 +3090,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> {
> struct ib_uverbs_ex_create_rwq_ind_table cmd;
> struct ib_uverbs_ex_create_rwq_ind_table_resp resp = {};
> - struct ib_uobject *uobj;
> + struct ib_uobject *uobj;
> int err;
> struct ib_rwq_ind_table_init_attr init_attr = {};
> struct ib_rwq_ind_table *rwq_ind_tbl;
> - struct ib_wq **wqs = NULL;
> + struct ib_wq **wqs = NULL;
> u32 *wqs_handles = NULL;
> - struct ib_wq *wq = NULL;
> + struct ib_wq *wq = NULL;
> int i, j, num_read_wqs;
> u32 num_wq_handles;
> struct uverbs_req_iter iter;
> @@ -3151,17 +3151,15 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> goto put_wqs;
> }
>
> - init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
> - init_attr.ind_tbl = wqs;
> -
> - rwq_ind_tbl = ib_dev->ops.create_rwq_ind_table(ib_dev, &init_attr,
> - &attrs->driver_udata);
> -
> - if (IS_ERR(rwq_ind_tbl)) {
> - err = PTR_ERR(rwq_ind_tbl);
> + rwq_ind_tbl = rdma_zalloc_drv_obj(ib_dev, ib_rwq_ind_table);
> + if (!rwq_ind_tbl) {
> + err = -ENOMEM;
> goto err_uobj;
> }
>
> + init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
> + init_attr.ind_tbl = wqs;
> +
> rwq_ind_tbl->ind_tbl = wqs;
> rwq_ind_tbl->log_ind_tbl_size = init_attr.log_ind_tbl_size;
> rwq_ind_tbl->uobject = uobj;
> @@ -3169,6 +3167,12 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> rwq_ind_tbl->device = ib_dev;
> atomic_set(&rwq_ind_tbl->usecnt, 0);
>
> + err = ib_dev->ops.create_rwq_ind_table(rwq_ind_tbl, &init_attr,
> + &rwq_ind_tbl->ind_tbl_num,
> + &attrs->driver_udata);
> + if (err)
> + goto err_create;
> +
> for (i = 0; i < num_wq_handles; i++)
> atomic_inc(&wqs[i]->usecnt);
>
> @@ -3190,7 +3194,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> return 0;
>
> err_copy:
> - ib_destroy_rwq_ind_table(rwq_ind_tbl);
> + for (i = 0; i < num_wq_handles; i++)
> + atomic_dec(&wqs[i]->usecnt);
> +
> + if (ib_dev->ops.destroy_rwq_ind_table)
> + ib_dev->ops.destroy_rwq_ind_table(rwq_ind_tbl);
> +err_create:
> + kfree(rwq_ind_tbl);
> err_uobj:
> uobj_alloc_abort(uobj, attrs);
> put_wqs:
> @@ -4018,8 +4028,7 @@ const struct uapi_definition uverbs_def_write_intf[] = {
> IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
> ib_uverbs_ex_destroy_rwq_ind_table,
> UAPI_DEF_WRITE_I(
> - struct ib_uverbs_ex_destroy_rwq_ind_table),
> - UAPI_DEF_METHOD_NEEDS_FN(destroy_rwq_ind_table))),
> + struct ib_uverbs_ex_destroy_rwq_ind_table))),
>
> DECLARE_UVERBS_OBJECT(
> UVERBS_OBJECT_WQ,
> diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
> index e4994cc4cc51..cd82a5a80bdf 100644
> --- a/drivers/infiniband/core/uverbs_std_types.c
> +++ b/drivers/infiniband/core/uverbs_std_types.c
> @@ -82,12 +82,21 @@ static int uverbs_free_rwq_ind_tbl(struct ib_uobject *uobject,
> {
> struct ib_rwq_ind_table *rwq_ind_tbl = uobject->object;
> struct ib_wq **ind_tbl = rwq_ind_tbl->ind_tbl;
> - int ret;
> + u32 table_size = (1 << rwq_ind_tbl->log_ind_tbl_size);
> + int ret = 0, i;
> +
> + if (atomic_read(&rwq_ind_tbl->usecnt))
> + ret = -EBUSY;
>
> - ret = ib_destroy_rwq_ind_table(rwq_ind_tbl);
> if (ib_is_destroy_retryable(ret, why, uobject))
> return ret;
>
> + for (i = 0; i < table_size; i++)
> + atomic_dec(&ind_tbl[i]->usecnt);
> +
Upon destroy we first expect to destroy the object that is pointing to
other objetcs and only then descraese their ref count, this was the
orignal logic here. (See also ib_destroy_qp_user(), ib_destroy_wq() and
others).
> + if (rwq_ind_tbl->device->ops.destroy_rwq_ind_table)
> + rwq_ind_tbl->device->ops.destroy_rwq_ind_table(rwq_ind_tbl);
This doesn't really make sense, if no destroy function was set, the
allocation should be prevented as well.
> + kfree(rwq_ind_tbl);
> kfree(ind_tbl);
> return ret;
> }
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index 65c9118a931c..4210f0842bc6 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -1703,7 +1703,7 @@ static int _ib_modify_qp(struct ib_qp *qp, struct ib_qp_attr *attr,
> &old_sgid_attr_alt_av);
> if (ret)
> goto out_av;
> -
> +//
> /*
> * Today the core code can only handle alternate paths and APM
> * for IB. Ban them in roce mode.
> @@ -2412,29 +2412,6 @@ int ib_modify_wq(struct ib_wq *wq, struct ib_wq_attr *wq_attr,
> }
> EXPORT_SYMBOL(ib_modify_wq);
>
> -/*
> - * ib_destroy_rwq_ind_table - Destroys the specified Indirection Table.
> - * @wq_ind_table: The Indirection Table to destroy.
> -*/
> -int ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *rwq_ind_table)
> -{
> - int err, i;
> - u32 table_size = (1 << rwq_ind_table->log_ind_tbl_size);
> - struct ib_wq **ind_tbl = rwq_ind_table->ind_tbl;
> -
> - if (atomic_read(&rwq_ind_table->usecnt))
> - return -EBUSY;
> -
> - err = rwq_ind_table->device->ops.destroy_rwq_ind_table(rwq_ind_table);
> - if (!err) {
> - for (i = 0; i < table_size; i++)
> - atomic_dec(&ind_tbl[i]->usecnt);
> - }
> -
> - return err;
> -}
> -EXPORT_SYMBOL(ib_destroy_rwq_ind_table);
> -
> int ib_check_mr_status(struct ib_mr *mr, u32 check_mask,
> struct ib_mr_status *mr_status)
> {
> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> index 0ad584a3b8d6..88a3a9a88bee 100644
> --- a/drivers/infiniband/hw/mlx4/main.c
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -2585,9 +2585,11 @@ static const struct ib_device_ops mlx4_ib_dev_ops = {
> static const struct ib_device_ops mlx4_ib_dev_wq_ops = {
> .create_rwq_ind_table = mlx4_ib_create_rwq_ind_table,
> .create_wq = mlx4_ib_create_wq,
> - .destroy_rwq_ind_table = mlx4_ib_destroy_rwq_ind_table,
> .destroy_wq = mlx4_ib_destroy_wq,
> .modify_wq = mlx4_ib_modify_wq,
> +
> + INIT_RDMA_OBJ_SIZE(ib_rwq_ind_table, mlx4_ib_rwq_ind_table,
> + ib_rwq_ind_tbl),
> };
>
> static const struct ib_device_ops mlx4_ib_dev_mw_ops = {
> diff --git a/drivers/infiniband/hw/mlx4/mlx4_ib.h b/drivers/infiniband/hw/mlx4/mlx4_ib.h
> index 5fbe766aef6b..2730a0f65f47 100644
> --- a/drivers/infiniband/hw/mlx4/mlx4_ib.h
> +++ b/drivers/infiniband/hw/mlx4/mlx4_ib.h
> @@ -366,6 +366,10 @@ struct mlx4_ib_ah {
> union mlx4_ext_av av;
> };
>
> +struct mlx4_ib_rwq_ind_table {
> + struct ib_rwq_ind_table ib_rwq_ind_tbl;
> +};
> +
> /****************************************/
> /* alias guid support */
> /****************************************/
> @@ -893,11 +897,9 @@ void mlx4_ib_destroy_wq(struct ib_wq *wq, struct ib_udata *udata);
> int mlx4_ib_modify_wq(struct ib_wq *wq, struct ib_wq_attr *wq_attr,
> u32 wq_attr_mask, struct ib_udata *udata);
>
> -struct ib_rwq_ind_table
> -*mlx4_ib_create_rwq_ind_table(struct ib_device *device,
> - struct ib_rwq_ind_table_init_attr *init_attr,
> - struct ib_udata *udata);
> -int mlx4_ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *wq_ind_table);
> +int mlx4_ib_create_rwq_ind_table(struct ib_rwq_ind_table *ib_rwq_ind_tbl,
> + struct ib_rwq_ind_table_init_attr *init_attr,
> + u32 *ind_tbl_num, struct ib_udata *udata);
> int mlx4_ib_umem_calc_optimal_mtt_size(struct ib_umem *umem, u64 start_va,
> int *num_of_mtts);
>
> diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
> index cf51e3cbd969..5795bfc1a512 100644
> --- a/drivers/infiniband/hw/mlx4/qp.c
> +++ b/drivers/infiniband/hw/mlx4/qp.c
> @@ -4340,34 +4340,32 @@ void mlx4_ib_destroy_wq(struct ib_wq *ibwq, struct ib_udata *udata)
> kfree(qp);
> }
>
> -struct ib_rwq_ind_table
> -*mlx4_ib_create_rwq_ind_table(struct ib_device *device,
> - struct ib_rwq_ind_table_init_attr *init_attr,
> - struct ib_udata *udata)
> +int mlx4_ib_create_rwq_ind_table(struct ib_rwq_ind_table *rwq_ind_table,
> + struct ib_rwq_ind_table_init_attr *init_attr,
> + u32 *ind_tbl_num, struct ib_udata *udata)
> {
> - struct ib_rwq_ind_table *rwq_ind_table;
> struct mlx4_ib_create_rwq_ind_tbl_resp resp = {};
> unsigned int ind_tbl_size = 1 << init_attr->log_ind_tbl_size;
> + struct ib_device *device = rwq_ind_table->device;
> unsigned int base_wqn;
> size_t min_resp_len;
> - int i;
> - int err;
> + int i, err = 0;
>
> if (udata->inlen > 0 &&
> !ib_is_udata_cleared(udata, 0,
> udata->inlen))
> - return ERR_PTR(-EOPNOTSUPP);
> + return -EOPNOTSUPP;
>
> min_resp_len = offsetof(typeof(resp), reserved) + sizeof(resp.reserved);
> if (udata->outlen && udata->outlen < min_resp_len)
> - return ERR_PTR(-EINVAL);
> + return -EINVAL;
>
> if (ind_tbl_size >
> device->attrs.rss_caps.max_rwq_indirection_table_size) {
> pr_debug("log_ind_tbl_size = %d is bigger than supported = %d\n",
> ind_tbl_size,
> device->attrs.rss_caps.max_rwq_indirection_table_size);
> - return ERR_PTR(-EINVAL);
> + return -EINVAL;
> }
>
> base_wqn = init_attr->ind_tbl[0]->wq_num;
> @@ -4375,39 +4373,23 @@ struct ib_rwq_ind_table
> if (base_wqn % ind_tbl_size) {
> pr_debug("WQN=0x%x isn't aligned with indirection table size\n",
> base_wqn);
> - return ERR_PTR(-EINVAL);
> + return -EINVAL;
> }
>
> for (i = 1; i < ind_tbl_size; i++) {
> if (++base_wqn != init_attr->ind_tbl[i]->wq_num) {
> pr_debug("indirection table's WQNs aren't consecutive\n");
> - return ERR_PTR(-EINVAL);
> + return -EINVAL;
> }
> }
>
> - rwq_ind_table = kzalloc(sizeof(*rwq_ind_table), GFP_KERNEL);
> - if (!rwq_ind_table)
> - return ERR_PTR(-ENOMEM);
> -
> if (udata->outlen) {
> resp.response_length = offsetof(typeof(resp), response_length) +
> sizeof(resp.response_length);
> err = ib_copy_to_udata(udata, &resp, resp.response_length);
> - if (err)
> - goto err;
> }
>
> - return rwq_ind_table;
> -
> -err:
> - kfree(rwq_ind_table);
> - return ERR_PTR(err);
> -}
> -
> -int mlx4_ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *ib_rwq_ind_tbl)
> -{
> - kfree(ib_rwq_ind_tbl);
> - return 0;
> + return err;
> }
>
> struct mlx4_ib_drain_cqe {
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index a6d5c35a2845..682ba1b6f34a 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -6844,6 +6844,9 @@ static const struct ib_device_ops mlx5_ib_dev_common_roce_ops = {
> .destroy_wq = mlx5_ib_destroy_wq,
> .get_netdev = mlx5_ib_get_netdev,
> .modify_wq = mlx5_ib_modify_wq,
> +
> + INIT_RDMA_OBJ_SIZE(ib_rwq_ind_table, mlx5_ib_rwq_ind_table,
> + ib_rwq_ind_tbl),
> };
>
> static int mlx5_ib_stage_common_roce_init(struct mlx5_ib_dev *dev)
> diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> index 726386fe4440..c4a285e1950d 100644
> --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
> +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> @@ -1272,10 +1272,10 @@ struct ib_wq *mlx5_ib_create_wq(struct ib_pd *pd,
> void mlx5_ib_destroy_wq(struct ib_wq *wq, struct ib_udata *udata);
> int mlx5_ib_modify_wq(struct ib_wq *wq, struct ib_wq_attr *wq_attr,
> u32 wq_attr_mask, struct ib_udata *udata);
> -struct ib_rwq_ind_table *mlx5_ib_create_rwq_ind_table(struct ib_device *device,
> - struct ib_rwq_ind_table_init_attr *init_attr,
> - struct ib_udata *udata);
> -int mlx5_ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *wq_ind_table);
> +int mlx5_ib_create_rwq_ind_table(struct ib_rwq_ind_table *ib_rwq_ind_table,
> + struct ib_rwq_ind_table_init_attr *init_attr,
> + u32 *ind_tbl_num, struct ib_udata *udata);
> +void mlx5_ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *wq_ind_table);
> struct ib_dm *mlx5_ib_alloc_dm(struct ib_device *ibdev,
> struct ib_ucontext *context,
> struct ib_dm_alloc_attr *attr,
> diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> index 0ae73f4e28a3..3124c80169fb 100644
> --- a/drivers/infiniband/hw/mlx5/qp.c
> +++ b/drivers/infiniband/hw/mlx5/qp.c
> @@ -5063,12 +5063,13 @@ void mlx5_ib_destroy_wq(struct ib_wq *wq, struct ib_udata *udata)
> kfree(rwq);
> }
>
> -struct ib_rwq_ind_table *mlx5_ib_create_rwq_ind_table(struct ib_device *device,
> - struct ib_rwq_ind_table_init_attr *init_attr,
> - struct ib_udata *udata)
> +int mlx5_ib_create_rwq_ind_table(struct ib_rwq_ind_table *ib_rwq_ind_table,
> + struct ib_rwq_ind_table_init_attr *init_attr,
> + u32 *ind_tbl_num, struct ib_udata *udata)
> {
> - struct mlx5_ib_dev *dev = to_mdev(device);
> - struct mlx5_ib_rwq_ind_table *rwq_ind_tbl;
> + struct mlx5_ib_rwq_ind_table *rwq_ind_tbl =
> + to_mrwq_ind_table(ib_rwq_ind_table);
> + struct mlx5_ib_dev *dev = to_mdev(ib_rwq_ind_table->device);
> int sz = 1 << init_attr->log_ind_tbl_size;
> struct mlx5_ib_create_rwq_ind_tbl_resp resp = {};
> size_t min_resp_len;
> @@ -5081,30 +5082,24 @@ struct ib_rwq_ind_table *mlx5_ib_create_rwq_ind_table(struct ib_device *device,
> if (udata->inlen > 0 &&
> !ib_is_udata_cleared(udata, 0,
> udata->inlen))
> - return ERR_PTR(-EOPNOTSUPP);
> + return -EOPNOTSUPP;
>
> if (init_attr->log_ind_tbl_size >
> MLX5_CAP_GEN(dev->mdev, log_max_rqt_size)) {
> mlx5_ib_dbg(dev, "log_ind_tbl_size = %d is bigger than supported = %d\n",
> init_attr->log_ind_tbl_size,
> MLX5_CAP_GEN(dev->mdev, log_max_rqt_size));
> - return ERR_PTR(-EINVAL);
> + return -EINVAL;
> }
>
> min_resp_len = offsetof(typeof(resp), reserved) + sizeof(resp.reserved);
> if (udata->outlen && udata->outlen < min_resp_len)
> - return ERR_PTR(-EINVAL);
> -
> - rwq_ind_tbl = kzalloc(sizeof(*rwq_ind_tbl), GFP_KERNEL);
> - if (!rwq_ind_tbl)
> - return ERR_PTR(-ENOMEM);
> + return -EINVAL;
>
> inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + sizeof(u32) * sz;
> in = kvzalloc(inlen, GFP_KERNEL);
> - if (!in) {
> - err = -ENOMEM;
> - goto err;
> - }
> + if (!in)
> + return -ENOMEM;
>
> rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
>
> @@ -5118,12 +5113,10 @@ struct ib_rwq_ind_table *mlx5_ib_create_rwq_ind_table(struct ib_device *device,
> MLX5_SET(create_rqt_in, in, uid, rwq_ind_tbl->uid);
>
> err = mlx5_core_create_rqt(dev->mdev, in, inlen, &rwq_ind_tbl->rqtn);
> - kvfree(in);
> -
> if (err)
> goto err;
>
> - rwq_ind_tbl->ib_rwq_ind_tbl.ind_tbl_num = rwq_ind_tbl->rqtn;
> + *ind_tbl_num = rwq_ind_tbl->rqtn;
> if (udata->outlen) {
> resp.response_length = offsetof(typeof(resp), response_length) +
> sizeof(resp.response_length);
> @@ -5132,24 +5125,22 @@ struct ib_rwq_ind_table *mlx5_ib_create_rwq_ind_table(struct ib_device *device,
> goto err_copy;
> }
>
> - return &rwq_ind_tbl->ib_rwq_ind_tbl;
> + kvfree(in);
> + return 0;
>
> err_copy:
> mlx5_cmd_destroy_rqt(dev->mdev, rwq_ind_tbl->rqtn, rwq_ind_tbl->uid);
> err:
> - kfree(rwq_ind_tbl);
> - return ERR_PTR(err);
> + kvfree(in);
> + return err;
> }
>
> -int mlx5_ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *ib_rwq_ind_tbl)
> +void mlx5_ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *ib_rwq_ind_tbl)
> {
> struct mlx5_ib_rwq_ind_table *rwq_ind_tbl = to_mrwq_ind_table(ib_rwq_ind_tbl);
> struct mlx5_ib_dev *dev = to_mdev(ib_rwq_ind_tbl->device);
>
> mlx5_cmd_destroy_rqt(dev->mdev, rwq_ind_tbl->rqtn, rwq_ind_tbl->uid);
> -
> - kfree(rwq_ind_tbl);
> - return 0;
> }
>
> int mlx5_ib_modify_wq(struct ib_wq *wq, struct ib_wq_attr *wq_attr,
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 6e2cb69fe90b..ed948946e443 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -2526,11 +2526,10 @@ struct ib_device_ops {
> void (*destroy_wq)(struct ib_wq *wq, struct ib_udata *udata);
> int (*modify_wq)(struct ib_wq *wq, struct ib_wq_attr *attr,
> u32 wq_attr_mask, struct ib_udata *udata);
> - struct ib_rwq_ind_table *(*create_rwq_ind_table)(
> - struct ib_device *device,
> - struct ib_rwq_ind_table_init_attr *init_attr,
> - struct ib_udata *udata);
> - int (*destroy_rwq_ind_table)(struct ib_rwq_ind_table *wq_ind_table);
> + int (*create_rwq_ind_table)(struct ib_rwq_ind_table *ib_rwq_ind_table,
> + struct ib_rwq_ind_table_init_attr *init_attr,
> + u32 *ind_tbl_num, struct ib_udata *udata);
> + void (*destroy_rwq_ind_table)(struct ib_rwq_ind_table *wq_ind_table);
> struct ib_dm *(*alloc_dm)(struct ib_device *device,
> struct ib_ucontext *context,
> struct ib_dm_alloc_attr *attr,
> @@ -2653,6 +2652,7 @@ struct ib_device_ops {
> DECLARE_RDMA_OBJ_SIZE(ib_cq);
> DECLARE_RDMA_OBJ_SIZE(ib_mw);
> DECLARE_RDMA_OBJ_SIZE(ib_pd);
> + DECLARE_RDMA_OBJ_SIZE(ib_rwq_ind_table);
> DECLARE_RDMA_OBJ_SIZE(ib_srq);
> DECLARE_RDMA_OBJ_SIZE(ib_ucontext);
> DECLARE_RDMA_OBJ_SIZE(ib_xrcd);
> @@ -4430,7 +4430,6 @@ struct ib_wq *ib_create_wq(struct ib_pd *pd,
> int ib_destroy_wq(struct ib_wq *wq, struct ib_udata *udata);
> int ib_modify_wq(struct ib_wq *wq, struct ib_wq_attr *attr,
> u32 wq_attr_mask);
> -int ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *wq_ind_table);
>
> int ib_map_mr_sg(struct ib_mr *mr, struct scatterlist *sg, int sg_nents,
> unsigned int *sg_offset, unsigned int page_size);
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme
2020-06-28 9:11 ` Yishai Hadas
@ 2020-06-28 9:41 ` Leon Romanovsky
2020-06-28 10:08 ` Yishai Hadas
0 siblings, 1 reply; 21+ messages in thread
From: Leon Romanovsky @ 2020-06-28 9:41 UTC (permalink / raw)
To: Yishai Hadas
Cc: Doug Ledford, Jason Gunthorpe, linux-rdma, Yishai Hadas, Maor Gottlieb
On Sun, Jun 28, 2020 at 12:11:41PM +0300, Yishai Hadas wrote:
> On 6/24/2020 1:54 PM, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > Move struct ib_rwq_ind_table allocation to ib_core.
> >
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > ---
> > drivers/infiniband/core/device.c | 1 +
> > drivers/infiniband/core/uverbs_cmd.c | 37 ++++++++++++-------
> > drivers/infiniband/core/uverbs_std_types.c | 13 ++++++-
> > drivers/infiniband/core/verbs.c | 25 +------------
> > drivers/infiniband/hw/mlx4/main.c | 4 +-
> > drivers/infiniband/hw/mlx4/mlx4_ib.h | 12 +++---
> > drivers/infiniband/hw/mlx4/qp.c | 40 ++++++--------------
> > drivers/infiniband/hw/mlx5/main.c | 3 ++
> > drivers/infiniband/hw/mlx5/mlx5_ib.h | 8 ++--
> > drivers/infiniband/hw/mlx5/qp.c | 43 +++++++++-------------
> > include/rdma/ib_verbs.h | 11 +++---
> > 11 files changed, 86 insertions(+), 111 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > index b15fa3fa09ac..85d921c8e2b5 100644
> > --- a/drivers/infiniband/core/device.c
> > +++ b/drivers/infiniband/core/device.c
> > @@ -2686,6 +2686,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
> > SET_OBJ_SIZE(dev_ops, ib_cq);
> > SET_OBJ_SIZE(dev_ops, ib_mw);
> > SET_OBJ_SIZE(dev_ops, ib_pd);
> > + SET_OBJ_SIZE(dev_ops, ib_rwq_ind_table);
> > SET_OBJ_SIZE(dev_ops, ib_srq);
> > SET_OBJ_SIZE(dev_ops, ib_ucontext);
> > SET_OBJ_SIZE(dev_ops, ib_xrcd);
> > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> > index a5301f3d3059..a83d11d1e3ee 100644
> > --- a/drivers/infiniband/core/uverbs_cmd.c
> > +++ b/drivers/infiniband/core/uverbs_cmd.c
> > @@ -3090,13 +3090,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> > {
> > struct ib_uverbs_ex_create_rwq_ind_table cmd;
> > struct ib_uverbs_ex_create_rwq_ind_table_resp resp = {};
> > - struct ib_uobject *uobj;
> > + struct ib_uobject *uobj;
> > int err;
> > struct ib_rwq_ind_table_init_attr init_attr = {};
> > struct ib_rwq_ind_table *rwq_ind_tbl;
> > - struct ib_wq **wqs = NULL;
> > + struct ib_wq **wqs = NULL;
> > u32 *wqs_handles = NULL;
> > - struct ib_wq *wq = NULL;
> > + struct ib_wq *wq = NULL;
> > int i, j, num_read_wqs;
> > u32 num_wq_handles;
> > struct uverbs_req_iter iter;
> > @@ -3151,17 +3151,15 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> > goto put_wqs;
> > }
> > - init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
> > - init_attr.ind_tbl = wqs;
> > -
> > - rwq_ind_tbl = ib_dev->ops.create_rwq_ind_table(ib_dev, &init_attr,
> > - &attrs->driver_udata);
> > -
> > - if (IS_ERR(rwq_ind_tbl)) {
> > - err = PTR_ERR(rwq_ind_tbl);
> > + rwq_ind_tbl = rdma_zalloc_drv_obj(ib_dev, ib_rwq_ind_table);
> > + if (!rwq_ind_tbl) {
> > + err = -ENOMEM;
> > goto err_uobj;
> > }
> > + init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
> > + init_attr.ind_tbl = wqs;
> > +
> > rwq_ind_tbl->ind_tbl = wqs;
> > rwq_ind_tbl->log_ind_tbl_size = init_attr.log_ind_tbl_size;
> > rwq_ind_tbl->uobject = uobj;
> > @@ -3169,6 +3167,12 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> > rwq_ind_tbl->device = ib_dev;
> > atomic_set(&rwq_ind_tbl->usecnt, 0);
> > + err = ib_dev->ops.create_rwq_ind_table(rwq_ind_tbl, &init_attr,
> > + &rwq_ind_tbl->ind_tbl_num,
> > + &attrs->driver_udata);
> > + if (err)
> > + goto err_create;
> > +
> > for (i = 0; i < num_wq_handles; i++)
> > atomic_inc(&wqs[i]->usecnt);
> > @@ -3190,7 +3194,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> > return 0;
> > err_copy:
> > - ib_destroy_rwq_ind_table(rwq_ind_tbl);
> > + for (i = 0; i < num_wq_handles; i++)
> > + atomic_dec(&wqs[i]->usecnt);
> > +
> > + if (ib_dev->ops.destroy_rwq_ind_table)
> > + ib_dev->ops.destroy_rwq_ind_table(rwq_ind_tbl);
> > +err_create:
> > + kfree(rwq_ind_tbl);
> > err_uobj:
> > uobj_alloc_abort(uobj, attrs);
> > put_wqs:
> > @@ -4018,8 +4028,7 @@ const struct uapi_definition uverbs_def_write_intf[] = {
> > IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
> > ib_uverbs_ex_destroy_rwq_ind_table,
> > UAPI_DEF_WRITE_I(
> > - struct ib_uverbs_ex_destroy_rwq_ind_table),
> > - UAPI_DEF_METHOD_NEEDS_FN(destroy_rwq_ind_table))),
> > + struct ib_uverbs_ex_destroy_rwq_ind_table))),
> > DECLARE_UVERBS_OBJECT(
> > UVERBS_OBJECT_WQ,
> > diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
> > index e4994cc4cc51..cd82a5a80bdf 100644
> > --- a/drivers/infiniband/core/uverbs_std_types.c
> > +++ b/drivers/infiniband/core/uverbs_std_types.c
> > @@ -82,12 +82,21 @@ static int uverbs_free_rwq_ind_tbl(struct ib_uobject *uobject,
> > {
> > struct ib_rwq_ind_table *rwq_ind_tbl = uobject->object;
> > struct ib_wq **ind_tbl = rwq_ind_tbl->ind_tbl;
> > - int ret;
> > + u32 table_size = (1 << rwq_ind_tbl->log_ind_tbl_size);
> > + int ret = 0, i;
> > +
> > + if (atomic_read(&rwq_ind_tbl->usecnt))
> > + ret = -EBUSY;
> > - ret = ib_destroy_rwq_ind_table(rwq_ind_tbl);
> > if (ib_is_destroy_retryable(ret, why, uobject))
> > return ret;
> > + for (i = 0; i < table_size; i++)
> > + atomic_dec(&ind_tbl[i]->usecnt);
> > +
>
> Upon destroy we first expect to destroy the object that is pointing to other
> objetcs and only then descraese their ref count, this was the orignal logic
> here. (See also ib_destroy_qp_user(), ib_destroy_wq() and others).
It is a bug, proper logic is to allocate HW object, elevate reference
counting, decrease reference counter and deallocate HW object.
Everything else should be fixed too.
>
> > + if (rwq_ind_tbl->device->ops.destroy_rwq_ind_table)
> > + rwq_ind_tbl->device->ops.destroy_rwq_ind_table(rwq_ind_tbl);
>
> This doesn't really make sense, if no destroy function was set, the
> allocation should be prevented as well.
Should we delete mlx4 RWQ implementation too?
After this refactoring, mlx4 won't have .destroy_rwq_ind_table.
Thanks
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme
2020-06-28 9:41 ` Leon Romanovsky
@ 2020-06-28 10:08 ` Yishai Hadas
2020-06-28 10:33 ` Leon Romanovsky
0 siblings, 1 reply; 21+ messages in thread
From: Yishai Hadas @ 2020-06-28 10:08 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Jason Gunthorpe, linux-rdma, Yishai Hadas, Maor Gottlieb
On 6/28/2020 12:41 PM, Leon Romanovsky wrote:
> On Sun, Jun 28, 2020 at 12:11:41PM +0300, Yishai Hadas wrote:
>> On 6/24/2020 1:54 PM, Leon Romanovsky wrote:
>>> From: Leon Romanovsky <leonro@mellanox.com>
>>>
>>> Move struct ib_rwq_ind_table allocation to ib_core.
>>>
>>> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>>> ---
>>> drivers/infiniband/core/device.c | 1 +
>>> drivers/infiniband/core/uverbs_cmd.c | 37 ++++++++++++-------
>>> drivers/infiniband/core/uverbs_std_types.c | 13 ++++++-
>>> drivers/infiniband/core/verbs.c | 25 +------------
>>> drivers/infiniband/hw/mlx4/main.c | 4 +-
>>> drivers/infiniband/hw/mlx4/mlx4_ib.h | 12 +++---
>>> drivers/infiniband/hw/mlx4/qp.c | 40 ++++++--------------
>>> drivers/infiniband/hw/mlx5/main.c | 3 ++
>>> drivers/infiniband/hw/mlx5/mlx5_ib.h | 8 ++--
>>> drivers/infiniband/hw/mlx5/qp.c | 43 +++++++++-------------
>>> include/rdma/ib_verbs.h | 11 +++---
>>> 11 files changed, 86 insertions(+), 111 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
>>> index b15fa3fa09ac..85d921c8e2b5 100644
>>> --- a/drivers/infiniband/core/device.c
>>> +++ b/drivers/infiniband/core/device.c
>>> @@ -2686,6 +2686,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
>>> SET_OBJ_SIZE(dev_ops, ib_cq);
>>> SET_OBJ_SIZE(dev_ops, ib_mw);
>>> SET_OBJ_SIZE(dev_ops, ib_pd);
>>> + SET_OBJ_SIZE(dev_ops, ib_rwq_ind_table);
>>> SET_OBJ_SIZE(dev_ops, ib_srq);
>>> SET_OBJ_SIZE(dev_ops, ib_ucontext);
>>> SET_OBJ_SIZE(dev_ops, ib_xrcd);
>>> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
>>> index a5301f3d3059..a83d11d1e3ee 100644
>>> --- a/drivers/infiniband/core/uverbs_cmd.c
>>> +++ b/drivers/infiniband/core/uverbs_cmd.c
>>> @@ -3090,13 +3090,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
>>> {
>>> struct ib_uverbs_ex_create_rwq_ind_table cmd;
>>> struct ib_uverbs_ex_create_rwq_ind_table_resp resp = {};
>>> - struct ib_uobject *uobj;
>>> + struct ib_uobject *uobj;
>>> int err;
>>> struct ib_rwq_ind_table_init_attr init_attr = {};
>>> struct ib_rwq_ind_table *rwq_ind_tbl;
>>> - struct ib_wq **wqs = NULL;
>>> + struct ib_wq **wqs = NULL;
>>> u32 *wqs_handles = NULL;
>>> - struct ib_wq *wq = NULL;
>>> + struct ib_wq *wq = NULL;
>>> int i, j, num_read_wqs;
>>> u32 num_wq_handles;
>>> struct uverbs_req_iter iter;
>>> @@ -3151,17 +3151,15 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
>>> goto put_wqs;
>>> }
>>> - init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
>>> - init_attr.ind_tbl = wqs;
>>> -
>>> - rwq_ind_tbl = ib_dev->ops.create_rwq_ind_table(ib_dev, &init_attr,
>>> - &attrs->driver_udata);
>>> -
>>> - if (IS_ERR(rwq_ind_tbl)) {
>>> - err = PTR_ERR(rwq_ind_tbl);
>>> + rwq_ind_tbl = rdma_zalloc_drv_obj(ib_dev, ib_rwq_ind_table);
>>> + if (!rwq_ind_tbl) {
>>> + err = -ENOMEM;
>>> goto err_uobj;
>>> }
>>> + init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
>>> + init_attr.ind_tbl = wqs;
>>> +
>>> rwq_ind_tbl->ind_tbl = wqs;
>>> rwq_ind_tbl->log_ind_tbl_size = init_attr.log_ind_tbl_size;
>>> rwq_ind_tbl->uobject = uobj;
>>> @@ -3169,6 +3167,12 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
>>> rwq_ind_tbl->device = ib_dev;
>>> atomic_set(&rwq_ind_tbl->usecnt, 0);
>>> + err = ib_dev->ops.create_rwq_ind_table(rwq_ind_tbl, &init_attr,
>>> + &rwq_ind_tbl->ind_tbl_num,
>>> + &attrs->driver_udata);
>>> + if (err)
>>> + goto err_create;
>>> +
>>> for (i = 0; i < num_wq_handles; i++)
>>> atomic_inc(&wqs[i]->usecnt);
>>> @@ -3190,7 +3194,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
>>> return 0;
>>> err_copy:
>>> - ib_destroy_rwq_ind_table(rwq_ind_tbl);
>>> + for (i = 0; i < num_wq_handles; i++)
>>> + atomic_dec(&wqs[i]->usecnt);
>>> +
>>> + if (ib_dev->ops.destroy_rwq_ind_table)
>>> + ib_dev->ops.destroy_rwq_ind_table(rwq_ind_tbl);
>>> +err_create:
>>> + kfree(rwq_ind_tbl);
>>> err_uobj:
>>> uobj_alloc_abort(uobj, attrs);
>>> put_wqs:
>>> @@ -4018,8 +4028,7 @@ const struct uapi_definition uverbs_def_write_intf[] = {
>>> IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
>>> ib_uverbs_ex_destroy_rwq_ind_table,
>>> UAPI_DEF_WRITE_I(
>>> - struct ib_uverbs_ex_destroy_rwq_ind_table),
>>> - UAPI_DEF_METHOD_NEEDS_FN(destroy_rwq_ind_table))),
>>> + struct ib_uverbs_ex_destroy_rwq_ind_table))),
>>> DECLARE_UVERBS_OBJECT(
>>> UVERBS_OBJECT_WQ,
>>> diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
>>> index e4994cc4cc51..cd82a5a80bdf 100644
>>> --- a/drivers/infiniband/core/uverbs_std_types.c
>>> +++ b/drivers/infiniband/core/uverbs_std_types.c
>>> @@ -82,12 +82,21 @@ static int uverbs_free_rwq_ind_tbl(struct ib_uobject *uobject,
>>> {
>>> struct ib_rwq_ind_table *rwq_ind_tbl = uobject->object;
>>> struct ib_wq **ind_tbl = rwq_ind_tbl->ind_tbl;
>>> - int ret;
>>> + u32 table_size = (1 << rwq_ind_tbl->log_ind_tbl_size);
>>> + int ret = 0, i;
>>> +
>>> + if (atomic_read(&rwq_ind_tbl->usecnt))
>>> + ret = -EBUSY;
>>> - ret = ib_destroy_rwq_ind_table(rwq_ind_tbl);
>>> if (ib_is_destroy_retryable(ret, why, uobject))
>>> return ret;
>>> + for (i = 0; i < table_size; i++)
>>> + atomic_dec(&ind_tbl[i]->usecnt);
>>> +
>>
>> Upon destroy we first expect to destroy the object that is pointing to other
>> objetcs and only then descraese their ref count, this was the orignal logic
>> here. (See also ib_destroy_qp_user(), ib_destroy_wq() and others).
>
> It is a bug, proper logic is to allocate HW object, elevate reference
> counting, decrease reference counter and deallocate HW object.
>
I don't agree, only upon a succesfull destroy the we can go a ahead and
safely decraese ref count from the pointed objects. This pevents any
race from parallel desctrcution of the pointed object and consider any
potenaial failure on the original object before decaresing ref counts as
of in the QP case.
> Everything else should be fixed too.
I beleive that this patch should be fixed and not all other places.
By the way, see your previous patch from this series regarding MW that
follows same logic, first dealloc_mw() and only then does atomic_dec()
on the pointed PD.
>
>>
>>> + if (rwq_ind_tbl->device->ops.destroy_rwq_ind_table)
>>> + rwq_ind_tbl->device->ops.destroy_rwq_ind_table(rwq_ind_tbl);
>>
>> This doesn't really make sense, if no destroy function was set, the
>> allocation should be prevented as well.
>
> Should we delete mlx4 RWQ implementation too?
> After this refactoring, mlx4 won't have .destroy_rwq_ind_table.
>
We can expect from symetric reasons to have always alloc/destroy from
drivers point of view. The IB layer will not check whether the destroy
function exist but will always call it, the driver in its turn may do
nothing.
Yishai
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme
2020-06-28 10:08 ` Yishai Hadas
@ 2020-06-28 10:33 ` Leon Romanovsky
2020-06-28 11:55 ` Yishai Hadas
0 siblings, 1 reply; 21+ messages in thread
From: Leon Romanovsky @ 2020-06-28 10:33 UTC (permalink / raw)
To: Yishai Hadas
Cc: Doug Ledford, Jason Gunthorpe, linux-rdma, Yishai Hadas, Maor Gottlieb
On Sun, Jun 28, 2020 at 01:08:24PM +0300, Yishai Hadas wrote:
> On 6/28/2020 12:41 PM, Leon Romanovsky wrote:
> > On Sun, Jun 28, 2020 at 12:11:41PM +0300, Yishai Hadas wrote:
> > > On 6/24/2020 1:54 PM, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <leonro@mellanox.com>
> > > >
> > > > Move struct ib_rwq_ind_table allocation to ib_core.
> > > >
> > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > > ---
> > > > drivers/infiniband/core/device.c | 1 +
> > > > drivers/infiniband/core/uverbs_cmd.c | 37 ++++++++++++-------
> > > > drivers/infiniband/core/uverbs_std_types.c | 13 ++++++-
> > > > drivers/infiniband/core/verbs.c | 25 +------------
> > > > drivers/infiniband/hw/mlx4/main.c | 4 +-
> > > > drivers/infiniband/hw/mlx4/mlx4_ib.h | 12 +++---
> > > > drivers/infiniband/hw/mlx4/qp.c | 40 ++++++--------------
> > > > drivers/infiniband/hw/mlx5/main.c | 3 ++
> > > > drivers/infiniband/hw/mlx5/mlx5_ib.h | 8 ++--
> > > > drivers/infiniband/hw/mlx5/qp.c | 43 +++++++++-------------
> > > > include/rdma/ib_verbs.h | 11 +++---
> > > > 11 files changed, 86 insertions(+), 111 deletions(-)
> > > >
> > > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > > > index b15fa3fa09ac..85d921c8e2b5 100644
> > > > --- a/drivers/infiniband/core/device.c
> > > > +++ b/drivers/infiniband/core/device.c
> > > > @@ -2686,6 +2686,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
> > > > SET_OBJ_SIZE(dev_ops, ib_cq);
> > > > SET_OBJ_SIZE(dev_ops, ib_mw);
> > > > SET_OBJ_SIZE(dev_ops, ib_pd);
> > > > + SET_OBJ_SIZE(dev_ops, ib_rwq_ind_table);
> > > > SET_OBJ_SIZE(dev_ops, ib_srq);
> > > > SET_OBJ_SIZE(dev_ops, ib_ucontext);
> > > > SET_OBJ_SIZE(dev_ops, ib_xrcd);
> > > > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> > > > index a5301f3d3059..a83d11d1e3ee 100644
> > > > --- a/drivers/infiniband/core/uverbs_cmd.c
> > > > +++ b/drivers/infiniband/core/uverbs_cmd.c
> > > > @@ -3090,13 +3090,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> > > > {
> > > > struct ib_uverbs_ex_create_rwq_ind_table cmd;
> > > > struct ib_uverbs_ex_create_rwq_ind_table_resp resp = {};
> > > > - struct ib_uobject *uobj;
> > > > + struct ib_uobject *uobj;
> > > > int err;
> > > > struct ib_rwq_ind_table_init_attr init_attr = {};
> > > > struct ib_rwq_ind_table *rwq_ind_tbl;
> > > > - struct ib_wq **wqs = NULL;
> > > > + struct ib_wq **wqs = NULL;
> > > > u32 *wqs_handles = NULL;
> > > > - struct ib_wq *wq = NULL;
> > > > + struct ib_wq *wq = NULL;
> > > > int i, j, num_read_wqs;
> > > > u32 num_wq_handles;
> > > > struct uverbs_req_iter iter;
> > > > @@ -3151,17 +3151,15 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> > > > goto put_wqs;
> > > > }
> > > > - init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
> > > > - init_attr.ind_tbl = wqs;
> > > > -
> > > > - rwq_ind_tbl = ib_dev->ops.create_rwq_ind_table(ib_dev, &init_attr,
> > > > - &attrs->driver_udata);
> > > > -
> > > > - if (IS_ERR(rwq_ind_tbl)) {
> > > > - err = PTR_ERR(rwq_ind_tbl);
> > > > + rwq_ind_tbl = rdma_zalloc_drv_obj(ib_dev, ib_rwq_ind_table);
> > > > + if (!rwq_ind_tbl) {
> > > > + err = -ENOMEM;
> > > > goto err_uobj;
> > > > }
> > > > + init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
> > > > + init_attr.ind_tbl = wqs;
> > > > +
> > > > rwq_ind_tbl->ind_tbl = wqs;
> > > > rwq_ind_tbl->log_ind_tbl_size = init_attr.log_ind_tbl_size;
> > > > rwq_ind_tbl->uobject = uobj;
> > > > @@ -3169,6 +3167,12 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> > > > rwq_ind_tbl->device = ib_dev;
> > > > atomic_set(&rwq_ind_tbl->usecnt, 0);
> > > > + err = ib_dev->ops.create_rwq_ind_table(rwq_ind_tbl, &init_attr,
> > > > + &rwq_ind_tbl->ind_tbl_num,
> > > > + &attrs->driver_udata);
> > > > + if (err)
> > > > + goto err_create;
> > > > +
> > > > for (i = 0; i < num_wq_handles; i++)
> > > > atomic_inc(&wqs[i]->usecnt);
> > > > @@ -3190,7 +3194,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> > > > return 0;
> > > > err_copy:
> > > > - ib_destroy_rwq_ind_table(rwq_ind_tbl);
> > > > + for (i = 0; i < num_wq_handles; i++)
> > > > + atomic_dec(&wqs[i]->usecnt);
> > > > +
> > > > + if (ib_dev->ops.destroy_rwq_ind_table)
> > > > + ib_dev->ops.destroy_rwq_ind_table(rwq_ind_tbl);
> > > > +err_create:
> > > > + kfree(rwq_ind_tbl);
> > > > err_uobj:
> > > > uobj_alloc_abort(uobj, attrs);
> > > > put_wqs:
> > > > @@ -4018,8 +4028,7 @@ const struct uapi_definition uverbs_def_write_intf[] = {
> > > > IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
> > > > ib_uverbs_ex_destroy_rwq_ind_table,
> > > > UAPI_DEF_WRITE_I(
> > > > - struct ib_uverbs_ex_destroy_rwq_ind_table),
> > > > - UAPI_DEF_METHOD_NEEDS_FN(destroy_rwq_ind_table))),
> > > > + struct ib_uverbs_ex_destroy_rwq_ind_table))),
> > > > DECLARE_UVERBS_OBJECT(
> > > > UVERBS_OBJECT_WQ,
> > > > diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
> > > > index e4994cc4cc51..cd82a5a80bdf 100644
> > > > --- a/drivers/infiniband/core/uverbs_std_types.c
> > > > +++ b/drivers/infiniband/core/uverbs_std_types.c
> > > > @@ -82,12 +82,21 @@ static int uverbs_free_rwq_ind_tbl(struct ib_uobject *uobject,
> > > > {
> > > > struct ib_rwq_ind_table *rwq_ind_tbl = uobject->object;
> > > > struct ib_wq **ind_tbl = rwq_ind_tbl->ind_tbl;
> > > > - int ret;
> > > > + u32 table_size = (1 << rwq_ind_tbl->log_ind_tbl_size);
> > > > + int ret = 0, i;
> > > > +
> > > > + if (atomic_read(&rwq_ind_tbl->usecnt))
> > > > + ret = -EBUSY;
> > > > - ret = ib_destroy_rwq_ind_table(rwq_ind_tbl);
> > > > if (ib_is_destroy_retryable(ret, why, uobject))
> > > > return ret;
> > > > + for (i = 0; i < table_size; i++)
> > > > + atomic_dec(&ind_tbl[i]->usecnt);
> > > > +
> > >
> > > Upon destroy we first expect to destroy the object that is pointing to other
> > > objetcs and only then descraese their ref count, this was the orignal logic
> > > here. (See also ib_destroy_qp_user(), ib_destroy_wq() and others).
> >
> > It is a bug, proper logic is to allocate HW object, elevate reference
> > counting, decrease reference counter and deallocate HW object.
> >
>
> I don't agree, only upon a succesfull destroy the we can go a ahead and
> safely decraese ref count from the pointed objects. This pevents any race
> from parallel desctrcution of the pointed object and consider any potenaial
> failure on the original object before decaresing ref counts as of in the QP
> case.
There is no such thing successful destroy, from user perspective ALL
valid destroys should success. Users don't know how to recover from
failure at this stage and they have one of two options: reboot the
server or leave memory/HW leak.
There are not supposed to be parallel destructions if core code locked
everything properly and one of the threads decreased reference counter
to zero.
Can you please be more specific and give the scenario of your potential
flow?
>
> > Everything else should be fixed too.
>
> I beleive that this patch should be fixed and not all other places.
> By the way, see your previous patch from this series regarding MW that
> follows same logic, first dealloc_mw() and only then does atomic_dec() on
> the pointed PD.
Of course, it was intended, it is completely different case and flow. PD holds
MW and this is why the reference counter is decreased only after MW was destroyed.
>
> >
> > >
> > > > + if (rwq_ind_tbl->device->ops.destroy_rwq_ind_table)
> > > > + rwq_ind_tbl->device->ops.destroy_rwq_ind_table(rwq_ind_tbl);
> > >
> > > This doesn't really make sense, if no destroy function was set, the
> > > allocation should be prevented as well.
> >
> > Should we delete mlx4 RWQ implementation too?
> > After this refactoring, mlx4 won't have .destroy_rwq_ind_table.
> >
>
> We can expect from symetric reasons to have always alloc/destroy from
> drivers point of view. The IB layer will not check whether the destroy
> function exist but will always call it, the driver in its turn may do
> nothing.
Maybe, I don't like empty functions in the drivers, it is silly.
>
> Yishai
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme
2020-06-28 10:33 ` Leon Romanovsky
@ 2020-06-28 11:55 ` Yishai Hadas
2020-06-28 13:10 ` Leon Romanovsky
0 siblings, 1 reply; 21+ messages in thread
From: Yishai Hadas @ 2020-06-28 11:55 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Jason Gunthorpe, linux-rdma, Yishai Hadas, Maor Gottlieb
On 6/28/2020 1:33 PM, Leon Romanovsky wrote:
> On Sun, Jun 28, 2020 at 01:08:24PM +0300, Yishai Hadas wrote:
>> On 6/28/2020 12:41 PM, Leon Romanovsky wrote:
>>> On Sun, Jun 28, 2020 at 12:11:41PM +0300, Yishai Hadas wrote:
>>>> On 6/24/2020 1:54 PM, Leon Romanovsky wrote:
>>>>> From: Leon Romanovsky <leonro@mellanox.com>
>>>>>
>>>>> Move struct ib_rwq_ind_table allocation to ib_core.
>>>>>
>>>>> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>>>>> ---
>>>>> drivers/infiniband/core/device.c | 1 +
>>>>> drivers/infiniband/core/uverbs_cmd.c | 37 ++++++++++++-------
>>>>> drivers/infiniband/core/uverbs_std_types.c | 13 ++++++-
>>>>> drivers/infiniband/core/verbs.c | 25 +------------
>>>>> drivers/infiniband/hw/mlx4/main.c | 4 +-
>>>>> drivers/infiniband/hw/mlx4/mlx4_ib.h | 12 +++---
>>>>> drivers/infiniband/hw/mlx4/qp.c | 40 ++++++--------------
>>>>> drivers/infiniband/hw/mlx5/main.c | 3 ++
>>>>> drivers/infiniband/hw/mlx5/mlx5_ib.h | 8 ++--
>>>>> drivers/infiniband/hw/mlx5/qp.c | 43 +++++++++-------------
>>>>> include/rdma/ib_verbs.h | 11 +++---
>>>>> 11 files changed, 86 insertions(+), 111 deletions(-)
>>>>>
>>>>> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
>>>>> index b15fa3fa09ac..85d921c8e2b5 100644
>>>>> --- a/drivers/infiniband/core/device.c
>>>>> +++ b/drivers/infiniband/core/device.c
>>>>> @@ -2686,6 +2686,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
>>>>> SET_OBJ_SIZE(dev_ops, ib_cq);
>>>>> SET_OBJ_SIZE(dev_ops, ib_mw);
>>>>> SET_OBJ_SIZE(dev_ops, ib_pd);
>>>>> + SET_OBJ_SIZE(dev_ops, ib_rwq_ind_table);
>>>>> SET_OBJ_SIZE(dev_ops, ib_srq);
>>>>> SET_OBJ_SIZE(dev_ops, ib_ucontext);
>>>>> SET_OBJ_SIZE(dev_ops, ib_xrcd);
>>>>> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
>>>>> index a5301f3d3059..a83d11d1e3ee 100644
>>>>> --- a/drivers/infiniband/core/uverbs_cmd.c
>>>>> +++ b/drivers/infiniband/core/uverbs_cmd.c
>>>>> @@ -3090,13 +3090,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
>>>>> {
>>>>> struct ib_uverbs_ex_create_rwq_ind_table cmd;
>>>>> struct ib_uverbs_ex_create_rwq_ind_table_resp resp = {};
>>>>> - struct ib_uobject *uobj;
>>>>> + struct ib_uobject *uobj;
>>>>> int err;
>>>>> struct ib_rwq_ind_table_init_attr init_attr = {};
>>>>> struct ib_rwq_ind_table *rwq_ind_tbl;
>>>>> - struct ib_wq **wqs = NULL;
>>>>> + struct ib_wq **wqs = NULL;
>>>>> u32 *wqs_handles = NULL;
>>>>> - struct ib_wq *wq = NULL;
>>>>> + struct ib_wq *wq = NULL;
>>>>> int i, j, num_read_wqs;
>>>>> u32 num_wq_handles;
>>>>> struct uverbs_req_iter iter;
>>>>> @@ -3151,17 +3151,15 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
>>>>> goto put_wqs;
>>>>> }
>>>>> - init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
>>>>> - init_attr.ind_tbl = wqs;
>>>>> -
>>>>> - rwq_ind_tbl = ib_dev->ops.create_rwq_ind_table(ib_dev, &init_attr,
>>>>> - &attrs->driver_udata);
>>>>> -
>>>>> - if (IS_ERR(rwq_ind_tbl)) {
>>>>> - err = PTR_ERR(rwq_ind_tbl);
>>>>> + rwq_ind_tbl = rdma_zalloc_drv_obj(ib_dev, ib_rwq_ind_table);
>>>>> + if (!rwq_ind_tbl) {
>>>>> + err = -ENOMEM;
>>>>> goto err_uobj;
>>>>> }
>>>>> + init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
>>>>> + init_attr.ind_tbl = wqs;
>>>>> +
>>>>> rwq_ind_tbl->ind_tbl = wqs;
>>>>> rwq_ind_tbl->log_ind_tbl_size = init_attr.log_ind_tbl_size;
>>>>> rwq_ind_tbl->uobject = uobj;
>>>>> @@ -3169,6 +3167,12 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
>>>>> rwq_ind_tbl->device = ib_dev;
>>>>> atomic_set(&rwq_ind_tbl->usecnt, 0);
>>>>> + err = ib_dev->ops.create_rwq_ind_table(rwq_ind_tbl, &init_attr,
>>>>> + &rwq_ind_tbl->ind_tbl_num,
>>>>> + &attrs->driver_udata);
>>>>> + if (err)
>>>>> + goto err_create;
>>>>> +
>>>>> for (i = 0; i < num_wq_handles; i++)
>>>>> atomic_inc(&wqs[i]->usecnt);
>>>>> @@ -3190,7 +3194,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
>>>>> return 0;
>>>>> err_copy:
>>>>> - ib_destroy_rwq_ind_table(rwq_ind_tbl);
>>>>> + for (i = 0; i < num_wq_handles; i++)
>>>>> + atomic_dec(&wqs[i]->usecnt);
>>>>> +
>>>>> + if (ib_dev->ops.destroy_rwq_ind_table)
>>>>> + ib_dev->ops.destroy_rwq_ind_table(rwq_ind_tbl);
>>>>> +err_create:
>>>>> + kfree(rwq_ind_tbl);
>>>>> err_uobj:
>>>>> uobj_alloc_abort(uobj, attrs);
>>>>> put_wqs:
>>>>> @@ -4018,8 +4028,7 @@ const struct uapi_definition uverbs_def_write_intf[] = {
>>>>> IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
>>>>> ib_uverbs_ex_destroy_rwq_ind_table,
>>>>> UAPI_DEF_WRITE_I(
>>>>> - struct ib_uverbs_ex_destroy_rwq_ind_table),
>>>>> - UAPI_DEF_METHOD_NEEDS_FN(destroy_rwq_ind_table))),
>>>>> + struct ib_uverbs_ex_destroy_rwq_ind_table))),
>>>>> DECLARE_UVERBS_OBJECT(
>>>>> UVERBS_OBJECT_WQ,
>>>>> diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
>>>>> index e4994cc4cc51..cd82a5a80bdf 100644
>>>>> --- a/drivers/infiniband/core/uverbs_std_types.c
>>>>> +++ b/drivers/infiniband/core/uverbs_std_types.c
>>>>> @@ -82,12 +82,21 @@ static int uverbs_free_rwq_ind_tbl(struct ib_uobject *uobject,
>>>>> {
>>>>> struct ib_rwq_ind_table *rwq_ind_tbl = uobject->object;
>>>>> struct ib_wq **ind_tbl = rwq_ind_tbl->ind_tbl;
>>>>> - int ret;
>>>>> + u32 table_size = (1 << rwq_ind_tbl->log_ind_tbl_size);
>>>>> + int ret = 0, i;
>>>>> +
>>>>> + if (atomic_read(&rwq_ind_tbl->usecnt))
>>>>> + ret = -EBUSY;
>>>>> - ret = ib_destroy_rwq_ind_table(rwq_ind_tbl);
>>>>> if (ib_is_destroy_retryable(ret, why, uobject))
>>>>> return ret;
>>>>> + for (i = 0; i < table_size; i++)
>>>>> + atomic_dec(&ind_tbl[i]->usecnt);
>>>>> +
>>>>
>>>> Upon destroy we first expect to destroy the object that is pointing to other
>>>> objetcs and only then descraese their ref count, this was the orignal logic
>>>> here. (See also ib_destroy_qp_user(), ib_destroy_wq() and others).
>>>
>>> It is a bug, proper logic is to allocate HW object, elevate reference
>>> counting, decrease reference counter and deallocate HW object.
>>>
>>
>> I don't agree, only upon a succesfull destroy the we can go a ahead and
>> safely decraese ref count from the pointed objects. This pevents any race
>> from parallel desctrcution of the pointed object and consider any potenaial
>> failure on the original object before decaresing ref counts as of in the QP
>> case.
>
> There is no such thing successful destroy, from user perspective ALL
> valid destroys should success.
This is exactly the ref count purpsoe to prevent calling destoy on an
object that may be pointed by other one, otherwise it will fail in the
driver / FW.
> Users don't know how to recover from
> failure at this stage and they have one of two options: reboot the
> server or leave memory/HW leak.
>
> There are not supposed to be parallel destructions if core code locked
> everything properly and one of the threads decreased reference counter
> to zero.
>
Right, this should be achived by a proer ref counting odering as done today.
> Can you please be more specific and give the scenario of your potential
> flow?
>
The above clearly explain the motivation for current ref count usage.
>>
>>> Everything else should be fixed too.
>>
>> I beleive that this patch should be fixed and not all other places.
>> By the way, see your previous patch from this series regarding MW that
>> follows same logic, first dealloc_mw() and only then does atomic_dec() on
>> the pointed PD.
>
> Of course, it was intended, it is completely different case and flow. PD holds
> MW and this is why the reference counter is decreased only after MW was destroyed.
PD doesn't hold MW, MW points to a PD upon its creation as of QP, MR and
othres.
>
>>
>>>
>>>>
>>>>> + if (rwq_ind_tbl->device->ops.destroy_rwq_ind_table)
>>>>> + rwq_ind_tbl->device->ops.destroy_rwq_ind_table(rwq_ind_tbl);
>>>>
>>>> This doesn't really make sense, if no destroy function was set, the
>>>> allocation should be prevented as well.
>>>
>>> Should we delete mlx4 RWQ implementation too?
>>> After this refactoring, mlx4 won't have .destroy_rwq_ind_table.
>>>
>>
>> We can expect from symetric reasons to have always alloc/destroy from
>> drivers point of view. The IB layer will not check whether the destroy
>> function exist but will always call it, the driver in its turn may do
>> nothing.
>
> Maybe, I don't like empty functions in the drivers, it is silly.
>
We can consier that point, let's get other people feedback as well.
Yishai
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme
2020-06-28 11:55 ` Yishai Hadas
@ 2020-06-28 13:10 ` Leon Romanovsky
0 siblings, 0 replies; 21+ messages in thread
From: Leon Romanovsky @ 2020-06-28 13:10 UTC (permalink / raw)
To: Yishai Hadas
Cc: Doug Ledford, Jason Gunthorpe, linux-rdma, Yishai Hadas, Maor Gottlieb
On Sun, Jun 28, 2020 at 02:55:31PM +0300, Yishai Hadas wrote:
> On 6/28/2020 1:33 PM, Leon Romanovsky wrote:
> > On Sun, Jun 28, 2020 at 01:08:24PM +0300, Yishai Hadas wrote:
> > > On 6/28/2020 12:41 PM, Leon Romanovsky wrote:
> > > > On Sun, Jun 28, 2020 at 12:11:41PM +0300, Yishai Hadas wrote:
> > > > > On 6/24/2020 1:54 PM, Leon Romanovsky wrote:
> > > > > > From: Leon Romanovsky <leonro@mellanox.com>
> > > > > >
> > > > > > Move struct ib_rwq_ind_table allocation to ib_core.
> > > > > >
> > > > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > > > > ---
> > > > > > drivers/infiniband/core/device.c | 1 +
> > > > > > drivers/infiniband/core/uverbs_cmd.c | 37 ++++++++++++-------
> > > > > > drivers/infiniband/core/uverbs_std_types.c | 13 ++++++-
> > > > > > drivers/infiniband/core/verbs.c | 25 +------------
> > > > > > drivers/infiniband/hw/mlx4/main.c | 4 +-
> > > > > > drivers/infiniband/hw/mlx4/mlx4_ib.h | 12 +++---
> > > > > > drivers/infiniband/hw/mlx4/qp.c | 40 ++++++--------------
> > > > > > drivers/infiniband/hw/mlx5/main.c | 3 ++
> > > > > > drivers/infiniband/hw/mlx5/mlx5_ib.h | 8 ++--
> > > > > > drivers/infiniband/hw/mlx5/qp.c | 43 +++++++++-------------
> > > > > > include/rdma/ib_verbs.h | 11 +++---
> > > > > > 11 files changed, 86 insertions(+), 111 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > > > > > index b15fa3fa09ac..85d921c8e2b5 100644
> > > > > > --- a/drivers/infiniband/core/device.c
> > > > > > +++ b/drivers/infiniband/core/device.c
> > > > > > @@ -2686,6 +2686,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
> > > > > > SET_OBJ_SIZE(dev_ops, ib_cq);
> > > > > > SET_OBJ_SIZE(dev_ops, ib_mw);
> > > > > > SET_OBJ_SIZE(dev_ops, ib_pd);
> > > > > > + SET_OBJ_SIZE(dev_ops, ib_rwq_ind_table);
> > > > > > SET_OBJ_SIZE(dev_ops, ib_srq);
> > > > > > SET_OBJ_SIZE(dev_ops, ib_ucontext);
> > > > > > SET_OBJ_SIZE(dev_ops, ib_xrcd);
> > > > > > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> > > > > > index a5301f3d3059..a83d11d1e3ee 100644
> > > > > > --- a/drivers/infiniband/core/uverbs_cmd.c
> > > > > > +++ b/drivers/infiniband/core/uverbs_cmd.c
> > > > > > @@ -3090,13 +3090,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> > > > > > {
> > > > > > struct ib_uverbs_ex_create_rwq_ind_table cmd;
> > > > > > struct ib_uverbs_ex_create_rwq_ind_table_resp resp = {};
> > > > > > - struct ib_uobject *uobj;
> > > > > > + struct ib_uobject *uobj;
> > > > > > int err;
> > > > > > struct ib_rwq_ind_table_init_attr init_attr = {};
> > > > > > struct ib_rwq_ind_table *rwq_ind_tbl;
> > > > > > - struct ib_wq **wqs = NULL;
> > > > > > + struct ib_wq **wqs = NULL;
> > > > > > u32 *wqs_handles = NULL;
> > > > > > - struct ib_wq *wq = NULL;
> > > > > > + struct ib_wq *wq = NULL;
> > > > > > int i, j, num_read_wqs;
> > > > > > u32 num_wq_handles;
> > > > > > struct uverbs_req_iter iter;
> > > > > > @@ -3151,17 +3151,15 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> > > > > > goto put_wqs;
> > > > > > }
> > > > > > - init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
> > > > > > - init_attr.ind_tbl = wqs;
> > > > > > -
> > > > > > - rwq_ind_tbl = ib_dev->ops.create_rwq_ind_table(ib_dev, &init_attr,
> > > > > > - &attrs->driver_udata);
> > > > > > -
> > > > > > - if (IS_ERR(rwq_ind_tbl)) {
> > > > > > - err = PTR_ERR(rwq_ind_tbl);
> > > > > > + rwq_ind_tbl = rdma_zalloc_drv_obj(ib_dev, ib_rwq_ind_table);
> > > > > > + if (!rwq_ind_tbl) {
> > > > > > + err = -ENOMEM;
> > > > > > goto err_uobj;
> > > > > > }
> > > > > > + init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
> > > > > > + init_attr.ind_tbl = wqs;
> > > > > > +
> > > > > > rwq_ind_tbl->ind_tbl = wqs;
> > > > > > rwq_ind_tbl->log_ind_tbl_size = init_attr.log_ind_tbl_size;
> > > > > > rwq_ind_tbl->uobject = uobj;
> > > > > > @@ -3169,6 +3167,12 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> > > > > > rwq_ind_tbl->device = ib_dev;
> > > > > > atomic_set(&rwq_ind_tbl->usecnt, 0);
> > > > > > + err = ib_dev->ops.create_rwq_ind_table(rwq_ind_tbl, &init_attr,
> > > > > > + &rwq_ind_tbl->ind_tbl_num,
> > > > > > + &attrs->driver_udata);
> > > > > > + if (err)
> > > > > > + goto err_create;
> > > > > > +
> > > > > > for (i = 0; i < num_wq_handles; i++)
> > > > > > atomic_inc(&wqs[i]->usecnt);
> > > > > > @@ -3190,7 +3194,13 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> > > > > > return 0;
> > > > > > err_copy:
> > > > > > - ib_destroy_rwq_ind_table(rwq_ind_tbl);
> > > > > > + for (i = 0; i < num_wq_handles; i++)
> > > > > > + atomic_dec(&wqs[i]->usecnt);
> > > > > > +
> > > > > > + if (ib_dev->ops.destroy_rwq_ind_table)
> > > > > > + ib_dev->ops.destroy_rwq_ind_table(rwq_ind_tbl);
> > > > > > +err_create:
> > > > > > + kfree(rwq_ind_tbl);
> > > > > > err_uobj:
> > > > > > uobj_alloc_abort(uobj, attrs);
> > > > > > put_wqs:
> > > > > > @@ -4018,8 +4028,7 @@ const struct uapi_definition uverbs_def_write_intf[] = {
> > > > > > IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
> > > > > > ib_uverbs_ex_destroy_rwq_ind_table,
> > > > > > UAPI_DEF_WRITE_I(
> > > > > > - struct ib_uverbs_ex_destroy_rwq_ind_table),
> > > > > > - UAPI_DEF_METHOD_NEEDS_FN(destroy_rwq_ind_table))),
> > > > > > + struct ib_uverbs_ex_destroy_rwq_ind_table))),
> > > > > > DECLARE_UVERBS_OBJECT(
> > > > > > UVERBS_OBJECT_WQ,
> > > > > > diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
> > > > > > index e4994cc4cc51..cd82a5a80bdf 100644
> > > > > > --- a/drivers/infiniband/core/uverbs_std_types.c
> > > > > > +++ b/drivers/infiniband/core/uverbs_std_types.c
> > > > > > @@ -82,12 +82,21 @@ static int uverbs_free_rwq_ind_tbl(struct ib_uobject *uobject,
> > > > > > {
> > > > > > struct ib_rwq_ind_table *rwq_ind_tbl = uobject->object;
> > > > > > struct ib_wq **ind_tbl = rwq_ind_tbl->ind_tbl;
> > > > > > - int ret;
> > > > > > + u32 table_size = (1 << rwq_ind_tbl->log_ind_tbl_size);
> > > > > > + int ret = 0, i;
> > > > > > +
> > > > > > + if (atomic_read(&rwq_ind_tbl->usecnt))
> > > > > > + ret = -EBUSY;
> > > > > > - ret = ib_destroy_rwq_ind_table(rwq_ind_tbl);
> > > > > > if (ib_is_destroy_retryable(ret, why, uobject))
> > > > > > return ret;
> > > > > > + for (i = 0; i < table_size; i++)
> > > > > > + atomic_dec(&ind_tbl[i]->usecnt);
> > > > > > +
> > > > >
> > > > > Upon destroy we first expect to destroy the object that is pointing to other
> > > > > objetcs and only then descraese their ref count, this was the orignal logic
> > > > > here. (See also ib_destroy_qp_user(), ib_destroy_wq() and others).
> > > >
> > > > It is a bug, proper logic is to allocate HW object, elevate reference
> > > > counting, decrease reference counter and deallocate HW object.
> > > >
> > >
> > > I don't agree, only upon a succesfull destroy the we can go a ahead and
> > > safely decraese ref count from the pointed objects. This pevents any race
> > > from parallel desctrcution of the pointed object and consider any potenaial
> > > failure on the original object before decaresing ref counts as of in the QP
> > > case.
> >
> > There is no such thing successful destroy, from user perspective ALL
> > valid destroys should success.
>
> This is exactly the ref count purpsoe to prevent calling destoy on an object
> that may be pointed by other one, otherwise it will fail in the driver / FW.
It is hard to believe, at this point, the refcount decrease is done without
any relation to the real value, drivers doesn't check refcounts, or not supposed
too. The idea that FW will be always in sync with this value without locking
can't be true either.
More on that, it is not uobject refcount, but object usecnt which has slightly
different meaning.
>
> > Users don't know how to recover from
> > failure at this stage and they have one of two options: reboot the
> > server or leave memory/HW leak.
> >
> > There are not supposed to be parallel destructions if core code locked
> > everything properly and one of the threads decreased reference counter
> > to zero.
> >
>
> Right, this should be achived by a proer ref counting odering as done today.
I afraid that we are talking about different ref counters.
>
> > Can you please be more specific and give the scenario of your potential
> > flow?
> >
>
> The above clearly explain the motivation for current ref count usage.
Not really, at least to me.
>
> > >
> > > > Everything else should be fixed too.
> > >
> > > I beleive that this patch should be fixed and not all other places.
> > > By the way, see your previous patch from this series regarding MW that
> > > follows same logic, first dealloc_mw() and only then does atomic_dec() on
> > > the pointed PD.
> >
> > Of course, it was intended, it is completely different case and flow. PD holds
> > MW and this is why the reference counter is decreased only after MW was destroyed.
>
> PD doesn't hold MW, MW points to a PD upon its creation as of QP, MR and
> othres.
It doesn't matter as long as you understood my point that this is
different flow.
>
> >
> > >
> > > >
> > > > >
> > > > > > + if (rwq_ind_tbl->device->ops.destroy_rwq_ind_table)
> > > > > > + rwq_ind_tbl->device->ops.destroy_rwq_ind_table(rwq_ind_tbl);
> > > > >
> > > > > This doesn't really make sense, if no destroy function was set, the
> > > > > allocation should be prevented as well.
> > > >
> > > > Should we delete mlx4 RWQ implementation too?
> > > > After this refactoring, mlx4 won't have .destroy_rwq_ind_table.
> > > >
> > >
> > > We can expect from symetric reasons to have always alloc/destroy from
> > > drivers point of view. The IB layer will not check whether the destroy
> > > function exist but will always call it, the driver in its turn may do
> > > nothing.
> >
> > Maybe, I don't like empty functions in the drivers, it is silly.
> >
>
> We can consier that point, let's get other people feedback as well.
Sure
>
> Yishai
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme
2020-06-24 10:54 ` [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme Leon Romanovsky
2020-06-28 9:11 ` Yishai Hadas
@ 2020-06-29 15:39 ` Jason Gunthorpe
2020-06-30 7:21 ` Leon Romanovsky
1 sibling, 1 reply; 21+ messages in thread
From: Jason Gunthorpe @ 2020-06-29 15:39 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Doug Ledford, Leon Romanovsky, linux-rdma, Yishai Hadas
On Wed, Jun 24, 2020 at 01:54:22PM +0300, Leon Romanovsky wrote:
> @@ -4018,8 +4028,7 @@ const struct uapi_definition uverbs_def_write_intf[] = {
> IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
> ib_uverbs_ex_destroy_rwq_ind_table,
> UAPI_DEF_WRITE_I(
> - struct ib_uverbs_ex_destroy_rwq_ind_table),
> - UAPI_DEF_METHOD_NEEDS_FN(destroy_rwq_ind_table))),
> + struct ib_uverbs_ex_destroy_rwq_ind_table))),
Removing these is kind of troublesome.. This misses the one for ioctl:
UAPI_DEF_CHAIN_OBJ_TREE_NAMED(
UVERBS_OBJECT_RWQ_IND_TBL,
UAPI_DEF_OBJ_NEEDS_FN(destroy_rwq_ind_table)),
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index 65c9118a931c..4210f0842bc6 100644
> +++ b/drivers/infiniband/core/verbs.c
> @@ -1703,7 +1703,7 @@ static int _ib_modify_qp(struct ib_qp *qp, struct ib_qp_attr *attr,
> &old_sgid_attr_alt_av);
> if (ret)
> goto out_av;
> -
> +//
??
Jason
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme
2020-06-29 15:39 ` Jason Gunthorpe
@ 2020-06-30 7:21 ` Leon Romanovsky
2020-06-30 11:37 ` Jason Gunthorpe
0 siblings, 1 reply; 21+ messages in thread
From: Leon Romanovsky @ 2020-06-30 7:21 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Doug Ledford, linux-rdma, Yishai Hadas
On Mon, Jun 29, 2020 at 12:39:07PM -0300, Jason Gunthorpe wrote:
> On Wed, Jun 24, 2020 at 01:54:22PM +0300, Leon Romanovsky wrote:
> > @@ -4018,8 +4028,7 @@ const struct uapi_definition uverbs_def_write_intf[] = {
> > IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
> > ib_uverbs_ex_destroy_rwq_ind_table,
> > UAPI_DEF_WRITE_I(
> > - struct ib_uverbs_ex_destroy_rwq_ind_table),
> > - UAPI_DEF_METHOD_NEEDS_FN(destroy_rwq_ind_table))),
> > + struct ib_uverbs_ex_destroy_rwq_ind_table))),
>
> Removing these is kind of troublesome.. This misses the one for ioctl:
>
> UAPI_DEF_CHAIN_OBJ_TREE_NAMED(
> UVERBS_OBJECT_RWQ_IND_TBL,
> UAPI_DEF_OBJ_NEEDS_FN(destroy_rwq_ind_table)),
I will remove, but it seems that we have some gap here, I would expect
any sort of compilation error for mlx4.
>
> > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> > index 65c9118a931c..4210f0842bc6 100644
> > +++ b/drivers/infiniband/core/verbs.c
> > @@ -1703,7 +1703,7 @@ static int _ib_modify_qp(struct ib_qp *qp, struct ib_qp_attr *attr,
> > &old_sgid_attr_alt_av);
> > if (ret)
> > goto out_av;
> > -
> > +//
>
> ??
Interesting, I have no clue how it slipped, because I'm not using this
type of coding style even for debug.
Thanks
>
> Jason
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme
2020-06-30 7:21 ` Leon Romanovsky
@ 2020-06-30 11:37 ` Jason Gunthorpe
2020-06-30 11:52 ` Leon Romanovsky
0 siblings, 1 reply; 21+ messages in thread
From: Jason Gunthorpe @ 2020-06-30 11:37 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Doug Ledford, linux-rdma, Yishai Hadas
On Tue, Jun 30, 2020 at 10:21:37AM +0300, Leon Romanovsky wrote:
> On Mon, Jun 29, 2020 at 12:39:07PM -0300, Jason Gunthorpe wrote:
> > On Wed, Jun 24, 2020 at 01:54:22PM +0300, Leon Romanovsky wrote:
> > > @@ -4018,8 +4028,7 @@ const struct uapi_definition uverbs_def_write_intf[] = {
> > > IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
> > > ib_uverbs_ex_destroy_rwq_ind_table,
> > > UAPI_DEF_WRITE_I(
> > > - struct ib_uverbs_ex_destroy_rwq_ind_table),
> > > - UAPI_DEF_METHOD_NEEDS_FN(destroy_rwq_ind_table))),
> > > + struct ib_uverbs_ex_destroy_rwq_ind_table))),
> >
> > Removing these is kind of troublesome.. This misses the one for ioctl:
> >
> > UAPI_DEF_CHAIN_OBJ_TREE_NAMED(
> > UVERBS_OBJECT_RWQ_IND_TBL,
> > UAPI_DEF_OBJ_NEEDS_FN(destroy_rwq_ind_table)),
>
> I will remove, but it seems that we have some gap here, I would expect
> any sort of compilation error for mlx4.
Why would there be a compilation error?
And it should not be removed, it needs to be reworked to point to some
other function I suppose.
Jason
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme
2020-06-30 11:37 ` Jason Gunthorpe
@ 2020-06-30 11:52 ` Leon Romanovsky
2020-06-30 12:06 ` Jason Gunthorpe
0 siblings, 1 reply; 21+ messages in thread
From: Leon Romanovsky @ 2020-06-30 11:52 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Doug Ledford, linux-rdma, Yishai Hadas
On Tue, Jun 30, 2020 at 08:37:29AM -0300, Jason Gunthorpe wrote:
> On Tue, Jun 30, 2020 at 10:21:37AM +0300, Leon Romanovsky wrote:
> > On Mon, Jun 29, 2020 at 12:39:07PM -0300, Jason Gunthorpe wrote:
> > > On Wed, Jun 24, 2020 at 01:54:22PM +0300, Leon Romanovsky wrote:
> > > > @@ -4018,8 +4028,7 @@ const struct uapi_definition uverbs_def_write_intf[] = {
> > > > IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
> > > > ib_uverbs_ex_destroy_rwq_ind_table,
> > > > UAPI_DEF_WRITE_I(
> > > > - struct ib_uverbs_ex_destroy_rwq_ind_table),
> > > > - UAPI_DEF_METHOD_NEEDS_FN(destroy_rwq_ind_table))),
> > > > + struct ib_uverbs_ex_destroy_rwq_ind_table))),
> > >
> > > Removing these is kind of troublesome.. This misses the one for ioctl:
> > >
> > > UAPI_DEF_CHAIN_OBJ_TREE_NAMED(
> > > UVERBS_OBJECT_RWQ_IND_TBL,
> > > UAPI_DEF_OBJ_NEEDS_FN(destroy_rwq_ind_table)),
> >
> > I will remove, but it seems that we have some gap here, I would expect
> > any sort of compilation error for mlx4.
>
> Why would there be a compilation error?
I would expect BUILD_BUG_ON_ZERO() is thrown if ibdev_fn == NULL
>
> And it should not be removed, it needs to be reworked to point to some
> other function I suppose.
Why?
>
> Jason
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme
2020-06-30 11:52 ` Leon Romanovsky
@ 2020-06-30 12:06 ` Jason Gunthorpe
2020-06-30 12:13 ` Leon Romanovsky
0 siblings, 1 reply; 21+ messages in thread
From: Jason Gunthorpe @ 2020-06-30 12:06 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Doug Ledford, linux-rdma, Yishai Hadas
On Tue, Jun 30, 2020 at 02:52:24PM +0300, Leon Romanovsky wrote:
> On Tue, Jun 30, 2020 at 08:37:29AM -0300, Jason Gunthorpe wrote:
> > On Tue, Jun 30, 2020 at 10:21:37AM +0300, Leon Romanovsky wrote:
> > > On Mon, Jun 29, 2020 at 12:39:07PM -0300, Jason Gunthorpe wrote:
> > > > On Wed, Jun 24, 2020 at 01:54:22PM +0300, Leon Romanovsky wrote:
> > > > > @@ -4018,8 +4028,7 @@ const struct uapi_definition uverbs_def_write_intf[] = {
> > > > > IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
> > > > > ib_uverbs_ex_destroy_rwq_ind_table,
> > > > > UAPI_DEF_WRITE_I(
> > > > > - struct ib_uverbs_ex_destroy_rwq_ind_table),
> > > > > - UAPI_DEF_METHOD_NEEDS_FN(destroy_rwq_ind_table))),
> > > > > + struct ib_uverbs_ex_destroy_rwq_ind_table))),
> > > >
> > > > Removing these is kind of troublesome.. This misses the one for ioctl:
> > > >
> > > > UAPI_DEF_CHAIN_OBJ_TREE_NAMED(
> > > > UVERBS_OBJECT_RWQ_IND_TBL,
> > > > UAPI_DEF_OBJ_NEEDS_FN(destroy_rwq_ind_table)),
> > >
> > > I will remove, but it seems that we have some gap here, I would expect
> > > any sort of compilation error for mlx4.
> >
> > Why would there be a compilation error?
>
> I would expect BUILD_BUG_ON_ZERO() is thrown if ibdev_fn == NULL
??
> > And it should not be removed, it needs to be reworked to point to some
> > other function I suppose.
>
> Why?
The destroy function should not be registered at all if rwq_ind is not
supported by the driver - this is the methodlogy.
Jason
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH rdma-next 5/5] RDMA/core: Convert RWQ table logic to ib_core allocation scheme
2020-06-30 12:06 ` Jason Gunthorpe
@ 2020-06-30 12:13 ` Leon Romanovsky
0 siblings, 0 replies; 21+ messages in thread
From: Leon Romanovsky @ 2020-06-30 12:13 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Doug Ledford, linux-rdma, Yishai Hadas
On Tue, Jun 30, 2020 at 09:06:30AM -0300, Jason Gunthorpe wrote:
> On Tue, Jun 30, 2020 at 02:52:24PM +0300, Leon Romanovsky wrote:
> > On Tue, Jun 30, 2020 at 08:37:29AM -0300, Jason Gunthorpe wrote:
> > > On Tue, Jun 30, 2020 at 10:21:37AM +0300, Leon Romanovsky wrote:
> > > > On Mon, Jun 29, 2020 at 12:39:07PM -0300, Jason Gunthorpe wrote:
> > > > > On Wed, Jun 24, 2020 at 01:54:22PM +0300, Leon Romanovsky wrote:
> > > > > > @@ -4018,8 +4028,7 @@ const struct uapi_definition uverbs_def_write_intf[] = {
> > > > > > IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL,
> > > > > > ib_uverbs_ex_destroy_rwq_ind_table,
> > > > > > UAPI_DEF_WRITE_I(
> > > > > > - struct ib_uverbs_ex_destroy_rwq_ind_table),
> > > > > > - UAPI_DEF_METHOD_NEEDS_FN(destroy_rwq_ind_table))),
> > > > > > + struct ib_uverbs_ex_destroy_rwq_ind_table))),
> > > > >
> > > > > Removing these is kind of troublesome.. This misses the one for ioctl:
> > > > >
> > > > > UAPI_DEF_CHAIN_OBJ_TREE_NAMED(
> > > > > UVERBS_OBJECT_RWQ_IND_TBL,
> > > > > UAPI_DEF_OBJ_NEEDS_FN(destroy_rwq_ind_table)),
> > > >
> > > > I will remove, but it seems that we have some gap here, I would expect
> > > > any sort of compilation error for mlx4.
> > >
> > > Why would there be a compilation error?
> >
> > I would expect BUILD_BUG_ON_ZERO() is thrown if ibdev_fn == NULL
>
> ??
>
> > > And it should not be removed, it needs to be reworked to point to some
> > > other function I suppose.
> >
> > Why?
>
> The destroy function should not be registered at all if rwq_ind is not
> supported by the driver - this is the methodlogy.
And here comes mlx4 case that needs create() call but doesn't need
destroy(), because it will be empty after this refactoring.
Thanks
>
> Jason
^ permalink raw reply [flat|nested] 21+ messages in thread