Re: [RFC PATCH v2 11/22] devlink: add functions to take snapshot while locked

Jiri Pirko
2020-03-02T17:44:01Z

Sat, Feb 15, 2020 at 12:22:10AM 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 prefixed
>by `__` to indicate they are internal helper functions. Modify the
>original functions to be implemented in terms of the new locked
>functions.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

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

>---
> net/core/devlink.c | 93 ++++++++++++++++++++++++++++++----------------
> 1 file changed, 61 insertions(+), 32 deletions(-)
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index fef93f48028c..0e94887713f4 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -3760,6 +3760,65 @@ 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.
>+ */

You don't need this docu comment for static functions.

>+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
>+ * 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
>+ */
>+static int
>+__devlink_region_snapshot_create(struct devlink_region *region,
>+ u8 *data, u32 snapshot_id)
>+{
>+ struct devlink *devlink = region->devlink;
>+ struct devlink_snapshot *snapshot;
>+
>+ lockdep_assert_held(&devlink->lock);
>+
>+ /* 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;
>+
>+ 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)
> {
>@@ -7618,7 +7677,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;
>@@ -7641,42 +7700,12 @@ int devlink_region_snapshot_create(struct devlink_region *region,
> u8 *data, u32 snapshot_id)
> {
> 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;
>-
>- 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(region, data, snapshot_id);
> mutex_unlock(&devlink->lock);
>- return 0;
>
>-unlock:
>- mutex_unlock(&devlink->lock);
> return err;
> }
> EXPORT_SYMBOL_GPL(devlink_region_snapshot_create);
>--
>2.25.0.368.g28a2d05eebfb
>