[PATCH net-next 0/6] Allow parallel devlink execution

[PATCH net-next 0/6] Allow parallel devlink execution

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Hi,

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.

It tested on mlx5, mlx4, mlxsw, netdevsim + Ido's syzkaller.

BTW, in the future patches, we will clean devlink from over-engineered
APIs and implementations. It will open a venue to revise all the places
that hold devlink lock, if they really need to do it.

Thanks

[1] https://lore.kernel.org/netdev/cover.1636390483.git.leonro@nvidia.com/
[2] https://lore.kernel.org/netdev/9716f9a13e217a0a163b745b6e92e02d40973d2c.1635701665.git.leonro@nvidia.com/

Leon Romanovsky (6):
  devlink: Clean registration of devlink port
  devlink: Be explicit with devlink port protection
  devlink: Add devlink nested locking primitive
  devlink: Require devlink lock during device reload
  devlink: Use xarray locking mechanism instead big devlink lock
  devlink: Open devlink to parallel operations

 net/core/devlink.c | 432 ++++++++++++++++++++++++---------------------
 1 file changed, 232 insertions(+), 200 deletions(-)

-- 
2.33.1

[PATCH net-next 1/6] devlink: Clean registration of devlink port

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

devlink_port_register() is in-kernel API and as such can't really
fail as long as driver author didn't make a mistake by providing
already existing port index.

Instead of relying on various error prints from the driver, convert
the existence check to be WARN_ON(), so such a mistake will be caught
immediately.

Tested-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 net/core/devlink.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index db3b52110cf2..34d0f623b2a9 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -268,12 +268,6 @@ static struct devlink_port *devlink_port_get_by_index(struct devlink *devlink,
 	return NULL;
 }
 
