Re: [PATCH for-next 2/3] RDMA/rtrs: Fix rxe_dealloc_pd warning

[Zhijian Li (Fujitsu)]

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;