Re: [PATCH v3] iwcm: don't hold the irq disabled lock on iw_rem_ref

[Sagi Grimberg <sagi@grimberg.me>]

>> To: "Krishnamraju Eraparaju" <krishna2@chelsio.com>
>> From: "Jason Gunthorpe" <jgg@ziepe.ca>
>> Date: 09/16/2019 06:28PM
>> Cc: "Steve Wise" <larrystevenwise@gmail.com>, "Bernard Metzler"
>> <BMT@zurich.ibm.com>, "Sagi Grimberg" <sagi@grimberg.me>,
>> "linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>
>> Subject: [EXTERNAL] Re: Re: [PATCH v3] iwcm: don't hold the irq
>> disabled lock on iw_rem_ref
>>
>> On Wed, Sep 11, 2019 at 09:28:16PM +0530, Krishnamraju Eraparaju
>> wrote:
>>> Hi Steve & Bernard,
>>>
>>> Thanks for the review comments.
>>> I will do those formating changes.
>>
>> I don't see anything in patchworks, but the consensus is to drop
>> Sagi's patch pending this future patch?
>>
>> Jason
>>
> This is my impression as well. But consensus should be
> explicit...Sagi, what do you think?

I don't really care, but given the changes from Krishnamraju cause other
problems I'd ask if my version is also offending his test.

In general, I do not think that making resources free routines (both
explict or implicit via ref dec) under a spinlock is not a robust
design.

I would first make it clear and documented what cm_id_priv->lock is
protecting. In my mind, it should protect *its own* mutations of
cm_id_priv and by design leave all the ops calls outside the lock.

I don't understand what is causing the Chelsio issue observed, but
it looks like c4iw_destroy_qp blocks on a completion that depends on a
refcount that is taken also by iwcm, which means that I cannot call
ib_destroy_qp if the cm is not destroyed as well?

If that is madatory, I'd say that instead of blocking on this
completion, we can simply convert c4iw_qp_rem_ref if use a kref
which is not order dependent.