Tue, Nov 09, 2021 at 04:07:02PM CET, kuba@kernel.org wrote:
On Tue, 9 Nov 2021 10:43:58 -0400 Jason Gunthorpe wrote:
quoted
This becomes all entangled in the aux device stuff we did before.
So entangled in fact that neither of you is willing to elucidate
the exact need ;)
quoted
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.
Agreed, it is somewhat uncomfortable that the same callback achieves
two things. As clear as the need for reload-for-reset is (reconfig,
recovery etc.) I'm not as clear on reload for netns.
The main use case for reload for netns is placing a VF in a namespace,
for a container to use. Is that right? I've not seen use cases
requiring the PF to be moved, are there any?
devlink now lives in a networking namespace yet it spans such
namespaces (thru global notifiers). I think we need to define what it
means for devlink to live in a namespace. Is it just about the
configuration / notification channel? Or do we expect proper isolation?
Jiri?
Well honestly the primary motivation was to be able to run smoothly with
syzkaller for which the "configuration / notification channel" is
enough.
By "proper isolation" you mean what exactly?
quoted
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.
Seems to me the locking is just a symptom.