[PATCH] nvme-core: use xarray for cel storing

[PATCH] nvme-core: use xarray for cel storing

[Chaitanya Kulkarni]

When using linked list we have to open code the locking, search, and
destroy operations with the loops even if data structure doesn't fall
into the fast path.

One of the main advantage of having XArray to store, search, and remove
items is that it handles all the locking by itself, avoids the loops 
when using linked lists, provides clear API to replace the linked list's
search and destroy loops. 

This patch replaces the ctrl->cel list with XArray and removes :-

a. Extra code needed for the linked list for ctrl->cel item management
   such as nvme_find_cel().
b. Destroy loop in the nvme_free_ctrl().
c. Explicit insertion locking in the nvme_get_effects_log().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/core.c | 31 ++++---------------------------
 drivers/nvme/host/nvme.h |  2 +-
 2 files changed, 5 insertions(+), 28 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0d4713307724..5bf6399588a8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3012,26 +3012,10 @@ int nvme_get_log(struct nvme_ctrl *ctrl, u32 nsid, u8 log_page, u8 lsp, u8 csi,
 	return nvme_submit_sync_cmd(ctrl->admin_q, &c, log, size);
 }
 
-static struct nvme_cel *nvme_find_cel(struct nvme_ctrl *ctrl, u8 csi)
-{
-	struct nvme_cel *cel, *ret = NULL;
-
-	spin_lock_irq(&ctrl->lock);
-	list_for_each_entry(cel, &ctrl->cels, entry) {
-		if (cel->csi == csi) {
-			ret = cel;
-			break;
-		}
-	}
-	spin_unlock_irq(&ctrl->lock);
-
-	return ret;
-}
-
 static int nvme_get_effects_log(struct nvme_ctrl *ctrl, u8 csi,
 				struct nvme_effects_log **log)
 {
-	struct nvme_cel *cel = nvme_find_cel(ctrl, csi);
+	struct nvme_cel *cel = xa_load(&ctrl->cels, csi);
 	int ret;
 
 	if (cel)
@@ -3049,10 +3033,7 @@ static int nvme_get_effects_log(struct nvme_ctrl *ctrl, u8 csi,
 	}
 
 	cel->csi = csi;
-
-	spin_lock_irq(&ctrl->lock);
-	list_add_tail(&cel->entry, &ctrl->cels);
-	spin_unlock_irq(&ctrl->lock);
+	xa_store(&ctrl->cels, cel->csi, cel, GFP_KERNEL);
 out:
 	*log = &cel->log;
 	return 0;
@@ -4431,15 +4412,11 @@ static void nvme_free_ctrl(struct device *dev)
 	struct nvme_ctrl *ctrl =
 		container_of(dev, struct nvme_ctrl, ctrl_device);
 	struct nvme_subsystem *subsys = ctrl->subsys;
-	struct nvme_cel *cel, *next;
 
 	if (!subsys || ctrl->instance != subsys->instance)
 		ida_simple_remove(&nvme_instance_ida, ctrl->instance);
 
-	list_for_each_entry_safe(cel, next, &ctrl->cels, entry) {
-		list_del(&cel->entry);
-		kfree(cel);
-	}
+	xa_destroy(&ctrl->cels);
 
 	nvme_mpath_uninit(ctrl);
 	__free_page(ctrl->discard_page);
@@ -4471,7 +4448,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	spin_lock_init(&ctrl->lock);
 	mutex_init(&ctrl->scan_lock);
 	INIT_LIST_HEAD(&ctrl->namespaces);
-	INIT_LIST_HEAD(&ctrl->cels);
+	xa_init(&ctrl->cels);
 	init_rwsem(&ctrl->namespaces_rwsem);
 	ctrl->dev = dev;
 	ctrl->ops = ops;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1e6aaa710262..07bdbdc19fa0 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -300,7 +300,7 @@ struct nvme_ctrl {
 	unsigned long quirks;
 	struct nvme_id_power_state psd[32];
 	struct nvme_effects_log *effects;
-	struct list_head cels;
+	struct xarray cels;
 	struct work_struct scan_work;
 	struct work_struct async_event_work;
 	struct delayed_work ka_work;
-- 
2.22.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-core: use xarray for cel storing

[Christoph Hellwig]

On Tue, Sep 22, 2020 at 02:05:29PM -0700, Chaitanya Kulkarni wrote:
> When using linked list we have to open code the locking, search, and
> destroy operations with the loops even if data structure doesn't fall
> into the fast path.
> 
> One of the main advantage of having XArray to store, search, and remove
> items is that it handles all the locking by itself, avoids the loops 
> when using linked lists, provides clear API to replace the linked list's
> search and destroy loops. 
> 
> This patch replaces the ctrl->cel list with XArray and removes :-
> 
> a. Extra code needed for the linked list for ctrl->cel item management
>    such as nvme_find_cel().
> b. Destroy loop in the nvme_free_ctrl().
> c. Explicit insertion locking in the nvme_get_effects_log().

A nice, I thought of that I while ago but never got to it.  This looks
exactly like what I had in mind.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-core: use xarray for cel storing

[Keith Busch]

On Tue, Sep 22, 2020 at 02:05:29PM -0700, Chaitanya Kulkarni wrote:
> When using linked list we have to open code the locking, search, and
> destroy operations with the loops even if data structure doesn't fall
> into the fast path.
> 
> One of the main advantage of having XArray to store, search, and remove
> items is that it handles all the locking by itself, avoids the loops 
> when using linked lists, provides clear API to replace the linked list's
> search and destroy loops. 
> 
> This patch replaces the ctrl->cel list with XArray and removes :-
> 
> a. Extra code needed for the linked list for ctrl->cel item management
>    such as nvme_find_cel().
> b. Destroy loop in the nvme_free_ctrl().
> c. Explicit insertion locking in the nvme_get_effects_log().
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  drivers/nvme/host/core.c | 31 ++++---------------------------
>  drivers/nvme/host/nvme.h |  2 +-
>  2 files changed, 5 insertions(+), 28 deletions(-)

That's some good looking change stats.

And removing the overloaded use for ctrl->lock will help avoid potential
coding errors in future patches.

So, this looks fine to me.

Reviewed-by: Keith Busch <kbusch@kernel.org>

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-core: use xarray for cel storing

[Christoph Hellwig]

Thanks,

applied to nvme-5.10.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme