diff mbox series

[net-next,1/9] net: devlink: remove region snapshot ID tracking dependency on devlink->lock

Message ID 1658941416-74393-2-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 success total: 0 errors, 0 warnings, 0 checks, 122 lines checked
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, functions to get/put
regions snapshot ID may be called from both locked and unlocked context.

So resolve this by removing dependency on devlink->lock for region
snapshot ID tracking by using internal xa_lock() to maintain
shapshot_ids xa_array consistency.

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

Comments

Jakub Kicinski July 28, 2022, 1:58 a.m. UTC | #1 
On Wed, 27 Jul 2022 20:03:28 +0300 Moshe Shemesh wrote:
> So resolve this by removing dependency on devlink->lock for region
> snapshot ID tracking by using internal xa_lock() to maintain
> shapshot_ids xa_array consistency.

xa_lock() is a spin lock, right?  s/GFP_KERNEL/GFP_ATOMIC/
Jiri Pirko July 28, 2022, 8:46 a.m. UTC | #2 
Thu, Jul 28, 2022 at 03:58:51AM CEST, kuba@kernel.org wrote:
>On Wed, 27 Jul 2022 20:03:28 +0300 Moshe Shemesh wrote:
>> So resolve this by removing dependency on devlink->lock for region
>> snapshot ID tracking by using internal xa_lock() to maintain
>> shapshot_ids xa_array consistency.
>
>xa_lock() is a spin lock, right?  s/GFP_KERNEL/GFP_ATOMIC/

Correct, will fix.
Jiri Pirko July 28, 2022, 9:21 a.m. UTC | #3 
Thu, Jul 28, 2022 at 10:46:29AM CEST, jiri@resnulli.us wrote:
>Thu, Jul 28, 2022 at 03:58:51AM CEST, kuba@kernel.org wrote:
>>On Wed, 27 Jul 2022 20:03:28 +0300 Moshe Shemesh wrote:
>>> So resolve this by removing dependency on devlink->lock for region
>>> snapshot ID tracking by using internal xa_lock() to maintain
>>> shapshot_ids xa_array consistency.
>>
>>xa_lock() is a spin lock, right?  s/GFP_KERNEL/GFP_ATOMIC/
>
>Correct, will fix.

Well, from how I read __xa_store(), it should be ok to have GFP_KERNEL
here, but I don't think it has any benefit over GFP_ATOMIC in this
usecase, so I will change it to GFP_ATOMIC.
diff mbox series

Patch

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 698b2d6e0ec7..da002791e300 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -5691,21 +5691,28 @@  static int __devlink_snapshot_id_increment(struct devlink *devlink, u32 id)
 {
 	unsigned long count;
 	void *p;
+	int err;
 
-	devl_assert_locked(devlink);
-
+	xa_lock(&devlink->snapshot_ids);
 	p = xa_load(&devlink->snapshot_ids, id);
-	if (WARN_ON(!p))
-		return -EINVAL;
+	if (WARN_ON(!p)) {
+		err = -EINVAL;
+		goto unlock;
+	}
 
-	if (WARN_ON(!xa_is_value(p)))
-		return -EINVAL;
+	if (WARN_ON(!xa_is_value(p))) {
+		err = -EINVAL;
+		goto unlock;
+	}
 
 	count = xa_to_value(p);
 	count++;
 
-	return xa_err(xa_store(&devlink->snapshot_ids, id, xa_mk_value(count),
-			       GFP_KERNEL));
+	err = xa_err(__xa_store(&devlink->snapshot_ids, id, xa_mk_value(count),
+				GFP_KERNEL));
+unlock:
+	xa_unlock(&devlink->snapshot_ids);
+	return err;
 }
 
 /**
@@ -5728,25 +5735,26 @@  static void __devlink_snapshot_id_decrement(struct devlink *devlink, u32 id)
 	unsigned long count;
 	void *p;
 
-	devl_assert_locked(devlink);
-
+	xa_lock(&devlink->snapshot_ids);
 	p = xa_load(&devlink->snapshot_ids, id);
 	if (WARN_ON(!p))
-		return;
+		goto unlock;
 
 	if (WARN_ON(!xa_is_value(p)))
-		return;
+		goto unlock;
 
 	count = xa_to_value(p);
 
 	if (count > 1) {
 		count--;
-		xa_store(&devlink->snapshot_ids, id, xa_mk_value(count),
-			 GFP_KERNEL);
+		__xa_store(&devlink->snapshot_ids, id, xa_mk_value(count),
+			   GFP_KERNEL);
 	} else {
 		/* If this was the last user, we can erase this id */
-		xa_erase(&devlink->snapshot_ids, id);
+		__xa_erase(&devlink->snapshot_ids, id);
 	}
+unlock:
+	xa_unlock(&devlink->snapshot_ids);
 }
 
 /**
@@ -5767,13 +5775,17 @@  static void __devlink_snapshot_id_decrement(struct devlink *devlink, u32 id)
  */
 static int __devlink_snapshot_id_insert(struct devlink *devlink, u32 id)
 {
-	devl_assert_locked(devlink);
+	int err;
 
-	if (xa_load(&devlink->snapshot_ids, id))
+	xa_lock(&devlink->snapshot_ids);
+	if (xa_load(&devlink->snapshot_ids, id)) {
+		xa_unlock(&devlink->snapshot_ids);
 		return -EEXIST;
-
-	return xa_err(xa_store(&devlink->snapshot_ids, id, xa_mk_value(0),
-			       GFP_KERNEL));
+	}
+	err = xa_err(__xa_store(&devlink->snapshot_ids, id, xa_mk_value(0),
+				GFP_KERNEL));
+	xa_unlock(&devlink->snapshot_ids);
+	return err;
 }
 
 /**
@@ -5794,8 +5806,6 @@  static int __devlink_snapshot_id_insert(struct devlink *devlink, u32 id)
  */
 static int __devlink_region_snapshot_id_get(struct devlink *devlink, u32 *id)
 {
-	devl_assert_locked(devlink);
-
 	return xa_alloc(&devlink->snapshot_ids, id, xa_mk_value(1),
 			xa_limit_32b, GFP_KERNEL);
 }
@@ -11227,13 +11237,7 @@  EXPORT_SYMBOL_GPL(devlink_region_destroy);
  */
 int devlink_region_snapshot_id_get(struct devlink *devlink, u32 *id)
 {
-	int err;
-
-	devl_lock(devlink);
-	err = __devlink_region_snapshot_id_get(devlink, id);
-	devl_unlock(devlink);
-
-	return err;
+	return __devlink_region_snapshot_id_get(devlink, id);
 }
 EXPORT_SYMBOL_GPL(devlink_region_snapshot_id_get);
 
@@ -11249,9 +11253,7 @@  EXPORT_SYMBOL_GPL(devlink_region_snapshot_id_get);
  */
 void devlink_region_snapshot_id_put(struct devlink *devlink, u32 id)
 {
-	devl_lock(devlink);
 	__devlink_snapshot_id_decrement(devlink, id);
-	devl_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_region_snapshot_id_put);