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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 19/04/2023 21:20, Jinpu Wang wrote:
> On Wed, Apr 19, 2023 at 11:53 AM Zhijian Li (Fujitsu)
> <lizhijian@xxxxxxxxxxx> wrote:
>>
>> 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;
> 

Jinpu, thanks for your testing.

Indeed, above changes are not correct. it breaks the normal cleanup like below:
for (i=0; i < N, i++)
   destroy_con_cq_qp(con[i])


Thanks
Zhijian

> I run a regression test in our test env, it triggers a warning on
> 
> 1681                 if (WARN_ON(clt_path->s.dev))
> 
> [ 1333.042633] ------------[ cut here ]------------
> [ 1333.042650] WARNING: CPU: 8 PID: 559 at
> /root/kernel-test/ibnbd2/rtrs/rtrs-clt.c:1681
> rtrs_clt_rdma_cm_handler+0x864/0x8a0 [rtrs_client]
> [ 1333.042651] Modules linked in: loop rnbd_client(O) rtrs_client(O)
> rtrs_core(O) kvm_amd kvm input_leds led_class irqbypass crc32_pclmul
> aesni_intel sp5100_tco evdev libaes watchdog sg k10temp crypto_simd
> fam15h_power ipmi_si serio_raw cryptd ipmi_devintf glue_helper
> ipmi_msghandler acpi_cpufreq button ib_ipoib ib_umad null_blk brd
> rdma_cm iw_cm ib_cm ip_tables x_tables autofs4 raid10 raid456
> async_raid6_recov async_memcpy async_pq async_xor async_tx xor
> raid6_pq libcrc32c raid1 raid0 linear mlx4_ib md_mod ib_uverbs ib_core
> sd_mod t10_pi crc_t10dif crct10dif_generic ahci libahci
> crct10dif_pclmul crct10dif_common crc32c_intel igb libata usb_storage
> psmouse i2c_piix4 i2c_algo_bit mlx4_core dca scsi_mod i2c_core ptp
> pps_core
> [ 1333.042737] CPU: 8 PID: 559 Comm: kworker/u128:1 Tainted: G
>    O      5.10.136-pserver-develop-5.10 #257
> [ 1333.042738] Hardware name: Supermicro H8QG6/H8QG6, BIOS 3.00       09/04/2012
> [ 1333.042752] Workqueue: rdma_cm cma_work_handler [rdma_cm]
> [ 1333.042758] RIP: 0010:rtrs_clt_rdma_cm_handler+0x864/0x8a0 [rtrs_client]
> [ 1333.042761] Code: ff bb ea ff ff ff e8 db a5 24 fc 49 8d b4 24 10
> 01 00 00 89 da 48 c7 c7 40 93 5b c0 e8 4b 47 21 fc 4d 8b 65 00 e9 15
> fe ff ff <0f> 0b 4c 89 ff bb ea ff ff ff e8 ad a5 24 fc eb d0 0f 0b 4c
> 89 ff
> [ 1333.042763] RSP: 0018:ffffaff68e57bdb0 EFLAGS: 00010286
> [ 1333.042765] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff9eddc0051420
> [ 1333.042767] RDX: ffff9ee4ef716e40 RSI: ffff9f14ea288f30 RDI: ffff9eddc88db240
> [ 1333.042768] RBP: ffffaff68e57be50 R08: 0000000000000000 R09: 006d635f616d6472
> [ 1333.042769] R10: ffffaff68e57be68 R11: 0000000000000000 R12: ffff9edde1388000
> [ 1333.042771] R13: ffff9eddc88db200 R14: ffff9edde1388000 R15: ffff9eddc88db240
> [ 1333.042773] FS:  0000000000000000(0000) GS:ffff9eecc7c00000(0000)
> knlGS:0000000000000000
> [ 1333.042774] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1333.042776] CR2: 00007f0ac4ed4004 CR3: 0000002b5040a000 CR4: 00000000000406e0
> [ 1333.042777] Call Trace:
> [ 1333.042790]  ? newidle_balance+0x25e/0x3c0
> [ 1333.042795]  ? psi_group_change+0x43/0x230
> [ 1333.042801]  ? cma_cm_event_handler+0x23/0xb0 [rdma_cm]
> [ 1333.042807]  cma_cm_event_handler+0x23/0xb0 [rdma_cm]
> [ 1333.042814]  cma_work_handler+0x5a/0xb0 [rdma_cm]
> [ 1333.042819]  process_one_work+0x1f3/0x390
> [ 1333.042822]  worker_thread+0x2d/0x3c0




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux