Re: [PATCH v3 10/14] RDMA/ionic: Register device ops for control path

[Abhijit Gangurde <abhijit.gangurde@amd.com>]

On 7/7/25 22:16, Leon Romanovsky wrote:
> On Mon, Jul 07, 2025 at 08:26:20PM +0530, Abhijit Gangurde wrote:
>> On 7/7/25 12:51, Leon Romanovsky wrote:
>>> On Mon, Jul 07, 2025 at 10:57:13AM +0530, Abhijit Gangurde wrote:
>>>> On 7/4/25 22:38, Leon Romanovsky wrote:
>>>>> On Thu, Jul 03, 2025 at 12:49:30PM +0530, Abhijit Gangurde wrote:
>>>>>> On 7/2/25 23:30, Leon Romanovsky wrote:
>>>>>>> On Wed, Jul 02, 2025 at 10:18:03AM -0300, Jason Gunthorpe wrote:
>>>>>>>> On Tue, Jul 01, 2025 at 01:38:44PM +0300, Leon Romanovsky wrote:
>>>>>>>>>> +static void ionic_flush_qs(struct ionic_ibdev *dev)
>>>>>>>>>> +{
>>>>>>>>>> +	struct ionic_qp *qp, *qp_tmp;
>>>>>>>>>> +	struct ionic_cq *cq, *cq_tmp;
>>>>>>>>>> +	LIST_HEAD(flush_list);
>>>>>>>>>> +	unsigned long index;
>>>>>>>>>> +
>>>>>>>>>> +	/* Flush qp send and recv */
>>>>>>>>>> +	rcu_read_lock();
>>>>>>>>>> +	xa_for_each(&dev->qp_tbl, index, qp) {
>>>>>>>>>> +		kref_get(&qp->qp_kref);
>>>>>>>>>> +		list_add_tail(&qp->ibkill_flush_ent, &flush_list);
>>>>>>>>>> +	}
>>>>>>>>>> +	rcu_read_unlock();
>>>>>>>>> Same question as for CQ. What does RCU lock protect here?
>>>>>>>> It should protect the kref_get against free of qp. The qp memory must
>>>>>>>> be RCU freed.
>>>>>>> I'm not sure that this was intension here. Let's wait for an answer from the author.
>>>>>> As Jason mentioned, It was intended to protect the kref_get against free of
>>>>>> cq and qp
>>>>>> in the destroy path.
>>>>> How is it possible? IB/core is supposed to protect from accessing verbs
>>>>> resources post their release/destroy.
>>>>>
>>>>> After you answered what RCU is protecting, I don't see why you would
>>>>> have custom kref over QP/CQ/e.t.c objects.
>>>>>
>>>>> Thanks
>>>> The RCU protected kref here is making sure that all the hw events are
>>>> processed before destroy callback returns. Similarly, when driver is
>>>> going for ib_unregister_device, it is draining the pending WRs and events.
>>> I asked why do you have kref in first place? When ib_unregister_device
>>> is called all "pending MR" already supposed to be destroyed.
>>>
>>> Thansk
>> The custom kref on QP/CQ object is holding the completion for the destroy
>> callback.
>> If any pending async hw events are being processed, destroy would wait on
>> this completion
>> before it returns.
> Please see how other drivers avoid such situation. There is no need in
> custom kref.
>
> Thanks

As per your suggestion, I looked some of the other RDMA drivers. While 
many are using locks, that approach would negate the lockless lookup we 
gain from the xarray.
The MANA RDMA driver, for instance, uses a similar refcount and 
completion mechanism to handle asynchronous events.

Thanks