Skip to contentSkip to conversation

Allow parallel devlink execution

PATCH In review Leon Romanovsky 2 participants 13 messages Dec 5, 2021 – Dec 8, 2021

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Leon Romanovsky

author
#0 lore raw

cover Allow parallel devlink execution

From: Leon Romanovsky <[email protected]>

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/[email protected]/
[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(-)
Signature · 1 line
2.33.1
author
#1 lore raw

PATCH 1/6 devlink: Clean registration of devlink port

From: Leon Romanovsky <[email protected]>

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 <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 net/core/devlink.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)
net/core/devlink.c +8 -15
index db3b52110cf2..34d0f623b2a9 100644
@@ -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);
Signature · 1 line
2.33.1
author
#2 lore raw

PATCH 2/6 devlink: Be explicit with devlink port protection

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

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 <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 net/core/devlink.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)
net/core/devlink.c +22 -7
index 34d0f623b2a9..eddd554b50d4 100644
@@ -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);
Signature · 1 line
2.33.1
author
#3 lore raw

PATCH 3/6 devlink: Add devlink nested locking primitive

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

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 <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 net/core/devlink.c | 146 +++++++++++++++++++++++++++------------------
 1 file changed, 88 insertions(+), 58 deletions(-)
net/core/devlink.c +88 -58
index eddd554b50d4..7dd6091b97af 100644
@@ -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);
Signature · 1 line
2.33.1
author
#4 lore raw

PATCH 4/6 devlink: Require devlink lock during device reload

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

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 <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 net/core/devlink.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
net/core/devlink.c +5 -1
index 7dd6091b97af..cbffafc1776f 100644
@@ -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:
Signature · 1 line
2.33.1
author
#5 lore raw

PATCH 5/6 devlink: Use xarray locking mechanism instead big devlink lock

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

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 <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 net/core/devlink.c | 249 ++++++++++++++++++++++-----------------------
 1 file changed, 119 insertions(+), 130 deletions(-)
net/core/devlink.c +119 -130
index cbffafc1776f..7666249b346f 100644
@@ -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 = {
Signature · 1 line
2.33.1
author
#6 lore raw

PATCH 6/6 devlink: Open devlink to parallel operations

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Allow parallel execution of devlink ops.

Tested-by Ido Schimmel <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 net/core/devlink.c | 1 +
 1 file changed, 1 insertion(+)
net/core/devlink.c +1 -0
index 7666249b346f..3b311043cbff 100644
@@ -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)
Signature · 1 line
2.33.1
maintainer
#7 lore raw
In reply to Leon Romanovsky
Quoted 9 lines · 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.

author
#8 lore raw
Quoted 11 lines · Jakub Kicinski
Quoted 9 lines · 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.

Quoted 2 lines

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/

maintainer
#9 lore raw
Quoted 15 lines · On Tue, 7 Dec 2021 09:29:03 +0200 Leon Romanovsky wrote:
Quoted 11 lines · Jakub Kicinski
Quoted 9 lines · 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.

Quoted 2 lines

I'm not okay with assuming that all sub-objects are added when devlink is not registered.

Quoted 8 lines

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?

author
#10 lore raw
Quoted 21 lines · Jakub Kicinski
Quoted 20 lines · On Tue, 7 Dec 2021 09:29:03 +0200 Leon Romanovsky wrote:
Quoted 11 lines · Jakub Kicinski
Quoted 9 lines · 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.

Quoted 2 lines

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.

Quoted 6 lines
Quoted 3 lines

+#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

maintainer
#11 lore raw
Quoted 29 lines · On Wed, 8 Dec 2021 09:54:55 +0200 Leon Romanovsky wrote:
Quoted 21 lines · Jakub Kicinski
Quoted 20 lines · On Tue, 7 Dec 2021 09:29:03 +0200 Leon Romanovsky wrote:
Quoted 11 lines · Jakub Kicinski
Quoted 9 lines · 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.

Quoted 2 lines

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.

Quoted 6 lines
Quoted 3 lines

+#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?

Quoted 6 lines

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.

Quoted 26 lines

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.

author
#12 lore raw

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

<...>

Quoted 1 line

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

No problem, I'll send different proposal.

Thanks