diff mbox series

[net-next,2/9] net: devlink: remove region snapshots list dependency on devlink->lock

Message ID 1658941416-74393-3-git-send-email-moshe@nvidia.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Take devlink lock on mlx4 and mlx5 callbacks | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 5 this patch: 5
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2 this patch: 2
netdev/checkpatch warning CHECK: struct mutex definition without comment
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Moshe Shemesh July 27, 2022, 5:03 p.m. UTC 
From: Jiri Pirko <jiri@nvidia.com>

After mlx4 driver is converted to do locked reload,
devlink_region_snapshot_create() may be called from both locked and
unlocked context.

So resolve this by removing dependency on devlink->lock for region
snapshots list consistency and introduce new mutex to ensure it.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/core/devlink.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

Comments

Jakub Kicinski July 28, 2022, 2:01 a.m. UTC | #1 
On Wed, 27 Jul 2022 20:03:29 +0300 Moshe Shemesh wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> After mlx4 driver is converted to do locked reload,
> devlink_region_snapshot_create() may be called from both locked and
> unlocked context.

You need to explain why, tho. What makes region snapshots special? 

> So resolve this by removing dependency on devlink->lock for region
> snapshots list consistency and introduce new mutex to ensure it.

I was hoping to avoid per-subobject locks. What prevents us from
depending on the instance lock here (once the driver is converted)?
Jiri Pirko July 28, 2022, 8:50 a.m. UTC | #2 
Thu, Jul 28, 2022 at 04:01:56AM CEST, kuba@kernel.org wrote:
>On Wed, 27 Jul 2022 20:03:29 +0300 Moshe Shemesh wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> After mlx4 driver is converted to do locked reload,
>> devlink_region_snapshot_create() may be called from both locked and
>> unlocked context.
>
>You need to explain why, tho. What makes region snapshots special? 

Will do.


>
>> So resolve this by removing dependency on devlink->lock for region
>> snapshots list consistency and introduce new mutex to ensure it.
>
>I was hoping to avoid per-subobject locks. What prevents us from
>depending on the instance lock here (once the driver is converted)?

The fact that it could be called in mlx4 from both devl locked and
unlocked context. Basically whenever CMD to fw is called.

What is wrong in small locks here and there when they are sufficient?
Jakub Kicinski July 28, 2022, 4:16 p.m. UTC | #3 
On Thu, 28 Jul 2022 10:50:29 +0200 Jiri Pirko wrote:
> >> So resolve this by removing dependency on devlink->lock for region
> >> snapshots list consistency and introduce new mutex to ensure it.  
> >
> >I was hoping to avoid per-subobject locks. What prevents us from
> >depending on the instance lock here (once the driver is converted)?  
> 
> The fact that it could be called in mlx4 from both devl locked and
> unlocked context. Basically whenever CMD to fw is called.

Ok, I guess mlx4 uses regions as proto-health reporters so too hard of
a battle to fight. Please update the commit message tho.

> What is wrong in small locks here and there when they are sufficient?

The more locks the less obvious the semantics and ordering of locking
are.
diff mbox series

Patch

diff --git a/net/core/devlink.c b/net/core/devlink.c
index da002791e300..4de1f93053a2 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -695,6 +695,10 @@  struct devlink_region {
 		const struct devlink_region_ops *ops;
 		const struct devlink_port_region_ops *port_ops;
 	};
+	struct mutex snapshot_lock; /* protects snapshot_list,
+				     * max_snapshots and cur_snapshots
+				     * consistency.
+				     */
 	struct list_head snapshot_list;
 	u32 max_snapshots;
 	u32 cur_snapshots;
@@ -5818,7 +5822,7 @@  static int __devlink_region_snapshot_id_get(struct devlink *devlink, u32 *id)
  *	Multiple snapshots can be created on a region.
  *	The @snapshot_id should be obtained using the getter function.
  *
- *	Must be called only while holding the devlink instance lock.
+ *	Must be called only while holding the region snapshot lock.
  *
  *	@region: devlink region of the snapshot
  *	@data: snapshot data
