Leon, Guoqing
On 18/04/2023 15:57, Leon Romanovsky wrote:
>>>> Currently, without this patch:
>>>> 1. PD and clt_path->s.dev are shared among connections.
>>>> 2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
>>>> 3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
>>>> clt_path->s.dev become zero, it will destroy PD.
>>>> 4. when con[1] failed to create, con[1] will not take clt_path->s.dev, but it try to decreased clt_path->s.dev <<< it's wrong to do that.
>>> So please fix it by making sure that failure to create con[1] will
>>> release resources which were allocated. If con[1] didn't increase
>>> s.dev_ref, it shouldn't decrease it either.
>> You are right, the current patch did exactly that.
>> It introduced a con owning flag 'has_dev' to indicate whether this con has taken s.dev.
>> so that its cleanup phase will only decrease its s.dev properly.
> The has_dev is a workaround and not a solution. In proper error unwind
> sequence, you won't need extra flag.
>
> Thanks
>
how about below changes
commit 61dba725384e226d472b8142d70d40d4103df87a
Author: Li Zhijian <lizhijian@xxxxxxxxxxx>
Date: Wed Apr 19 17:42:26 2023 +0800
RDMA/rtrs: Fix rxe_dealloc_pd warning
con[0] always sets s.dev to 1, correspondingly, we should let it to
release the last dev.
Previously,
1. PD and clt_path->s.dev are shared among connections.
2. every con[n]'s cleanup phase will call destroy_con_cq_qp()
3. clt_path->s.dev will be always decreased in destroy_con_cq_qp(), and when
clt_path->s.dev become zero, it will destroy PD.
4. when con[1] failed to create, con[1] will not take clt_path->s.dev,
but it try to decreased clt_path->s.dev <<< it's wrong to do that.
The warning occurs when destroying PD whose reference count is not zero.
Precodition: clt_path->s.con_num is 2.
So 2 cm connection will be created as below:
CPU0 CPU1
init_conns { |
create_cm() // a. con[0] created |
| a'. rtrs_clt_rdma_cm_handler() {
| rtrs_rdma_addr_resolved()
| create_con_cq_qp(con); << con[0]
| }
| in this moment, refcnt of PD was increased to 2+
|
create_cm() // b. cid = 1, failed |
destroy_con_cq_qp() |
rtrs_ib_dev_put() |
dev_free() |
ib_dealloc_pd(dev->ib_pd) << PD |
is destroyed, but refcnt is |
still greater than 0 |
}
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 80abf45a197a..1eb652dedca3 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -1743,6 +1743,15 @@ static void destroy_con_cq_qp(struct rtrs_clt_con *con)
con->rsp_ius = NULL;
con->queue_num = 0;
}
+
+ /*
+ * Every con will try to decreased s.dev_ref, but we should
+ * reserve the last s.dev_ref for con[0]. In case con[1+]'s
+ * cleanup phase call rtrs_ib_dev_put(clt_path->s.dev) early.
+ */
+ if (con->c.cid != 0 && clt_path->s.dev_ref == 1)
+ return;
+
if (clt_path->s.dev_ref && !--clt_path->s.dev_ref) {
rtrs_ib_dev_put(clt_path->s.dev);
clt_path->s.dev = NULL;