-static bool devlink_port_index_exists(struct devlink *devlink,
-				      unsigned int port_index)
-{
-	return devlink_port_get_by_index(devlink, port_index);
-}
-
 static struct devlink_port *devlink_port_get_from_attrs(struct devlink *devlink,
 							struct nlattr **attrs)
 {
@@ -9255,25 +9249,24 @@ int devlink_port_register(struct devlink *devlink,
 			  struct devlink_port *devlink_port,
 			  unsigned int port_index)
 {
-	mutex_lock(&devlink->lock);
-	if (devlink_port_index_exists(devlink, port_index)) {
-		mutex_unlock(&devlink->lock);
-		return -EEXIST;
-	}
-
+	WARN_ON(devlink_port_get_by_index(devlink, port_index));
 	WARN_ON(devlink_port->devlink);
+
 	devlink_port->devlink = devlink;
 	devlink_port->index = port_index;
 	spin_lock_init(&devlink_port->type_lock);
 	INIT_LIST_HEAD(&devlink_port->reporter_list);
 	mutex_init(&devlink_port->reporters_lock);
-	list_add_tail(&devlink_port->list, &devlink->port_list);
 	INIT_LIST_HEAD(&devlink_port->param_list);
 	INIT_LIST_HEAD(&devlink_port->region_list);
-	mutex_unlock(&devlink->lock);
 	INIT_DELAYED_WORK(&devlink_port->type_warn_dw, &devlink_port_type_warn);
-	devlink_port_type_warn_schedule(devlink_port);
+
+	mutex_lock(&devlink->lock);
+	list_add_tail(&devlink_port->list, &devlink->port_list);
+	mutex_unlock(&devlink->lock);
+
 	devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
+	devlink_port_type_warn_schedule(devlink_port);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(devlink_port_register);
-- 
2.33.1

[PATCH net-next 2/6] devlink: Be explicit with devlink port protection

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Devlink port flows use devlink instance lock to protect from parallel
addition and deletion from port_list. So instead of using global lock,
let's introduce specific protection lock with much more clear scope
that will protect port_list.

Tested-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 net/core/devlink.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 34d0f623b2a9..eddd554b50d4 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -41,6 +41,8 @@ struct devlink_dev_stats {
 struct devlink {
 	u32 index;
 	struct list_head port_list;
+	/* Protect add/delete operations of devlink_port */
+	struct mutex port_list_lock;
 	struct list_head rate_list;
 	struct list_head sb_list;
 	struct list_head dpipe_table_list;
@@ -60,7 +62,7 @@ struct devlink {
 	struct device *dev;
 	possible_net_t _net;
 	/* Serializes access to devlink instance specific objects such as
-	 * port, sb, dpipe, resource, params, region, traps and more.
+	 * sb, dpipe, resource, params, region, traps and more.
 	 */
 	struct mutex lock;
 	u8 reload_failed:1;
@@ -1373,6 +1375,7 @@ static int devlink_nl_cmd_port_get_dumpit(struct sk_buff *msg,
 			goto retry;
 
 		mutex_lock(&devlink->lock);
+		mutex_lock(&devlink->port_list_lock);
 		list_for_each_entry(devlink_port, &devlink->port_list, list) {
 			if (idx < start) {
 				idx++;
@@ -1384,12 +1387,14 @@ static int devlink_nl_cmd_port_get_dumpit(struct sk_buff *msg,
 						   cb->nlh->nlmsg_seq,
 						   NLM_F_MULTI, cb->extack);
 			if (err) {
+				mutex_unlock(&devlink->port_list_lock);
 				mutex_unlock(&devlink->lock);
 				devlink_put(devlink);
 				goto out;
 			}
 			idx++;
 		}
+		mutex_unlock(&devlink->port_list_lock);
 		mutex_unlock(&devlink->lock);
 retry:
 		devlink_put(devlink);
@@ -4977,6 +4982,7 @@ static int devlink_nl_cmd_port_param_get_dumpit(struct sk_buff *msg,
 			goto retry;
 
 		mutex_lock(&devlink->lock);
+		mutex_lock(&devlink->port_list_lock);
 		list_for_each_entry(devlink_port, &devlink->port_list, list) {
 			list_for_each_entry(param_item,
 					    &devlink_port->param_list, list) {
@@ -4994,6 +5000,7 @@ static int devlink_nl_cmd_port_param_get_dumpit(struct sk_buff *msg,
 				if (err == -EOPNOTSUPP) {
 					err = 0;
 				} else if (err) {
+					mutex_unlock(&devlink->port_list_lock);
 					mutex_unlock(&devlink->lock);
 					devlink_put(devlink);
 					goto out;
@@ -5001,6 +5008,7 @@ static int devlink_nl_cmd_port_param_get_dumpit(struct sk_buff *msg,
 				idx++;
 			}
 		}
+		mutex_unlock(&devlink->port_list_lock);
 		mutex_unlock(&devlink->lock);
 retry:
 		devlink_put(devlink);
@@ -5531,13 +5539,15 @@ static int devlink_nl_cmd_region_get_devlink_dumpit(struct sk_buff *msg,
 		(*idx)++;
 	}
 
+	mutex_lock(&devlink->port_list_lock);
 	list_for_each_entry(port, &devlink->port_list, list) {
 		err = devlink_nl_cmd_region_get_port_dumpit(msg, cb, port, idx,
 							    start);
 		if (err)
-			goto out;
+			goto out_port;
 	}
-
+out_port:
+	mutex_unlock(&devlink->port_list_lock);
 out:
 	mutex_unlock(&devlink->lock);
 	return err;
@@ -7305,6 +7315,7 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
 			goto retry_port;
 
 		mutex_lock(&devlink->lock);
+		mutex_lock(&devlink->port_list_lock);
 		list_for_each_entry(port, &devlink->port_list, list) {
 			mutex_lock(&port->reporters_lock);
 			list_for_each_entry(reporter, &port->reporter_list, list) {
@@ -7319,6 +7330,7 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
 					cb->nlh->nlmsg_seq, NLM_F_MULTI);
 				if (err) {
 					mutex_unlock(&port->reporters_lock);
+					mutex_unlock(&devlink->port_list_lock);
 					mutex_unlock(&devlink->lock);
 					devlink_put(devlink);
 					goto out;
@@ -7327,6 +7339,7 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
 			}
 			mutex_unlock(&port->reporters_lock);
 		}
+		mutex_unlock(&devlink->port_list_lock);
 		mutex_unlock(&devlink->lock);
 retry_port:
 		devlink_put(devlink);
@@ -9029,6 +9042,7 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
 	xa_init_flags(&devlink->snapshot_ids, XA_FLAGS_ALLOC);
 	write_pnet(&devlink->_net, net);
 	INIT_LIST_HEAD(&devlink->port_list);
+	mutex_init(&devlink->port_list_lock);
 	INIT_LIST_HEAD(&devlink->rate_list);
 	INIT_LIST_HEAD(&devlink->sb_list);
 	INIT_LIST_HEAD_RCU(&devlink->dpipe_table_list);
@@ -9190,6 +9204,7 @@ void devlink_free(struct devlink *devlink)
 	WARN_ON(!list_empty(&devlink->dpipe_table_list));
 	WARN_ON(!list_empty(&devlink->sb_list));
 	WARN_ON(!list_empty(&devlink->rate_list));
+	mutex_destroy(&devlink->port_list_lock);
 	WARN_ON(!list_empty(&devlink->port_list));
 
 	xa_destroy(&devlink->snapshot_ids);
@@ -9261,9 +9276,9 @@ int devlink_port_register(struct devlink *devlink,
 	INIT_LIST_HEAD(&devlink_port->region_list);
 	INIT_DELAYED_WORK(&devlink_port->type_warn_dw, &devlink_port_type_warn);
 
-	mutex_lock(&devlink->lock);
+	mutex_lock(&devlink->port_list_lock);
 	list_add_tail(&devlink_port->list, &devlink->port_list);
-	mutex_unlock(&devlink->lock);
+	mutex_unlock(&devlink->port_list_lock);
 
 	devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
 	devlink_port_type_warn_schedule(devlink_port);
@@ -9282,9 +9297,9 @@ void devlink_port_unregister(struct devlink_port *devlink_port)
 
 	devlink_port_type_warn_cancel(devlink_port);
 	devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_DEL);
-	mutex_lock(&devlink->lock);
+	mutex_lock(&devlink->port_list_lock);
 	list_del(&devlink_port->list);
-	mutex_unlock(&devlink->lock);
+	mutex_unlock(&devlink->port_list_lock);
 	WARN_ON(!list_empty(&devlink_port->reporter_list));
 	WARN_ON(!list_empty(&devlink_port->region_list));
 	mutex_destroy(&devlink_port->reporters_lock);
-- 
2.33.1

[PATCH net-next 3/6] devlink: Add devlink nested locking primitive

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Basic kernel locking primitives lack ability to avoid taking lock
if the lock is already taken by the caller before. In the devlink
case, we can easily implement such context behaviour, because all
user-space entries will take the devlink->lock in the following
patches, while kernel-space entries will need to take that lock
on the entry.

Such change will allow us to take devlink->lock for devlink reload too.

Tested-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 net/core/devlink.c | 146 +++++++++++++++++++++++++++------------------
 1 file changed, 88 insertions(+), 58 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index eddd554b50d4..7dd6091b97af 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -182,6 +182,7 @@ static const struct nla_policy devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_
 
 static DEFINE_XARRAY_FLAGS(devlinks, XA_FLAGS_ALLOC);
 #define DEVLINK_REGISTERED XA_MARK_1
+#define DEVLINK_NESTED_LOCK XA_MARK_2
 
 /* devlink instances are open to the access from the user space after
  * devlink_register() call. Such logical barrier allows us to have certain
@@ -226,6 +227,28 @@ struct devlink *__must_check devlink_try_get(struct devlink *devlink)
 	return NULL;
 }
 
+static void devlink_nested_lock(struct devlink *devlink)
+	__acquires(&devlink->lock)
+{
+	if (xa_get_mark(&devlinks, devlink->index, DEVLINK_NESTED_LOCK))
+		__acquire(&devlink->lock);
+	else
+		mutex_lock(&devlink->lock);
+
+	lockdep_assert_held(&devlink->lock);
+}
+
+static void devlink_nested_unlock(struct devlink *devlink)
+	__releases(&devlink->lock)
+{
+	lockdep_assert_held(&devlink->lock);
+
+	if (xa_get_mark(&devlinks, devlink->index, DEVLINK_NESTED_LOCK))
+		__release(&devlink->lock);
+	else
+		mutex_unlock(&devlink->lock);
+}
+
 static struct devlink *devlink_get_from_attrs(struct net *net,
 					      struct nlattr **attrs)
 {
@@ -594,6 +617,7 @@ devlink_region_snapshot_get_by_id(struct devlink_region *region, u32 id)
 
 static int devlink_nl_pre_doit(const struct genl_ops *ops,
 			       struct sk_buff *skb, struct genl_info *info)
+	__acquires(&devlink->lock)
 {
 	struct devlink_port *devlink_port;
 	struct devlink *devlink;
@@ -605,8 +629,10 @@ static int devlink_nl_pre_doit(const struct genl_ops *ops,
 		mutex_unlock(&devlink_mutex);
 		return PTR_ERR(devlink);
 	}
-	if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK)
+	if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK) {
 		mutex_lock(&devlink->lock);
+		xa_set_mark(&devlinks, devlink->index, DEVLINK_NESTED_LOCK);
+	}
 	info->user_ptr[0] = devlink;
 	if (ops->internal_flags & DEVLINK_NL_FLAG_NEED_PORT) {
 		devlink_port = devlink_port_get_from_info(devlink, info);
@@ -641,8 +667,10 @@ static int devlink_nl_pre_doit(const struct genl_ops *ops,
 	return 0;
 
 unlock:
-	if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK)
+	if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK) {
+		xa_clear_mark(&devlinks, devlink->index, DEVLINK_NESTED_LOCK);
 		mutex_unlock(&devlink->lock);
+	}
 	devlink_put(devlink);
 	mutex_unlock(&devlink_mutex);
 	return err;
@@ -650,12 +678,15 @@ static int devlink_nl_pre_doit(const struct genl_ops *ops,
 
 static void devlink_nl_post_doit(const struct genl_ops *ops,
 				 struct sk_buff *skb, struct genl_info *info)
+	__releases(&devlink->lock)
 {
 	struct devlink *devlink;
 
 	devlink = info->user_ptr[0];
-	if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK)
+	if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK) {
+		xa_clear_mark(&devlinks, devlink->index, DEVLINK_NESTED_LOCK);
 		mutex_unlock(&devlink->lock);
+	}
 	devlink_put(devlink);
 	mutex_unlock(&devlink_mutex);
 }
@@ -9545,7 +9576,7 @@ devlink_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
 	if (!devlink_rate)
 		return -ENOMEM;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	WARN_ON(devlink_port->devlink_rate);
 	devlink_rate->type = DEVLINK_RATE_TYPE_LEAF;
 	devlink_rate->devlink = devlink;
@@ -9554,7 +9585,7 @@ devlink_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
 	list_add_tail(&devlink_rate->list, &devlink->rate_list);
 	devlink_port->devlink_rate = devlink_rate;
 	devlink_rate_notify(devlink_rate, DEVLINK_CMD_RATE_NEW);
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 
 	return 0;
 }
@@ -9575,13 +9606,13 @@ void devlink_rate_leaf_destroy(struct devlink_port *devlink_port)
 	if (!devlink_rate)
 		return;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	devlink_rate_notify(devlink_rate, DEVLINK_CMD_RATE_DEL);
 	if (devlink_rate->parent)
 		refcount_dec(&devlink_rate->parent->refcnt);
 	list_del(&devlink_rate->list);
 	devlink_port->devlink_rate = NULL;
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	kfree(devlink_rate);
 }
 EXPORT_SYMBOL_GPL(devlink_rate_leaf_destroy);
@@ -9601,7 +9632,7 @@ void devlink_rate_nodes_destroy(struct devlink *devlink)
 	static struct devlink_rate *devlink_rate, *tmp;
 	const struct devlink_ops *ops = devlink->ops;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	list_for_each_entry(devlink_rate, &devlink->rate_list, list) {
 		if (!devlink_rate->parent)
 			continue;
@@ -9622,7 +9653,7 @@ void devlink_rate_nodes_destroy(struct devlink *devlink)
 			kfree(devlink_rate);
 		}
 	}
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_rate_nodes_destroy);
 
@@ -9700,7 +9731,7 @@ int devlink_sb_register(struct devlink *devlink, unsigned int sb_index,
 	struct devlink_sb *devlink_sb;
 	int err = 0;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	if (devlink_sb_index_exists(devlink, sb_index)) {
 		err = -EEXIST;
 		goto unlock;
@@ -9719,7 +9750,7 @@ int devlink_sb_register(struct devlink *devlink, unsigned int sb_index,
 	devlink_sb->egress_tc_count = egress_tc_count;
 	list_add_tail(&devlink_sb->list, &devlink->sb_list);
 unlock:
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	return err;
 }
 EXPORT_SYMBOL_GPL(devlink_sb_register);
@@ -9728,11 +9759,11 @@ void devlink_sb_unregister(struct devlink *devlink, unsigned int sb_index)
 {
 	struct devlink_sb *devlink_sb;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	devlink_sb = devlink_sb_get_by_index(devlink, sb_index);
 	WARN_ON(!devlink_sb);
 	list_del(&devlink_sb->list);
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	kfree(devlink_sb);
 }
 EXPORT_SYMBOL_GPL(devlink_sb_unregister);
@@ -9748,9 +9779,9 @@ EXPORT_SYMBOL_GPL(devlink_sb_unregister);
 int devlink_dpipe_headers_register(struct devlink *devlink,
 				   struct devlink_dpipe_headers *dpipe_headers)
 {
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	devlink->dpipe_headers = dpipe_headers;
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(devlink_dpipe_headers_register);
@@ -9764,9 +9795,9 @@ EXPORT_SYMBOL_GPL(devlink_dpipe_headers_register);
  */
 void devlink_dpipe_headers_unregister(struct devlink *devlink)
 {
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	devlink->dpipe_headers = NULL;
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_dpipe_headers_unregister);
 
@@ -9821,7 +9852,7 @@ int devlink_dpipe_table_register(struct devlink *devlink,
 	if (WARN_ON(!table_ops->size_get))
 		return -EINVAL;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 
 	if (devlink_dpipe_table_find(&devlink->dpipe_table_list, table_name,
 				     devlink)) {
@@ -9842,7 +9873,7 @@ int devlink_dpipe_table_register(struct devlink *devlink,
 
 	list_add_tail_rcu(&table->list, &devlink->dpipe_table_list);
 unlock:
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	return err;
 }
 EXPORT_SYMBOL_GPL(devlink_dpipe_table_register);
@@ -9858,17 +9889,17 @@ void devlink_dpipe_table_unregister(struct devlink *devlink,
 {
 	struct devlink_dpipe_table *table;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
 					 table_name, devlink);
 	if (!table)
 		goto unlock;
 	list_del_rcu(&table->list);
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	kfree_rcu(table, rcu);
 	return;
 unlock:
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_dpipe_table_unregister);
 
@@ -9900,7 +9931,7 @@ int devlink_resource_register(struct devlink *devlink,
 
 	top_hierarchy = parent_resource_id == DEVLINK_RESOURCE_ID_PARENT_TOP;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	resource = devlink_resource_find(devlink, NULL, resource_id);
 	if (resource) {
 		err = -EINVAL;
@@ -9940,7 +9971,7 @@ int devlink_resource_register(struct devlink *devlink,
 	INIT_LIST_HEAD(&resource->resource_list);
 	list_add_tail(&resource->list, resource_list);
 out:
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	return err;
 }
 EXPORT_SYMBOL_GPL(devlink_resource_register);
@@ -9967,7 +9998,7 @@ void devlink_resources_unregister(struct devlink *devlink)
 {
 	struct devlink_resource *tmp, *child_resource;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 
 	list_for_each_entry_safe(child_resource, tmp, &devlink->resource_list,
 				 list) {
@@ -9975,8 +10006,7 @@ void devlink_resources_unregister(struct devlink *devlink)
 		list_del(&child_resource->list);
 		kfree(child_resource);
 	}
-
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_resources_unregister);
 
@@ -9994,7 +10024,7 @@ int devlink_resource_size_get(struct devlink *devlink,
 	struct devlink_resource *resource;
 	int err = 0;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	resource = devlink_resource_find(devlink, NULL, resource_id);
 	if (!resource) {
 		err = -EINVAL;
@@ -10003,7 +10033,7 @@ int devlink_resource_size_get(struct devlink *devlink,
 	*p_resource_size = resource->size_new;
 	resource->size = resource->size_new;
 out:
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	return err;
 }
 EXPORT_SYMBOL_GPL(devlink_resource_size_get);
@@ -10023,7 +10053,7 @@ int devlink_dpipe_table_resource_set(struct devlink *devlink,
 	struct devlink_dpipe_table *table;
 	int err = 0;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
 					 table_name, devlink);
 	if (!table) {
@@ -10034,7 +10064,7 @@ int devlink_dpipe_table_resource_set(struct devlink *devlink,
 	table->resource_units = resource_units;
 	table->resource_valid = true;
 out:
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	return err;
 }
 EXPORT_SYMBOL_GPL(devlink_dpipe_table_resource_set);
@@ -10054,7 +10084,7 @@ void devlink_resource_occ_get_register(struct devlink *devlink,
 {
 	struct devlink_resource *resource;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	resource = devlink_resource_find(devlink, NULL, resource_id);
 	if (WARN_ON(!resource))
 		goto out;
@@ -10063,7 +10093,7 @@ void devlink_resource_occ_get_register(struct devlink *devlink,
 	resource->occ_get = occ_get;
 	resource->occ_get_priv = occ_get_priv;
 out:
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_resource_occ_get_register);
 
@@ -10078,7 +10108,7 @@ void devlink_resource_occ_get_unregister(struct devlink *devlink,
 {
 	struct devlink_resource *resource;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	resource = devlink_resource_find(devlink, NULL, resource_id);
 	if (WARN_ON(!resource))
 		goto out;
@@ -10087,7 +10117,7 @@ void devlink_resource_occ_get_unregister(struct devlink *devlink,
 	resource->occ_get = NULL;
 	resource->occ_get_priv = NULL;
 out:
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister);
 
@@ -10326,7 +10356,7 @@ devlink_region_create(struct devlink *devlink,
 	if (WARN_ON(!ops) || WARN_ON(!ops->destructor))
 		return ERR_PTR(-EINVAL);
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 
 	if (devlink_region_get_by_name(devlink, ops->name)) {
 		err = -EEXIST;
@@ -10347,11 +10377,11 @@ devlink_region_create(struct devlink *devlink,
 	list_add_tail(&region->list, &devlink->region_list);
 	devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW);
 
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	return region;
 
 unlock:
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(devlink_region_create);
@@ -10376,7 +10406,7 @@ devlink_port_region_create(struct devlink_port *port,
 	if (WARN_ON(!ops) || WARN_ON(!ops->destructor))
 		return ERR_PTR(-EINVAL);
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 
 	if (devlink_port_region_get_by_name(port, ops->name)) {
 		err = -EEXIST;
@@ -10398,11 +10428,11 @@ devlink_port_region_create(struct devlink_port *port,
 	list_add_tail(&region->list, &port->region_list);
 	devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW);
 
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	return region;
 
 unlock:
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(devlink_port_region_create);
@@ -10417,7 +10447,7 @@ void devlink_region_destroy(struct devlink_region *region)
 	struct devlink *devlink = region->devlink;
 	struct devlink_snapshot *snapshot, *ts;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 
 	/* Free all snapshots of region */
 	list_for_each_entry_safe(snapshot, ts, &region->snapshot_list, list)
@@ -10426,7 +10456,7 @@ void devlink_region_destroy(struct devlink_region *region)
 	list_del(&region->list);
 
 	devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_DEL);
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	kfree(region);
 }
 EXPORT_SYMBOL_GPL(devlink_region_destroy);
@@ -10879,7 +10909,7 @@ int devlink_traps_register(struct devlink *devlink,
 	if (!devlink->ops->trap_init || !devlink->ops->trap_action_set)
 		return -EINVAL;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	for (i = 0; i < traps_count; i++) {
 		const struct devlink_trap *trap = &traps[i];
 
@@ -10891,7 +10921,7 @@ int devlink_traps_register(struct devlink *devlink,
 		if (err)
 			goto err_trap_register;
 	}
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 
 	return 0;
 
@@ -10899,7 +10929,7 @@ int devlink_traps_register(struct devlink *devlink,
 err_trap_verify:
 	for (i--; i >= 0; i--)
 		devlink_trap_unregister(devlink, &traps[i]);
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	return err;
 }
 EXPORT_SYMBOL_GPL(devlink_traps_register);
@@ -10916,7 +10946,7 @@ void devlink_traps_unregister(struct devlink *devlink,
 {
 	int i;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	/* Make sure we do not have any packets in-flight while unregistering
 	 * traps by disabling all of them and waiting for a grace period.
 	 */
@@ -10925,7 +10955,7 @@ void devlink_traps_unregister(struct devlink *devlink,
 	synchronize_rcu();
 	for (i = traps_count - 1; i >= 0; i--)
 		devlink_trap_unregister(devlink, &traps[i]);
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_traps_unregister);
 
@@ -11097,7 +11127,7 @@ int devlink_trap_groups_register(struct devlink *devlink,
 {
 	int i, err;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	for (i = 0; i < groups_count; i++) {
 		const struct devlink_trap_group *group = &groups[i];
 
@@ -11109,7 +11139,7 @@ int devlink_trap_groups_register(struct devlink *devlink,
 		if (err)
 			goto err_trap_group_register;
 	}
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 
 	return 0;
 
@@ -11117,7 +11147,7 @@ int devlink_trap_groups_register(struct devlink *devlink,
 err_trap_group_verify:
 	for (i--; i >= 0; i--)
 		devlink_trap_group_unregister(devlink, &groups[i]);
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	return err;
 }
 EXPORT_SYMBOL_GPL(devlink_trap_groups_register);
@@ -11134,10 +11164,10 @@ void devlink_trap_groups_unregister(struct devlink *devlink,
 {
 	int i;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	for (i = groups_count - 1; i >= 0; i--)
 		devlink_trap_group_unregister(devlink, &groups[i]);
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_trap_groups_unregister);
 
@@ -11237,7 +11267,7 @@ devlink_trap_policers_register(struct devlink *devlink,
 {
 	int i, err;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	for (i = 0; i < policers_count; i++) {
 		const struct devlink_trap_policer *policer = &policers[i];
 
@@ -11252,7 +11282,7 @@ devlink_trap_policers_register(struct devlink *devlink,
 		if (err)
 			goto err_trap_policer_register;
 	}
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 
 	return 0;
 
@@ -11260,7 +11290,7 @@ devlink_trap_policers_register(struct devlink *devlink,
 err_trap_policer_verify:
 	for (i--; i >= 0; i--)
 		devlink_trap_policer_unregister(devlink, &policers[i]);
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 	return err;
 }
 EXPORT_SYMBOL_GPL(devlink_trap_policers_register);
@@ -11278,10 +11308,10 @@ devlink_trap_policers_unregister(struct devlink *devlink,
 {
 	int i;
 
-	mutex_lock(&devlink->lock);
+	devlink_nested_lock(devlink);
 	for (i = policers_count - 1; i >= 0; i--)
 		devlink_trap_policer_unregister(devlink, &policers[i]);
-	mutex_unlock(&devlink->lock);
+	devlink_nested_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_trap_policers_unregister);
 
-- 
2.33.1

[PATCH net-next 4/6] devlink: Require devlink lock during device reload

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Devlink reload was implemented as a special command which does _SET_
operation, but doesn't take devlink->lock, while recursive devlink
calls that were part of .reload_up()/.reload_down() sequence took it.

This fragile flow was possible due to holding a big devlink lock
(devlink_mutex), which effectively stopped all devlink activities,
even unrelated to reloaded devlink.

So let's make sure that devlink reload behaves as other commands.

Tested-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 net/core/devlink.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 7dd6091b97af..cbffafc1776f 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -8810,7 +8810,6 @@ static const struct genl_small_ops devlink_nl_ops[] = {
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_reload,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NO_LOCK,
 	},
 	{
 		.cmd = DEVLINK_CMD_PARAM_GET,
@@ -11448,10 +11447,15 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
 			goto retry;
 
 		WARN_ON(!(devlink->features & DEVLINK_F_RELOAD));
+
+		mutex_lock(&devlink->lock);
+		xa_set_mark(&devlinks, devlink->index, DEVLINK_NESTED_LOCK);
 		err = devlink_reload(devlink, &init_net,
 				     DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
 				     DEVLINK_RELOAD_LIMIT_UNSPEC,
 				     &actions_performed, NULL);
+		xa_clear_mark(&devlinks, devlink->index, DEVLINK_NESTED_LOCK);
+		mutex_unlock(&devlink->lock);
 		if (err && err != -EOPNOTSUPP)
 			pr_warn("Failed to reload devlink instance into init_net\n");
 retry:
-- 
2.33.1

[PATCH net-next 5/6] devlink: Use xarray locking mechanism instead big devlink lock

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

The conversion to XArray together with devlink reference counting
allows us reuse the following locking pattern:
 xa_lock()
  xa_for_each() {
   devlink_try_get()
   xa_unlock()
   ....
   xa_lock()
 }

This pattern gives us a way to run any commands between xa_unlock() and
xa_lock() without big devlink mutex, while making sure that devlink instance
won't be released.

Tested-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 net/core/devlink.c | 249 ++++++++++++++++++++++-----------------------
 1 file changed, 119 insertions(+), 130 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index cbffafc1776f..7666249b346f 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -200,14 +200,6 @@ static DEFINE_XARRAY_FLAGS(devlinks, XA_FLAGS_ALLOC);
 #define ASSERT_DEVLINK_NOT_REGISTERED(d)                                       \
 	WARN_ON_ONCE(xa_get_mark(&devlinks, (d)->index, DEVLINK_REGISTERED))
 
-/* devlink_mutex
- *
- * An overall lock guarding every operation coming from userspace.
- * It also guards devlink devices list and it is taken when
- * driver registers/unregisters it.
- */
-static DEFINE_MUTEX(devlink_mutex);
-
 struct net *devlink_net(const struct devlink *devlink)
 {
 	return read_pnet(&devlink->_net);
@@ -264,8 +256,7 @@ static struct devlink *devlink_get_from_attrs(struct net *net,
 	busname = nla_data(attrs[DEVLINK_ATTR_BUS_NAME]);
 	devname = nla_data(attrs[DEVLINK_ATTR_DEV_NAME]);
 
-	lockdep_assert_held(&devlink_mutex);
-
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (strcmp(devlink->dev->bus->name, busname) == 0 &&
 		    strcmp(dev_name(devlink->dev), devname) == 0 &&
@@ -277,6 +268,7 @@ static struct devlink *devlink_get_from_attrs(struct net *net,
 
 	if (!found || !devlink_try_get(devlink))
 		devlink = ERR_PTR(-ENODEV);
+	xa_unlock(&devlinks);
 
 	return devlink;
 }
@@ -609,12 +601,6 @@ devlink_region_snapshot_get_by_id(struct devlink_region *region, u32 id)
 #define DEVLINK_NL_FLAG_NEED_RATE		BIT(2)
 #define DEVLINK_NL_FLAG_NEED_RATE_NODE		BIT(3)
 
-/* The per devlink instance lock is taken by default in the pre-doit
- * operation, yet several commands do not require this. The global
- * devlink lock is taken and protects from disruption by user-calls.
- */
-#define DEVLINK_NL_FLAG_NO_LOCK			BIT(4)
-
 static int devlink_nl_pre_doit(const struct genl_ops *ops,
 			       struct sk_buff *skb, struct genl_info *info)
 	__acquires(&devlink->lock)
@@ -623,16 +609,12 @@ static int devlink_nl_pre_doit(const struct genl_ops *ops,
 	struct devlink *devlink;
 	int err;
 
-	mutex_lock(&devlink_mutex);
 	devlink = devlink_get_from_attrs(genl_info_net(info), info->attrs);
-	if (IS_ERR(devlink)) {
-		mutex_unlock(&devlink_mutex);
+	if (IS_ERR(devlink))
 		return PTR_ERR(devlink);
-	}
-	if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK) {
-		mutex_lock(&devlink->lock);
-		xa_set_mark(&devlinks, devlink->index, DEVLINK_NESTED_LOCK);
-	}
+
+	mutex_lock(&devlink->lock);
+	xa_set_mark(&devlinks, devlink->index, DEVLINK_NESTED_LOCK);
 	info->user_ptr[0] = devlink;
 	if (ops->internal_flags & DEVLINK_NL_FLAG_NEED_PORT) {
 		devlink_port = devlink_port_get_from_info(devlink, info);
@@ -667,12 +649,9 @@ static int devlink_nl_pre_doit(const struct genl_ops *ops,
 	return 0;
 
 unlock:
-	if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK) {
-		xa_clear_mark(&devlinks, devlink->index, DEVLINK_NESTED_LOCK);
-		mutex_unlock(&devlink->lock);
-	}
+	xa_clear_mark(&devlinks, devlink->index, DEVLINK_NESTED_LOCK);
+	mutex_unlock(&devlink->lock);
 	devlink_put(devlink);
-	mutex_unlock(&devlink_mutex);
 	return err;
 }
 
@@ -680,15 +659,11 @@ static void devlink_nl_post_doit(const struct genl_ops *ops,
 				 struct sk_buff *skb, struct genl_info *info)
 	__releases(&devlink->lock)
 {
-	struct devlink *devlink;
+	struct devlink *devlink = info->user_ptr[0];
 
-	devlink = info->user_ptr[0];
-	if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK) {
-		xa_clear_mark(&devlinks, devlink->index, DEVLINK_NESTED_LOCK);
-		mutex_unlock(&devlink->lock);
-	}
+	xa_clear_mark(&devlinks, devlink->index, DEVLINK_NESTED_LOCK);
+	mutex_unlock(&devlink->lock);
 	devlink_put(devlink);
-	mutex_unlock(&devlink_mutex);
 }
 
 static struct genl_family devlink_nl_family;
@@ -1231,11 +1206,12 @@ static int devlink_nl_cmd_rate_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err = 0;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			goto retry;
 
@@ -1253,6 +1229,7 @@ static int devlink_nl_cmd_rate_get_dumpit(struct sk_buff *msg,
 						   NLM_F_MULTI, NULL);
 			if (err) {
 				mutex_unlock(&devlink->lock);
+				xa_lock(&devlinks);
 				devlink_put(devlink);
 				goto out;
 			}
@@ -1260,10 +1237,11 @@ static int devlink_nl_cmd_rate_get_dumpit(struct sk_buff *msg,
 		}
 		mutex_unlock(&devlink->lock);
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 	if (err != -EMSGSIZE)
 		return err;
 
@@ -1334,32 +1312,37 @@ static int devlink_nl_cmd_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
-		if (!net_eq(devlink_net(devlink), sock_net(msg->sk))) {
-			devlink_put(devlink);
-			continue;
-		}
+		xa_unlock(&devlinks);
+		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
+			goto retry;
 
 		if (idx < start) {
 			idx++;
-			devlink_put(devlink);
-			continue;
+			goto retry;
 		}
 
+		mutex_lock(&devlink->lock);
 		err = devlink_nl_fill(msg, devlink, DEVLINK_CMD_NEW,
 				      NETLINK_CB(cb->skb).portid,
 				      cb->nlh->nlmsg_seq, NLM_F_MULTI);
-		devlink_put(devlink);
-		if (err)
+		mutex_unlock(&devlink->lock);
+		if (err) {
+			xa_lock(&devlinks);
+			devlink_put(devlink);
 			goto out;
+		}
 		idx++;
+retry:
+		xa_lock(&devlinks);
+		devlink_put(devlink);
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 
 	cb->args[0] = idx;
 	return msg->len;
@@ -1397,11 +1380,12 @@ static int devlink_nl_cmd_port_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			goto retry;
 
@@ -1420,6 +1404,7 @@ static int devlink_nl_cmd_port_get_dumpit(struct sk_buff *msg,
 			if (err) {
 				mutex_unlock(&devlink->port_list_lock);
 				mutex_unlock(&devlink->lock);
+				xa_lock(&devlinks);
 				devlink_put(devlink);
 				goto out;
 			}
@@ -1428,10 +1413,11 @@ static int devlink_nl_cmd_port_get_dumpit(struct sk_buff *msg,
 		mutex_unlock(&devlink->port_list_lock);
 		mutex_unlock(&devlink->lock);
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 
 	cb->args[0] = idx;
 	return msg->len;
@@ -1650,7 +1636,6 @@ static int devlink_port_new_notifiy(struct devlink *devlink,
 	if (!msg)
 		return -ENOMEM;
 
-	mutex_lock(&devlink->lock);
 	devlink_port = devlink_port_get_by_index(devlink, port_index);
 	if (!devlink_port) {
 		err = -ENODEV;
@@ -1662,12 +1647,9 @@ static int devlink_port_new_notifiy(struct devlink *devlink,
 	if (err)
 		goto out;
 
-	err = genlmsg_reply(msg, info);
-	mutex_unlock(&devlink->lock);
-	return err;
+	return genlmsg_reply(msg, info);
 
 out:
-	mutex_unlock(&devlink->lock);
 	nlmsg_free(msg);
 	return err;
 }
@@ -2066,11 +2048,12 @@ static int devlink_nl_cmd_sb_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			goto retry;
 
@@ -2087,6 +2070,7 @@ static int devlink_nl_cmd_sb_get_dumpit(struct sk_buff *msg,
 						 NLM_F_MULTI);
 			if (err) {
 				mutex_unlock(&devlink->lock);
+				xa_lock(&devlinks);
 				devlink_put(devlink);
 				goto out;
 			}
@@ -2094,10 +2078,11 @@ static int devlink_nl_cmd_sb_get_dumpit(struct sk_buff *msg,
 		}
 		mutex_unlock(&devlink->lock);
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 
 	cb->args[0] = idx;
 	return msg->len;
@@ -2218,11 +2203,12 @@ static int devlink_nl_cmd_sb_pool_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err = 0;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)) ||
 		    !devlink->ops->sb_pool_get)
 			goto retry;
@@ -2237,16 +2223,18 @@ static int devlink_nl_cmd_sb_pool_get_dumpit(struct sk_buff *msg,
 				err = 0;
 			} else if (err) {
 				mutex_unlock(&devlink->lock);
+				xa_lock(&devlinks);
 				devlink_put(devlink);
 				goto out;
 			}
 		}
 		mutex_unlock(&devlink->lock);
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 
 	if (err != -EMSGSIZE)
 		return err;
@@ -2439,11 +2427,12 @@ static int devlink_nl_cmd_sb_port_pool_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err = 0;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)) ||
 		    !devlink->ops->sb_port_pool_get)
 			goto retry;
@@ -2458,16 +2447,18 @@ static int devlink_nl_cmd_sb_port_pool_get_dumpit(struct sk_buff *msg,
 				err = 0;
 			} else if (err) {
 				mutex_unlock(&devlink->lock);
+				xa_lock(&devlinks);
 				devlink_put(devlink);
 				goto out;
 			}
 		}
 		mutex_unlock(&devlink->lock);
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 
 	if (err != -EMSGSIZE)
 		return err;
@@ -2688,11 +2679,12 @@ devlink_nl_cmd_sb_tc_pool_bind_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err = 0;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)) ||
 		    !devlink->ops->sb_tc_pool_bind_get)
 			goto retry;
@@ -2708,16 +2700,18 @@ devlink_nl_cmd_sb_tc_pool_bind_get_dumpit(struct sk_buff *msg,
 				err = 0;
 			} else if (err) {
 				mutex_unlock(&devlink->lock);
+				xa_lock(&devlinks);
 				devlink_put(devlink);
 				goto out;
 			}
 		}
 		mutex_unlock(&devlink->lock);
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 
 	if (err != -EMSGSIZE)
 		return err;
@@ -2895,15 +2889,11 @@ static int devlink_rate_nodes_check(struct devlink *devlink, u16 mode,
 {
 	struct devlink_rate *devlink_rate;
 
-	/* Take the lock to sync with devlink_rate_nodes_destroy() */
-	mutex_lock(&devlink->lock);
 	list_for_each_entry(devlink_rate, &devlink->rate_list, list)
 		if (devlink_rate_is_node(devlink_rate)) {
-			mutex_unlock(&devlink->lock);
 			NL_SET_ERR_MSG_MOD(extack, "Rate node(s) exists.");
 			return -EBUSY;
 		}
-	mutex_unlock(&devlink->lock);
 	return 0;
 }
 
@@ -4769,11 +4759,12 @@ static int devlink_nl_cmd_param_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err = 0;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			goto retry;
 
@@ -4792,6 +4783,7 @@ static int devlink_nl_cmd_param_get_dumpit(struct sk_buff *msg,
 				err = 0;
 			} else if (err) {
 				mutex_unlock(&devlink->lock);
+				xa_lock(&devlinks);
 				devlink_put(devlink);
 				goto out;
 			}
@@ -4799,10 +4791,11 @@ static int devlink_nl_cmd_param_get_dumpit(struct sk_buff *msg,
 		}
 		mutex_unlock(&devlink->lock);
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 
 	if (err != -EMSGSIZE)
 		return err;
@@ -5004,11 +4997,12 @@ static int devlink_nl_cmd_port_param_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err = 0;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			goto retry;
 
@@ -5033,6 +5027,7 @@ static int devlink_nl_cmd_port_param_get_dumpit(struct sk_buff *msg,
 				} else if (err) {
 					mutex_unlock(&devlink->port_list_lock);
 					mutex_unlock(&devlink->lock);
+					xa_lock(&devlinks);
 					devlink_put(devlink);
 					goto out;
 				}
@@ -5042,10 +5037,11 @@ static int devlink_nl_cmd_port_param_get_dumpit(struct sk_buff *msg,
 		mutex_unlock(&devlink->port_list_lock);
 		mutex_unlock(&devlink->lock);
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 
 	if (err != -EMSGSIZE)
 		return err;
@@ -5554,7 +5550,6 @@ static int devlink_nl_cmd_region_get_devlink_dumpit(struct sk_buff *msg,
 	struct devlink_port *port;
 	int err = 0;
 
-	mutex_lock(&devlink->lock);
 	list_for_each_entry(region, &devlink->region_list, list) {
 		if (*idx < start) {
 			(*idx)++;
@@ -5580,7 +5575,6 @@ static int devlink_nl_cmd_region_get_devlink_dumpit(struct sk_buff *msg,
 out_port:
 	mutex_unlock(&devlink->port_list_lock);
 out:
-	mutex_unlock(&devlink->lock);
 	return err;
 }
 
@@ -5593,23 +5587,27 @@ static int devlink_nl_cmd_region_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err = 0;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			goto retry;
 
+		mutex_lock(&devlink->lock);
 		err = devlink_nl_cmd_region_get_devlink_dumpit(msg, cb, devlink,
 							       &idx, start);
+		mutex_unlock(&devlink->lock);
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 		if (err)
 			goto out;
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 	cb->args[0] = idx;
 	return msg->len;
 }
@@ -5863,12 +5861,9 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 
 	start_offset = *((u64 *)&cb->args[0]);
 
-	mutex_lock(&devlink_mutex);
 	devlink = devlink_get_from_attrs(sock_net(cb->skb->sk), attrs);
-	if (IS_ERR(devlink)) {
-		err = PTR_ERR(devlink);
-		goto out_dev;
-	}
+	if (IS_ERR(devlink))
+		return PTR_ERR(devlink);
 
 	mutex_lock(&devlink->lock);
 
@@ -5968,7 +5963,6 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 	genlmsg_end(skb, hdr);
 	mutex_unlock(&devlink->lock);
 	devlink_put(devlink);
-	mutex_unlock(&devlink_mutex);
 
 	return skb->len;
 
@@ -5977,8 +5971,6 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 out_unlock:
 	mutex_unlock(&devlink->lock);
 	devlink_put(devlink);
-out_dev:
-	mutex_unlock(&devlink_mutex);
 	return err;
 }
 
@@ -6127,11 +6119,12 @@ static int devlink_nl_cmd_info_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err = 0;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			goto retry;
 
@@ -6147,15 +6140,17 @@ static int devlink_nl_cmd_info_get_dumpit(struct sk_buff *msg,
 		if (err == -EOPNOTSUPP)
 			err = 0;
 		else if (err) {
+			xa_lock(&devlinks);
 			devlink_put(devlink);
 			break;
 		}
 inc:
 		idx++;
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 
 	if (err != -EMSGSIZE)
 		return err;
@@ -7230,18 +7225,15 @@ devlink_health_reporter_get_from_cb(struct netlink_callback *cb)
 	struct nlattr **attrs = info->attrs;
 	struct devlink *devlink;
 
-	mutex_lock(&devlink_mutex);
 	devlink = devlink_get_from_attrs(sock_net(cb->skb->sk), attrs);
 	if (IS_ERR(devlink))
-		goto unlock;
+		return NULL;
 
+	mutex_lock(&devlink->lock);
 	reporter = devlink_health_reporter_get_from_attrs(devlink, attrs);
+	mutex_unlock(&devlink->lock);
 	devlink_put(devlink);
-	mutex_unlock(&devlink_mutex);
 	return reporter;
-unlock:
-	mutex_unlock(&devlink_mutex);
-	return NULL;
 }
 
 void
@@ -7307,14 +7299,16 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			goto retry_rep;
 
+		mutex_lock(&devlink->lock);
 		mutex_lock(&devlink->reporters_lock);
 		list_for_each_entry(reporter, &devlink->reporter_list,
 				    list) {
@@ -7328,13 +7322,17 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
 				NLM_F_MULTI);
 			if (err) {
 				mutex_unlock(&devlink->reporters_lock);
+				mutex_unlock(&devlink->lock);
+				xa_lock(&devlinks);
 				devlink_put(devlink);
 				goto out;
 			}
 			idx++;
 		}
 		mutex_unlock(&devlink->reporters_lock);
+		mutex_unlock(&devlink->lock);
 retry_rep:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
 
@@ -7342,6 +7340,7 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			goto retry_port;
 
@@ -7360,9 +7359,9 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
 					NETLINK_CB(cb->skb).portid,
 					cb->nlh->nlmsg_seq, NLM_F_MULTI);
 				if (err) {
-					mutex_unlock(&port->reporters_lock);
 					mutex_unlock(&devlink->port_list_lock);
 					mutex_unlock(&devlink->lock);
+					xa_lock(&devlinks);
 					devlink_put(devlink);
 					goto out;
 				}
@@ -7373,10 +7372,11 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
 		mutex_unlock(&devlink->port_list_lock);
 		mutex_unlock(&devlink->lock);
 retry_port:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 
 	cb->args[0] = idx;
 	return msg->len;
@@ -7906,11 +7906,12 @@ static int devlink_nl_cmd_trap_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			goto retry;
 
@@ -7927,6 +7928,7 @@ static int devlink_nl_cmd_trap_get_dumpit(struct sk_buff *msg,
 						   NLM_F_MULTI);
 			if (err) {
 				mutex_unlock(&devlink->lock);
+				xa_lock(&devlinks);
 				devlink_put(devlink);
 				goto out;
 			}
@@ -7934,10 +7936,11 @@ static int devlink_nl_cmd_trap_get_dumpit(struct sk_buff *msg,
 		}
 		mutex_unlock(&devlink->lock);
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 
 	cb->args[0] = idx;
 	return msg->len;
@@ -8133,11 +8136,12 @@ static int devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			goto retry;
 
@@ -8155,6 +8159,7 @@ static int devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
 							 NLM_F_MULTI);
 			if (err) {
 				mutex_unlock(&devlink->lock);
+				xa_lock(&devlinks);
 				devlink_put(devlink);
 				goto out;
 			}
@@ -8162,10 +8167,11 @@ static int devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
 		}
 		mutex_unlock(&devlink->lock);
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 
 	cb->args[0] = idx;
 	return msg->len;
@@ -8447,11 +8453,12 @@ static int devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
 	int idx = 0;
 	int err;
 
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			goto retry;
 
@@ -8469,6 +8476,7 @@ static int devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
 							   NLM_F_MULTI);
 			if (err) {
 				mutex_unlock(&devlink->lock);
+				xa_lock(&devlinks);
 				devlink_put(devlink);
 				goto out;
 			}
@@ -8476,10 +8484,11 @@ static int devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
 		}
 		mutex_unlock(&devlink->lock);
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
 out:
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 
 	cb->args[0] = idx;
 	return msg->len;
@@ -8672,26 +8681,22 @@ static const struct genl_small_ops devlink_nl_ops[] = {
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_port_split_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NO_LOCK,
 	},
 	{
 		.cmd = DEVLINK_CMD_PORT_UNSPLIT,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_port_unsplit_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NO_LOCK,
 	},
 	{
 		.cmd = DEVLINK_CMD_PORT_NEW,
 		.doit = devlink_nl_cmd_port_new_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NO_LOCK,
 	},
 	{
 		.cmd = DEVLINK_CMD_PORT_DEL,
 		.doit = devlink_nl_cmd_port_del_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NO_LOCK,
 	},
 	{
 		.cmd = DEVLINK_CMD_SB_GET,
@@ -8760,14 +8765,12 @@ static const struct genl_small_ops devlink_nl_ops[] = {
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_eswitch_get_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NO_LOCK,
 	},
 	{
 		.cmd = DEVLINK_CMD_ESWITCH_SET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_eswitch_set_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NO_LOCK,
 	},
 	{
 		.cmd = DEVLINK_CMD_DPIPE_TABLE_GET,
@@ -8877,8 +8880,7 @@ static const struct genl_small_ops devlink_nl_ops[] = {
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_health_reporter_get_doit,
 		.dumpit = devlink_nl_cmd_health_reporter_get_dumpit,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT |
-				  DEVLINK_NL_FLAG_NO_LOCK,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
 		/* can be retrieved by unprivileged users */
 	},
 	{
@@ -8886,24 +8888,21 @@ static const struct genl_small_ops devlink_nl_ops[] = {
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_health_reporter_set_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT |
-				  DEVLINK_NL_FLAG_NO_LOCK,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
 	},
 	{
 		.cmd = DEVLINK_CMD_HEALTH_REPORTER_RECOVER,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_health_reporter_recover_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT |
-				  DEVLINK_NL_FLAG_NO_LOCK,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
 	},
 	{
 		.cmd = DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_health_reporter_diagnose_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT |
-				  DEVLINK_NL_FLAG_NO_LOCK,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
 	},
 	{
 		.cmd = DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
@@ -8917,16 +8916,14 @@ static const struct genl_small_ops devlink_nl_ops[] = {
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_health_reporter_dump_clear_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT |
-				  DEVLINK_NL_FLAG_NO_LOCK,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
 	},
 	{
 		.cmd = DEVLINK_CMD_HEALTH_REPORTER_TEST,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_health_reporter_test_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT |
-				  DEVLINK_NL_FLAG_NO_LOCK,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
 	},
 	{
 		.cmd = DEVLINK_CMD_FLASH_UPDATE,
@@ -9186,10 +9183,8 @@ void devlink_register(struct devlink *devlink)
 	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
 	/* Make sure that we are in .probe() routine */
 
-	mutex_lock(&devlink_mutex);
 	xa_set_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
 	devlink_notify_register(devlink);
-	mutex_unlock(&devlink_mutex);
 }
 EXPORT_SYMBOL_GPL(devlink_register);
 
@@ -9206,10 +9201,8 @@ void devlink_unregister(struct devlink *devlink)
 	devlink_put(devlink);
 	wait_for_completion(&devlink->comp);
 
-	mutex_lock(&devlink_mutex);
 	devlink_notify_unregister(devlink);
 	xa_clear_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
-	mutex_unlock(&devlink_mutex);
 }
 EXPORT_SYMBOL_GPL(devlink_unregister);
 
@@ -9561,8 +9554,6 @@ EXPORT_SYMBOL_GPL(devlink_port_attrs_pci_sf_set);
  * Create devlink rate object of type leaf on provided @devlink_port.
  * Throws call trace if @devlink_port already has a devlink rate object.
  *
- * Context: Takes and release devlink->lock <mutex>.
- *
  * Return: -ENOMEM if failed to allocate rate object, 0 otherwise.
  */
 int
@@ -9594,8 +9585,6 @@ EXPORT_SYMBOL_GPL(devlink_rate_leaf_create);
  * devlink_rate_leaf_destroy - destroy devlink rate leaf
  *
  * @devlink_port: devlink port linked to the rate object
- *
- * Context: Takes and release devlink->lock <mutex>.
  */
 void devlink_rate_leaf_destroy(struct devlink_port *devlink_port)
 {
@@ -9623,8 +9612,6 @@ EXPORT_SYMBOL_GPL(devlink_rate_leaf_destroy);
  *
  * Unset parent for all rate objects and destroy all rate nodes
  * on specified device.
- *
- * Context: Takes and release devlink->lock <mutex>.
  */
 void devlink_rate_nodes_destroy(struct devlink *devlink)
 {
@@ -11438,11 +11425,12 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
 	/* In case network namespace is getting destroyed, reload
 	 * all devlink instances from this namespace into init_net.
 	 */
-	mutex_lock(&devlink_mutex);
+	xa_lock(&devlinks);
 	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
 		if (!devlink_try_get(devlink))
 			continue;
 
+		xa_unlock(&devlinks);
 		if (!net_eq(devlink_net(devlink), net))
 			goto retry;
 
@@ -11459,9 +11447,10 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
 		if (err && err != -EOPNOTSUPP)
 			pr_warn("Failed to reload devlink instance into init_net\n");
 retry:
+		xa_lock(&devlinks);
 		devlink_put(devlink);
 	}
-	mutex_unlock(&devlink_mutex);
+	xa_unlock(&devlinks);
 }
 
 static struct pernet_operations devlink_pernet_ops __net_initdata = {
-- 
2.33.1

[PATCH net-next 6/6] devlink: Open devlink to parallel operations

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Allow parallel execution of devlink ops.

Tested-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 net/core/devlink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 7666249b346f..3b311043cbff 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -8979,6 +8979,7 @@ static struct genl_family devlink_nl_family __ro_after_init = {
 	.n_small_ops	= ARRAY_SIZE(devlink_nl_ops),
 	.mcgrps		= devlink_nl_mcgrps,
 	.n_mcgrps	= ARRAY_SIZE(devlink_nl_mcgrps),
+	.parallel_ops   = true,
 };
 
 static bool devlink_reload_actions_valid(const struct devlink_ops *ops)
-- 
2.33.1

Re: [PATCH net-next 0/6] Allow parallel devlink execution

[Jakub Kicinski]

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.

Re: [PATCH net-next 0/6] Allow parallel devlink execution

[Leon Romanovsky]

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/

Re: [PATCH net-next 0/6] Allow parallel devlink execution

[Jakub Kicinski]

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?

Re: [PATCH net-next 0/6] Allow parallel devlink execution

[Leon Romanovsky]

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.

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

Re: [PATCH net-next 0/6] Allow parallel devlink execution

[Jakub Kicinski]

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. 

Re: [PATCH net-next 0/6] Allow parallel devlink execution

[Leon Romanovsky]

On Wed, Dec 08, 2021 at 07:15:07AM -0800, Jakub Kicinski wrote:

<...>

> I do appreciate your work, but we disagree on how the API should look.

No problem, I'll send different proposal.

Thanks