Re: [PATCH] RDMA/siw: Remove direct link to net_device

[Zhu Yanjun <yanjun.zhu@linux.dev>]

在 2024/12/12 11:20, Bernard Metzler 写道:
> 
> 
>> -----Original Message-----
>> From: Leon Romanovsky <leon@kernel.org>
>> Sent: Tuesday, December 10, 2024 8:13 PM
>> To: Bernard Metzler <BMT@zurich.ibm.com>
>> Cc: Jason Gunthorpe <jgg@ziepe.ca>; linux-rdma@vger.kernel.org
>> Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Remove direct link to net_device
>>
>> On Tue, Dec 10, 2024 at 05:08:51PM +0000, Bernard Metzler wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jason Gunthorpe <jgg@ziepe.ca>
>>>> Sent: Tuesday, December 10, 2024 3:56 PM
>>>> To: Bernard Metzler <BMT@zurich.ibm.com>
>>>> Cc: linux-rdma@vger.kernel.org; leon@kernel.org; linux-
>>>> kernel@vger.kernel.org; netdev@vger.kernel.org; syzkaller-
>>>> bugs@googlegroups.com; zyjzyj2000@gmail.com;
>>>> syzbot+4b87489410b4efd181bf@syzkaller.appspotmail.com
>>>> Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Remove direct link to
>> net_device
>>>>
>>>> On Tue, Dec 10, 2024 at 02:03:51PM +0100, Bernard Metzler wrote:
>>>>> diff --git a/drivers/infiniband/sw/siw/siw.h
>>>> b/drivers/infiniband/sw/siw/siw.h
>>>>> index 86d4d6a2170e..c8f75527b513 100644
>>>>> --- a/drivers/infiniband/sw/siw/siw.h
>>>>> +++ b/drivers/infiniband/sw/siw/siw.h
>>>>> @@ -69,16 +69,19 @@ struct siw_pd {
>>>>>
>>>>>   struct siw_device {
>>>>>   	struct ib_device base_dev;
>>>>> -	struct net_device *netdev;
>>>>>   	struct siw_dev_cap attrs;
>>>>>
>>>>>   	u32 vendor_part_id;
>>>>> +	struct {
>>>>> +		int ifindex;
>>>>
>>>> ifindex is only stable so long as you are holding a reference on the
>>>> netdev..
>>>>> --- a/drivers/infiniband/sw/siw/siw_main.c
>>>>> +++ b/drivers/infiniband/sw/siw/siw_main.c
>>>>> @@ -287,7 +287,6 @@ static struct siw_device
>> *siw_device_create(struct
>>>> net_device *netdev)
>>>>>   		return NULL;
>>>>>
>>>>>   	base_dev = &sdev->base_dev;
>>>>> -	sdev->netdev = netdev;
>>>>
>>>> Like here needed to grab a reference before storing the pointer in the
>>>> sdev struct.
>>>>
>>>
>>> This patch was supposed to remove siw's link to netdev. So no
>>> reference to netdev would be needed. I did it under the
>>> assumption siw can locally keep all needed information up to
>>> date via netdev_event().
>>> But it seems the netdev itself can change during the lifetime of
>>> a siw device? With that ifindex would become wrong.
>>>
>>> If the netdev can change without the siw driver being informed,
>>> holding a netdev pointer or reference seems useless.
>>>
>>> So it would be best to always use ib_device_get_netdev() to get
>>> a valid netdev pointer, if current netdev info is needed by the
>>> driver?
>>
>> Or call to dev_hold(netdev) in siw_device_create(). It will make sure
>> that netdev is stable.
>>
>> Thanks
>>
>> BTW, Need to check, maybe IB/core layer already called to dev_hold.
>>
> 
> Yes, drivers are calling ib_device_set_netdev(ibdev, netdev, port)
> during newlink(), which assigns the netdev to the ib_device's port
> info. The ibdev takes a hold on the ports netdev and handles the
> pointer assignment, clearing etc. appropriately. Unlinking is done
> via ib_device_set_netdev(, NULL, ), or ib_unregister_device()
> which unlinks and puts netdevs as well.
> 
> Given we have an instance taking care of the netdev, it is
> probably best to remove all direct netdev references from the
> driver - just to avoid replicating all its complex handling.
> So siw will use ib_device_get_netdev() whenever netdev info
> is required. It comes with some extra overhead, but its's more
> consistent.
> 
> BTW: The rdma_link interface is asymmetric, lacking an unlink()
> call. For using software only drivers it might be beneficial to
> allow explicit unlinking a driver from a device. Any thoughts
> on that?

I agree with you. In the following link, I add del_link call. If you all 
agree, I can separate this patch from the patchset 
https://patchwork.kernel.org/project/linux-rdma/cover/20230624073927.707915-1-yanjun.zhu@intel.com/.

This is the link to add unlink call.

https://patchwork.kernel.org/project/linux-rdma/patch/20230624073927.707915-4-yanjun.zhu@intel.com/

Zhu Yanjun

> 
> Best,
> Bernard.
>>>
>>> I just wanted to avoid the costly ib_device_get_netdev() call
>>> during query_qp(), query_port() and listen().
>>>
>>> Thank you!
>>> Bernard.
>>>