Messages in this thread | | | Date | Mon, 8 Nov 2021 20:24:37 +0200 | From | Leon Romanovsky <> | Subject | Re: [PATCH net-next] devlink: Require devlink lock during device reload |
| |
On Mon, Nov 08, 2021 at 10:16:46AM -0800, Jakub Kicinski wrote: > On Mon, 8 Nov 2021 19:32:19 +0200 Leon Romanovsky wrote: > > > I think it's common sense. We're just exporting something to make our > > > lives easier somewhere else in the three. Do you see a way in which > > > taking refs on devlink can help out-of-tree code? > > > > I didn't go such far in my thoughts. My main concern is that you ore > > exposing broken devlink internals in the hope that drivers will do better > > locking. I wanted to show that internal locking should be fixed first. > > > > https://lore.kernel.org/netdev/cover.1636390483.git.leonro@nvidia.com/T/#m093f067d0cafcbe6c05ed469bcfd708dd1eb7f36 > > > > While this series fixes locking and after all my changes devlink started > > to be more secure, that works correctly for simple drivers. > > I prefer my version. I think I asked you to show how the changes make > drivers simpler, which you failed to do.
Why did I fail? My version requires **zero** changes to the drivers. Everything works without them changing anything. You can't ask for more.
> > I already told you how this is going to go, don't expect me to comment > too much. > > > However for net namespace aware drivers it still stays DOA. > > > > As you can see, devlink reload holds pernet_ops_rwsem, which drivers should > > take too in order to unregister_netdevice_notifier. > > > > So for me, the difference between netdevsim and real device (mlx5) is > > too huge to really invest time into netdevsim-centric API, because it > > won't solve any of real world problems. > > Did we not already go over this? Sorry, it feels like you're repeating > arguments which I replied to before. This is exhausting.
I don't enjoy it either.
> > 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.
Thanks
| |