Date:   Fri, 14 Feb 2020 15:22:12 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     netdev@...r.kernel.org
Cc:     jiri@...nulli.us, valex@...lanox.com, linyunsheng@...wei.com,
        lihong.yang@...el.com, kuba@...nel.org,
        Jacob Keller <jacob.e.keller@...el.com>
Subject: [RFC PATCH v2 13/22] devlink: track snapshot ids using an IDR and refcounts

New snapshot ids are generated by calling a getter function. The same id
may be used by multiple snapshots created at the same trigger event.

Currently no effort is made to release any previously used snapshot ids.

Replace the basic logic of using a single devlink integer for tracking
ids with the IDR interface.

snapshot IDs will be reference counted using a refcount stored in the
IDR. First, ids are allocated using idr_alloc without a refcount (using
the NULL pointer).

Once the devlink_region_snapshot_create function is called, it will call
the new __devlink_region_snapshot_id_ref(). This function will insert
a new refcount or increment the pre-existing refcount.

devlink_region_snapshot_destroy will call the new
__devlink_region_snapshot_id_deref(), decrementing the reference count.
Once there are no other references, the refcount will be removed from
IDR.

Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
---
 include/net/devlink.h |   3 +-
 net/core/devlink.c    | 141 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 138 insertions(+), 6 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 3a7759355434..3a5ff6bea143 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -17,6 +17,7 @@
 #include <linux/refcount.h>
 #include <net/net_namespace.h>
 #include <uapi/linux/devlink.h>
+#include <linux/idr.h>
 
 struct devlink_ops;
 
