On Mon, Nov 08, 2021 at 03:31:26PM -0800, Jakub Kicinski wrote:
On Mon, 8 Nov 2021 21:58:36 +0200 Leon Romanovsky wrote:
quoted
quoted
quoted
quoted
nfp will benefit from the simplified locking as well, and so will bnxt,
although I'm not sure the maintainers will opt for using devlink framework
due to the downstream requirements.
Exactly why devlink should be fixed first.
If by "fixed first" you mean it needs 5 locks to be added and to remove
any guarantees on sub-object lifetime then no thanks.
How do you plan to fix pernet_ops_rwsem lock? By exposing devlink state
to the drivers? By providing unlocked version of unregister_netdevice_notifier?
This simple scenario has deadlocks:
sudo ip netns add n1
sudo devlink dev reload pci/0000:00:09.0 netns n1
sudo ip netns del n1
Okay - I'm not sure why you're asking me this. This is not related to
devlink locking as far as I can tell. Neither are you fixing this
problem in your own RFC.
You'd need to tell me more about what the notifier is used for (I see
RoCE in the call trace). I don't understand why you need to re-register
a global (i.e. not per netns) notifier when devlink is switching name
spaces.
This becomes all entangled in the aux device stuff we did before.
devlink reload is defined, for reasons unrelated to netns, to do a
complete restart of the aux devices below the devlink. This happens
necessarily during actual reconfiguration operations, for instance.
So we have a situation, which seems like bad design, where reload is
also triggered by net namespace change that has nothing to do with
reconfiguring. In this case the per-net-ns becomes a BKL that gets
held across way too much stuff as it recuses down the reload path,
through aux devices, into the driver core and beyond.
When I looked at trying to fix this from the RDMA side I could not
find any remedy that didn't involve some kind of change in netdev
land. The drivers must be able to register/unregister notifiers in
their struct device_driver probe/remove functions.
I once sketched out fixing this by removing the need to hold the
per_net_rwsem just for list iteration, which in turn avoids holding it
over the devlink reload paths. It seemed like a reasonable step toward
finer grained locking.
Jason