Messages in this thread | | | Date | Wed, 8 Dec 2021 07:15:07 -0800 | From | Jakub Kicinski <> | Subject | Re: [PATCH net-next 0/6] Allow parallel devlink execution |
| |
On Wed, 8 Dec 2021 09:54:55 +0200 Leon Romanovsky wrote: > On Tue, Dec 07, 2021 at 08:21:14PM -0800, Jakub Kicinski wrote: > > On Tue, 7 Dec 2021 09:29:03 +0200 Leon Romanovsky wrote: > > > On Mon, Dec 06, 2021 at 06:00:27PM -0800, Jakub Kicinski wrote: > > > > On Sun, 5 Dec 2021 10:22:00 +0200 Leon Romanovsky wrote: > > > > > This is final piece of devlink locking puzzle, where I remove global > > > > > mutex lock (devlink_mutex), so we can run devlink commands in parallel. > > > > > > > > > > The series starts with addition of port_list_lock, which is needed to > > > > > prevent locking dependency between netdevsim sysfs and devlink. It > > > > > follows by the patch that adds context aware locking primitives. Such > > > > > primitives allow us to make sure that devlink instance is locked and > > > > > stays locked even during reload operation. The last patches opens > > > > > devlink to parallel commands. > > > > > > > > I'm not okay with assuming that all sub-objects are added when devlink > > > > is not registered. > > > > > > But none of the patches in this series assume that. > > > > > > In devlink_nested_lock() patch [1], I added new marker just to make sure > > > that we don't lock if this specific command is called in locked context. > > > > > > +#define DEVLINK_NESTED_LOCK XA_MARK_2 > > > > > > [1] https://lore.kernel.org/all/2b64a2a81995b56fec0231751ff6075020058584.1638690564.git.leonro@nvidia.com/ > > > > You skip locking if the marker is set. So a register operation can race > > with a user space operation, right? > > Not in upstream code. > > In upstream code, we call to devlink_*_register()/devlink_*_unregister() > routines in two possible flows: before/after registration or as a part > of user space request through netlink interface. We don't call to them > randomly.
me: this code does X Leon: no it doesn't me: but it clear does, here's why Leon: <convoluted evasive explanation>
I think it's going to be more healthy at this point to merge my code.
I do appreciate your work, but we disagree on how the API should look.
> The current code is intermediate solution that allows us to get rid from > devlink_mutex lock together with annotations that help to spot problematic > flows. > > In next patches, I will: > 1. Reduce scope of devlink->lock to make sure that it locks exactly what > is needed to be protected (linked lists) instead of all-in-one lock as > it is now. > 2. Rename devlink->lock to be evlink->lists_lock to clear the mud around > the scope. > 3. Untangle mess with pre_doit, where some commands set _FLAG_NEED_* > flags and ignore user_ptr[1]. Every command should take internally the > object they need without any flags. It will make sub-object management > more clear. > 4. Push down the mutex_lock(&devlink->lock) pre_doit to actual commands, > so pre_doit won't take any locks at all. > 5. Reference count objects or use write semaphore in uregister paths to > make sure that we can access sub-objects without locks. I'm not sure > about the final implementations details yet. > > In the steps 3, 4 and 5, we will delete _nested_lock, pre/post doit mess > and make sure that commands are holding as less as possible locks. > > I afraid that many here are underestimate the amount of work needed that is > needed in devlink area to clean the rust due-to mixing in-kernel with > user-visible APIs.
| |