@@ -28,13 +29,13 @@ struct devlink {
 	struct list_head resource_list;
 	struct list_head param_list;
 	struct list_head region_list;
-	u32 snapshot_id;
 	struct list_head reporter_list;
 	struct mutex reporters_lock; /* protects reporter_list */
 	struct devlink_dpipe_headers *dpipe_headers;
 	struct list_head trap_list;
 	struct list_head trap_group_list;
 	const struct devlink_ops *ops;
+	struct idr snapshot_idr;
 	struct device *dev;
 	possible_net_t _net;
 	struct mutex lock;
diff --git a/net/core/devlink.c b/net/core/devlink.c
index da4e669f425b..9571063846cc 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3760,21 +3760,118 @@ static void devlink_nl_region_notify(struct devlink_region *region,
 	nlmsg_free(msg);
 }
 
+/**
+ * __devlink_region_snapshot_id_ref - Increment reference for a snapshot ID
+ *	@devlink: devlink instance
+ *	@id: the snapshot id being referenced
+ *
+ *	Increments the reference count for the given snapshot id. If the id
+ *	was not yet allocated, it is allocated immediately. If the id was
+ *	allocated but no references exist, a new refcount is created and
+ *	inserted.
+ */
+static int __devlink_region_snapshot_id_ref(struct devlink *devlink, u32 id)
+{
+	struct idr *idr = &devlink->snapshot_idr;
+	refcount_t *ref;
+	void *old_ptr;
+	int err;
+
+	lockdep_assert_held(&devlink->lock);
+
+	if (id < 1 || id >= INT_MAX)
+		return -EINVAL;
+
+	/* Check if a refcount already exists. If so, increment it and exit */
+	ref = idr_find(idr, id);
+	if (ref) {
+		refcount_inc(ref);
+		return 0;
+	}
+
+	/* Allocate a new reference count */
+	ref = kzalloc(sizeof(*ref), GFP_KERNEL);
+	refcount_set(ref, 1);
+
+	/* The id was likely allocated ahead of time using
+	 * devlink_region_snapshot_id_get, so attempt to replace the NULL
+	 * pointer with the refcount. Since idr_find returned NULL,
+	 * idr_replace should either return ERR_PTR(-ENOENT) or NULL.
+	 */
+	old_ptr = idr_replace(idr, ref, id);
+	/* if old_ptr is NULL, we've inserted the reference */
+	if (old_ptr == NULL)
+		return 0;
+	if (PTR_ERR(old_ptr) != -ENOENT) {
+		kfree(ref);
+		return PTR_ERR(old_ptr);
+	}
+
+	/* the snapshot id was not reserved, so reserve it now. */
+	err = idr_alloc(idr, ref, id, id+1, GFP_KERNEL);
+	if (err < 0)
+		return err;
+	WARN_ON(err != id);
+
+	return 0;
+}
+
+/**
+ * __devlink_region_snapshot_id_deref - Decrement reference for a snapshot ID
+ *	@devlink: devlink instance
+ *	@id: the snapshot id being referenced
+ *
+ *	Decrements the reference count for a given snapshot id. If the
+ *	refcount has reached zero then remove the reference from the IDR.
+ */
+static void __devlink_region_snapshot_id_deref(struct devlink *devlink, u32 id)
+{
+	struct idr *idr = &devlink->snapshot_idr;
+	refcount_t *ref;
+
+	lockdep_assert_held(&devlink->lock);
+
+	if (WARN_ON(id < 1 || id >= INT_MAX))
+		return;
+
+	/* Find the reference pointer */
+	ref = idr_find(idr, id);
+	if (!ref) {
+		WARN(true, "no previous reference was inserted");
+		/* this shouldn't happen, but at least attempt to cleanup if
+		 * something went wrong.
+		 */
+		idr_remove(idr, id);
+		return;
+	}
+
+	if (refcount_dec_and_test(ref)) {
+		/* There are no more references, so remove it from the IDR and
+		 * free the reference count.
+		 */
+		idr_remove(idr, id);
+		kfree(ref);
+	}
+}
+
 /**
  *	__devlink_region_snapshot_id_get - get snapshot ID
  *	@devlink: devlink instance
  *
  *	Returns a new snapshot id or a negative error code on failure. Must be
  *	called while holding the devlink instance lock.
+ *
+ *	Snapshot ids are stored in an IDR and reference counted by the number
+ *	of snapshots currently using that id. This function pre-allocates
+ *	a snapshot id but does not fill in a reference count. A later call to
+ *	devlink_region_snapshot_create will update the IDR pointer to
+ *	a reference count. On devlink_region_snapshot_destory, if there are no
+ *	further references, the id will be removed from the IDR.
  */
 static int __devlink_region_snapshot_id_get(struct devlink *devlink)
 {
 	lockdep_assert_held(&devlink->lock);
-
-	if (devlink->snapshot_id >= INT_MAX)
-		return -ENOSPC;
-
-	return ++devlink->snapshot_id;
+	return idr_alloc(&devlink->snapshot_idr, NULL, 1, INT_MAX, GFP_KERNEL);
 }
 
 /**
@@ -3797,6 +3894,7 @@ __devlink_region_snapshot_create(struct devlink_region *region,
 {
 	struct devlink *devlink = region->devlink;
 	struct devlink_snapshot *snapshot;
+	int err;
 
 	lockdep_assert_held(&devlink->lock);
 
@@ -3811,6 +3909,11 @@ __devlink_region_snapshot_create(struct devlink_region *region,
 	if (!snapshot)
 		return -ENOMEM;
 
+	/* Increment snapshot id reference */
+	err = __devlink_region_snapshot_id_ref(devlink, snapshot_id);
+	if (err)
+		goto err_free_snapshot;
+
 	snapshot->id = snapshot_id;
 	snapshot->region = region;
 	snapshot->data = data;
@@ -3821,15 +3924,25 @@ __devlink_region_snapshot_create(struct devlink_region *region,
 
 	devlink_nl_region_notify(region, snapshot, DEVLINK_CMD_REGION_NEW);
 	return 0;
+
+err_free_snapshot:
+	kfree(snapshot);
+	return err;
 }
 
 static void devlink_region_snapshot_del(struct devlink_region *region,
 					struct devlink_snapshot *snapshot)
 {
+	struct devlink *devlink = region->devlink;
+
+	lockdep_assert_held(&devlink_mutex);
+
 	devlink_nl_region_notify(region, snapshot, DEVLINK_CMD_REGION_DEL);
 	region->cur_snapshots--;
 	list_del(&snapshot->list);
 	region->ops->destructor(snapshot->data);
+	__devlink_region_snapshot_id_deref(devlink, snapshot->id);
+
 	kfree(snapshot);
 }
 
@@ -6388,6 +6501,7 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size)
 	if (!devlink)
 		return NULL;
 	devlink->ops = ops;
+	idr_init(&devlink->snapshot_idr);
 	__devlink_net_set(devlink, &init_net);
 	INIT_LIST_HEAD(&devlink->port_list);
 	INIT_LIST_HEAD(&devlink->sb_list);
@@ -6480,6 +6594,23 @@ EXPORT_SYMBOL_GPL(devlink_reload_disable);
  */
 void devlink_free(struct devlink *devlink)
 {
+	struct idr *idr = &devlink->snapshot_idr;
+
+	mutex_lock(&devlink->lock);
+	if (!idr_is_empty(idr)) {
+		refcount_t *ref;
+		int id;
+
+		WARN(true, "snapshot IDR is not empty");
+
+		idr_for_each_entry(idr, ref, id) {
+			if (ref)
+				kfree(ref);
+		}
+	}
+	idr_destroy(&devlink->snapshot_idr);
+	mutex_unlock(&devlink->lock);
+
 	mutex_destroy(&devlink->reporters_lock);
 	mutex_destroy(&devlink->lock);
 	WARN_ON(!list_empty(&devlink->trap_group_list));
-- 
2.25.0.368.g28a2d05eebfb

Powered by blists - more mailing lists