[net-next,v3,06/11] devlink: extract snapshot id allocation to helper function
diff mbox series

Message ID 20200326183718.2384349-7-jacob.e.keller@intel.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • implement DEVLINK_CMD_REGION_NEW
Related show

Commit Message

Jacob Keller March 26, 2020, 6:37 p.m. UTC
A future change is going to implement a new devlink command to request
a snapshot on demand. As part of this, the logic for handling the
snapshot ids will be refactored. To simplify the snapshot id allocation
function, move it to a separate function prefixed by `__`. This helper
function will assume the lock is held.

While no other callers will exist, it simplifies refactoring the logic
because there is no need to complicate the function with gotos to handle
unlocking on failure.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes since RFC:
* Added this patch to explain reasoning why the function is kept despite
  only having one caller.

 net/core/devlink.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Jiri Pirko March 26, 2020, 9:15 p.m. UTC | #1
Thu, Mar 26, 2020 at 07:37:13PM CET, jacob.e.keller@intel.com wrote:
>A future change is going to implement a new devlink command to request
>a snapshot on demand. As part of this, the logic for handling the
>snapshot ids will be refactored. To simplify the snapshot id allocation
>function, move it to a separate function prefixed by `__`. This helper
>function will assume the lock is held.
>
>While no other callers will exist, it simplifies refactoring the logic
>because there is no need to complicate the function with gotos to handle
>unlocking on failure.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Reviewed-by: Jiri Pirko <jiri@mellanox.com>

For the 3rd time. I'm not sure why you don't took this :/
Jacob Keller March 26, 2020, 9:37 p.m. UTC | #2
On 3/26/2020 2:15 PM, Jiri Pirko wrote:
> Thu, Mar 26, 2020 at 07:37:13PM CET, jacob.e.keller@intel.com wrote:
>> A future change is going to implement a new devlink command to request
>> a snapshot on demand. As part of this, the logic for handling the
>> snapshot ids will be refactored. To simplify the snapshot id allocation
>> function, move it to a separate function prefixed by `__`. This helper
>> function will assume the lock is held.
>>
>> While no other callers will exist, it simplifies refactoring the logic
>> because there is no need to complicate the function with gotos to handle
>> unlocking on failure.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> 
> For the 3rd time. I'm not sure why you don't took this :/
> 

Missed it while going over all the emails to take in review.

Patch
diff mbox series

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 8c2c4bc009bb..457b8303ae59 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3768,6 +3768,19 @@  static void devlink_nl_region_notify(struct devlink_region *region,
 	nlmsg_free(msg);
 }
 
+/**
+ *	__devlink_region_snapshot_id_get - get snapshot ID
+ *	@devlink: devlink instance
+ *
+ *	Returns a new snapshot id. Must be called while holding the
+ *	devlink instance lock.
+ */
+static u32 __devlink_region_snapshot_id_get(struct devlink *devlink)
+{
+	lockdep_assert_held(&devlink->lock);
+	return ++devlink->snapshot_id;
+}
+
 /**
  *	__devlink_region_snapshot_create - create a new snapshot
  *	This will add a new snapshot of a region. The snapshot
@@ -7775,7 +7788,7 @@  u32 devlink_region_snapshot_id_get(struct devlink *devlink)
 	u32 id;
 
 	mutex_lock(&devlink->lock);
-	id = ++devlink->snapshot_id;
+	id = __devlink_region_snapshot_id_get(devlink);
 	mutex_unlock(&devlink->lock);
 
 	return id;