Re: [PATCH for-next v7 10/12] RDMA/erdma: Add the erdma module

[Cheng Xu <chengyou@linux.alibaba.com>]

On 5/16/22 10:07 PM, Jason Gunthorpe wrote:
> On Mon, May 16, 2022 at 09:59:28PM +0800, Cheng Xu wrote:
>>
>>
>> On 5/16/22 8:40 PM, Cheng Xu wrote:
>>>
>>>
>>> On 5/16/22 7:49 PM, Jason Gunthorpe wrote:
>>>> On Mon, May 16, 2022 at 11:15:32AM +0800, Cheng Xu wrote:
>>>>>
>>>>>
>>>>> On 5/10/22 9:17 PM, Jason Gunthorpe wrote:
>>>>>> On Thu, Apr 21, 2022 at 03:17:45PM +0800, Cheng Xu wrote:
>>>>>>
>>>>>>> +static struct rdma_link_ops erdma_link_ops = {
>>>>>>> +    .type = "erdma",
>>>>>>> +    .newlink = erdma_newlink,
>>>>>>> +};
>>>>>>
>>>>>> Why is there still a newlink?
>>>>>>
>>>>>> Jason
>>>>>
>>>>>
>>>>> Yeah, I remember your suggestion that the ibdev should keep the same
>>>>> lifecycle with its PCI device, and so does it now.
>>>>>
>>>>> The initialization flow for erdma now:
>>>>>        probe:
>>>>>          - Hardware specified initialization
>>>>>          - IB device initialization
>>>>>          - Calling ib_register_device with ibdev->netdev == NULL
>>>>>
>>>>> After probe, The ib device has been registered, but we left it in
>>>>> invalid state.
>>>>> To fully complete the initialization, we should set the ibdev->netdev.
>>>>>
>>>>> And the newlink command in erdma only do one thing now: set the
>>>>> ibdev->netdev if the rule matches, and it is uncorrelated with the
>>>>> ib device lifecycle.
>>>>
>>>> This is not what newlink is for, it can't be used like this
>>>>
>>>> Jason
>>>
>>>
>>> [A typo in previous reply, so I re-edit it]
>>>
>> <...>
>>> Or, Is it ok that erdma registers a net notifier in our module to handle
>>> this?
>>>
>>
>> I think this twice, use a net notifier of module can not solve this, due
>> to the leak of all ib devices information in erdma module.
> 
> I don't understand what this means
At first I want to register a global net notifier in our module, and 
link the ib device and net device if matches in the callback. But in the
callback, we can not get the ib devices registered by erdma, because our
driver module does not maintain the ib device list. This is not the
right way.

>> The solution may be simple: register net notifier per device in probe,
>> and call ib_device_set_netdev if matches in the notifier callback.
> 
> That sounds wrong
> 

OK. So using rdma link command to handle the link relationship manually
is still a better choice？

Actually, erdma looks like a hardware implementation of siw/rxe (more
likes siw, because erdma uses iWarp, and have similar CM
implementation): the ethernet functionality is provided by existed net
device, no matter what kind of the net device (due to virtio-net is
de-facto in cloud virtualization, erdma is binded to virtio-net in
practice). Due to this similarity, siw/rxe use 'rdma link' to link the
ib device and net device (including create ib device), Does it sound
reasonable that erdma use it to link the net deivce?

Could you re-consider this way? Or could you give us some suggestion? I
think this is the main and important issue for our upstreaming. I would
be very grateful.

Thanks,
Cheng Xu