On 7/13/2018 3:51 AM, Jakub Kicinski wrote:

On Thu, 12 Jul 2018 15:13:09 +0300, Alex Vesker wrote:

quoted

To restrict the driver with the snapshot ID selection a new callback
is introduced for the driver to get the snapshot ID before creating
a new snapshot. This will also allow giving the same ID for multiple
snapshots taken of different regions on the same time.

I'm not in position to criticize other people's commit messages :), but
I find this one hard to parse.  I think what you meant to say is that
you add a helper for numbering the snapshot per-devlink instance.
There is no callback to be seen here.  You *prevent* from giving the
same ID to multiple snapshot even if they are from different regions.

Let me try to clarify,
The idea is to have a simple helper function that assigns IDs to provide 
a more complete
API, an example use case is when you want to add a new snapshot to 
multiple regions
from the same trigger, then it should be called once to get an ID, this 
ID should be used
on all new snapshots.

quoted

diff --git a/net/core/devlink.c b/net/core/devlink.c
index cac8561..6c92ddd 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c

@@ -4193,6 +4193,27 @@ void devlink_region_destroy(struct devlink_region *region)
  }
  EXPORT_SYMBOL_GPL(devlink_region_destroy);
  
+/**
+ *	devlink_region_shapshot_id_get - get snapshot ID
+ *
+ *	This callback should be called when adding a new snapshot,
+ *	Driver should use the same id for multiple snapshots taken
+ *	on multiple regions at the same time/by the same trigger.
+ *
+ *	@devlink: devlink
+ */
+u32 devlink_region_shapshot_id_get(struct devlink *devlink)
+{
+	u32 id;
+
+	mutex_lock(&devlink->lock);
+	id = ++devlink->snapshot_id;

Any reason not to use an IDA?  The reuse may seem unlikely, OTOH IDA
isn't going to cost much, so why risk it...

As you mentioned more than u32_max_value snapshots doesn't sound likely.
New snapshots will be created, old snapshots should be deleted by the user
a wrap around sounds unlikely. Let me think about it some more, might send a
patch that changes to IDA.

quoted

+	mutex_unlock(&devlink->lock);
+
+	return id;
+}
+EXPORT_SYMBOL_GPL(devlink_region_shapshot_id_get);

Sorry for only spotting this now.