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:

quoted

On Mon, Dec 06, 2021 at 06:00:27PM -0800, Jakub Kicinski wrote:

quoted

On Sun,  5 Dec 2021 10:22:00 +0200 Leon Romanovsky wrote:  

quoted

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/ (local)

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.

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. 

Thanks