@@ -5832,7 +5836,7 @@  __devlink_region_snapshot_create(struct devlink_region *region,
 	struct devlink_snapshot *snapshot;
 	int err;
 
-	devl_assert_locked(devlink);
+	lockdep_assert_held(&region->snapshot_lock);
 
 	/* check if region can hold one more snapshot */
 	if (region->cur_snapshots == region->max_snapshots)
@@ -5870,7 +5874,7 @@  static void devlink_region_snapshot_del(struct devlink_region *region,
 {
 	struct devlink *devlink = region->devlink;
 
-	devl_assert_locked(devlink);
+	lockdep_assert_held(&region->snapshot_lock);
 
 	devlink_nl_region_notify(region, snapshot, DEVLINK_CMD_REGION_DEL);
 	region->cur_snapshots--;
@@ -6049,11 +6053,15 @@  static int devlink_nl_cmd_region_del(struct sk_buff *skb,
 	if (!region)
 		return -EINVAL;
 
+	mutex_lock(&region->snapshot_lock);
 	snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
-	if (!snapshot)
+	if (!snapshot) {
+		mutex_unlock(&region->snapshot_lock);
 		return -EINVAL;
+	}
 
 	devlink_region_snapshot_del(region, snapshot);
+	mutex_unlock(&region->snapshot_lock);
 	return 0;
 }
 
@@ -6101,9 +6109,12 @@  devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
 		return -EOPNOTSUPP;
 	}
 
+	mutex_lock(&region->snapshot_lock);
+
 	if (region->cur_snapshots == region->max_snapshots) {
 		NL_SET_ERR_MSG_MOD(info->extack, "The region has reached the maximum number of stored snapshots");
-		return -ENOSPC;
+		err = -ENOSPC;
+		goto unlock;
 	}
 
 	snapshot_id_attr = info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID];
@@ -6112,17 +6123,18 @@  devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
 
 		if (devlink_region_snapshot_get_by_id(region, snapshot_id)) {
 			NL_SET_ERR_MSG_MOD(info->extack, "The requested snapshot id is already in use");
-			return -EEXIST;
+			err = -EEXIST;
+			goto unlock;
 		}
 
 		err = __devlink_snapshot_id_insert(devlink, snapshot_id);
 		if (err)
-			return err;
+			goto unlock;
 	} else {
 		err = __devlink_region_snapshot_id_get(devlink, &snapshot_id);
 		if (err) {
 			NL_SET_ERR_MSG_MOD(info->extack, "Failed to allocate a new snapshot id");
-			return err;
+			goto unlock;
 		}
 	}
 
@@ -6160,16 +6172,20 @@  devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
 			goto err_notify;
 	}
 
+	mutex_unlock(&region->snapshot_lock);
 	return 0;
 
 err_snapshot_create:
 	region->ops->destructor(data);
 err_snapshot_capture:
 	__devlink_snapshot_id_decrement(devlink, snapshot_id);
+	mutex_unlock(&region->snapshot_lock);
 	return err;
 
 err_notify:
 	devlink_region_snapshot_del(region, snapshot);
+unlock:
+	mutex_unlock(&region->snapshot_lock);
 	return err;
 }
 
@@ -11095,6 +11111,7 @@  struct devlink_region *devl_region_create(struct devlink *devlink,
 	region->ops = ops;
 	region->size = region_size;
 	INIT_LIST_HEAD(&region->snapshot_list);
+	mutex_init(&region->snapshot_lock);
 	list_add_tail(&region->list, &devlink->region_list);
 	devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW);
 
@@ -11168,6 +11185,7 @@  devlink_port_region_create(struct devlink_port *port,
 	region->port_ops = ops;
 	region->size = region_size;
 	INIT_LIST_HEAD(&region->snapshot_list);
+	mutex_init(&region->snapshot_lock);
 	list_add_tail(&region->list, &port->region_list);
 	devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW);
 
@@ -11197,6 +11215,7 @@  void devl_region_destroy(struct devlink_region *region)
 		devlink_region_snapshot_del(region, snapshot);
 
 	list_del(&region->list);
+	mutex_destroy(&region->snapshot_lock);
 
 	devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_DEL);
 	kfree(region);
@@ -11272,13 +11291,11 @@  EXPORT_SYMBOL_GPL(devlink_region_snapshot_id_put);
 int devlink_region_snapshot_create(struct devlink_region *region,
 				   u8 *data, u32 snapshot_id)
 {
-	struct devlink *devlink = region->devlink;
 	int err;
 
-	devl_lock(devlink);
+	mutex_lock(&region->snapshot_lock);
 	err = __devlink_region_snapshot_create(region, data, snapshot_id);
-	devl_unlock(devlink);
-
+	mutex_unlock(&region->snapshot_lock);
 	return err;
 }
 EXPORT_SYMBOL_GPL(devlink_region_snapshot_create);