在 2024/12/12 11:20, Bernard Metzler 写道:
-----Original Message-----
From: Leon Romanovsky <leon@xxxxxxxxxx>
Sent: Tuesday, December 10, 2024 8:13 PM
To: Bernard Metzler <BMT@xxxxxxxxxxxxxx>
Cc: Jason Gunthorpe <jgg@xxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx
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@xxxxxxxx>
Sent: Tuesday, December 10, 2024 3:56 PM
To: Bernard Metzler <BMT@xxxxxxxxxxxxxx>
Cc: linux-rdma@xxxxxxxxxxxxxxx; leon@xxxxxxxxxx; linux-
kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; syzkaller-
bugs@xxxxxxxxxxxxxxxx; zyjzyj2000@xxxxxxxxx;
syzbot+4b87489410b4efd181bf@xxxxxxxxxxxxxxxxxxxxxxxxx
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@xxxxxxxxx/.
This is the link to add unlink call.
https://patchwork.kernel.org/project/linux-rdma/patch/20230624073927.707915-4-yanjun.zhu@xxxxxxxxx/
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.
