Re: [PATCH 02/15] devlink: add functions to take snapshot while locked

[Jiri Pirko <jiri@resnulli.us>]

Thu, Jan 30, 2020 at 11:58:57PM CET, jacob.e.keller@intel.com wrote:
>A future change is going to add a new devlink command to request
>a snapshot on demand. This function will want to call the
>devlink_region_snapshot_id_get and devlink_region_snapshot_create
>functions while already holding the devlink instance lock.
>
>Extract the logic of these two functions into static functions with the
>_locked postfix. Modify the original functions to be implemented in
>terms of the new locked functions.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>---
> net/core/devlink.c | 95 +++++++++++++++++++++++++++++-----------------
> 1 file changed, 61 insertions(+), 34 deletions(-)
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index d1f7bfbf81da..faf4f4c5c539 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -3761,6 +3761,63 @@ static void devlink_nl_region_notify(struct devlink_region *region,
> 	nlmsg_free(msg);
> }
> 
>+/**
>+ *	devlink_region_snapshot_id_get_locked - get snapshot ID
>+ *
>+ *	Returns a new snapshot id. Must be called while holding the
>+ *	devlink instance lock.
>+ */
>+static u32 devlink_region_snapshot_id_get_locked(struct devlink *devlink)

__devlink_region_snapshot_id_get()


>+{
>+	return ++devlink->snapshot_id;
>+}
>+
>+/**
>+ *	devlink_region_snapshot_create_locked - create a new snapshot
>+ *	This will add a new snapshot of a region. The snapshot
>+ *	will be stored on the region struct and can be accessed
>+ *	from devlink. This is useful for future	analyses of snapshots.
>+ *	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.
>+ *
>+ *	@region: devlink region of the snapshot
>+ *	@data: snapshot data
>+ *	@snapshot_id: snapshot id to be created
>+ *	@destructor: pointer to destructor function to free data
>+ */
>+static int
>+devlink_region_snapshot_create_locked(struct devlink_region *region,

__devlink_region_snapshot_create()


>+				      u8 *data, u32 snapshot_id,
>+				      devlink_snapshot_data_dest_t *destructor)
>+{
>+	struct devlink_snapshot *snapshot;
>+
>+	/* check if region can hold one more snapshot */
>+	if (region->cur_snapshots == region->max_snapshots)
>+		return -ENOMEM;
>+
>+	if (devlink_region_snapshot_get_by_id(region, snapshot_id))
>+		return -EEXIST;
>+
>+	snapshot = kzalloc(sizeof(*snapshot), GFP_KERNEL);
>+	if (!snapshot)
>+		return -ENOMEM;
>+
>+	snapshot->id = snapshot_id;
>+	snapshot->region = region;
>+	snapshot->data = data;
>+	snapshot->data_destructor = destructor;
>+
>+	list_add_tail(&snapshot->list, &region->snapshot_list);
>+
>+	region->cur_snapshots++;
>+
>+	devlink_nl_region_notify(region, snapshot, DEVLINK_CMD_REGION_NEW);
>+	return 0;
>+}
>+
> static void devlink_region_snapshot_del(struct devlink_region *region,
> 					struct devlink_snapshot *snapshot)
> {
>@@ -7611,7 +7668,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_locked(devlink);
> 	mutex_unlock(&devlink->lock);
> 
> 	return id;
>@@ -7622,7 +7679,7 @@ EXPORT_SYMBOL_GPL(devlink_region_snapshot_id_get);
>  *	devlink_region_snapshot_create - create a new snapshot
>  *	This will add a new snapshot of a region. The snapshot
>  *	will be stored on the region struct and can be accessed
>- *	from devlink. This is useful for future	analyses of snapshots.
>+ *	from devlink. This is useful for future analyses of snapshots.

What this hunk is about? :O



>  *	Multiple snapshots can be created on a region.
>  *	The @snapshot_id should be obtained using the getter function.
>  *
>@@ -7636,43 +7693,13 @@ int devlink_region_snapshot_create(struct devlink_region *region,
> 				   devlink_snapshot_data_dest_t *data_destructor)
> {
> 	struct devlink *devlink = region->devlink;
>-	struct devlink_snapshot *snapshot;
> 	int err;
> 
> 	mutex_lock(&devlink->lock);
>-
>-	/* check if region can hold one more snapshot */
>-	if (region->cur_snapshots == region->max_snapshots) {
>-		err = -ENOMEM;
>-		goto unlock;
>-	}
>-
>-	if (devlink_region_snapshot_get_by_id(region, snapshot_id)) {
>-		err = -EEXIST;
>-		goto unlock;
>-	}
>-
>-	snapshot = kzalloc(sizeof(*snapshot), GFP_KERNEL);
>-	if (!snapshot) {
>-		err = -ENOMEM;
>-		goto unlock;
>-	}
>-
>-	snapshot->id = snapshot_id;
>-	snapshot->region = region;
>-	snapshot->data = data;
>-	snapshot->data_destructor = data_destructor;
>-
>-	list_add_tail(&snapshot->list, &region->snapshot_list);
>-
>-	region->cur_snapshots++;
>-
>-	devlink_nl_region_notify(region, snapshot, DEVLINK_CMD_REGION_NEW);
>+	err = devlink_region_snapshot_create_locked(region, data, snapshot_id,
>+						    data_destructor);
> 	mutex_unlock(&devlink->lock);
>-	return 0;
> 
>-unlock:
>-	mutex_unlock(&devlink->lock);
> 	return err;
> }
> EXPORT_SYMBOL_GPL(devlink_region_snapshot_create);
>-- 
>2.25.0.rc1
>