nvme: take node locality into account when selecting a path

PATCH New Christoph Hellwig 4 participants 13 messages Sep 27, 2018 – Oct 4, 2018 NVMe

Share & tools

Christoph Hellwig

author maintainer

2018-09-27 23:05 UTC #0 lore raw

PATCH 1/1 nvme: take node locality into account when selecting a path

Make current_path an array with an entry for every possible node, and cache the best path on a per-node basis. Take the node distance into account when selecting it. This is primarily useful for dual-ported PCIe devices which are connected to PCIe root ports on different sockets.

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/nvme/host/core.c      |  7 ++++-
 drivers/nvme/host/multipath.c | 50 +++++++++++++++++++++++++++--------
 drivers/nvme/host/nvme.h      | 25 +++++++-----------
 3 files changed, 54 insertions(+), 28 deletions(-)

drivers/nvme/host/core.c +6 -1

index 41a6180649d2..eb10b263de8f 100644
@@ -2900,9 +2900,14 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 		unsigned nsid, struct nvme_id_ns *id)
 {
 	struct nvme_ns_head *head;
+	size_t size = sizeof(*head);
 	int ret = -ENOMEM;
 
-	head = kzalloc(sizeof(*head), GFP_KERNEL);
+#ifdef CONFIG_NVME_MULTIPATH
+	size += num_possible_nodes() * sizeof(struct nvme_ns *);
+#endif
+
+	head = kzalloc(size, GFP_KERNEL);
 	if (!head)
 		goto out;
 	ret = ida_simple_get(&ctrl->subsys->ns_ida, 1, 0, GFP_KERNEL);

drivers/nvme/host/multipath.c +39 ~~-11~~

85 lines not on this page · raw message

drivers/nvme/host/nvme.h +9 ~~-16~~

index bb4a2003c097..5eec5b9205f3 100644
@@ -277,14 +277,6 @@ struct nvme_ns_ids {
  * only ever has a single entry for private namespaces.
  */
 struct nvme_ns_head {
-#ifdef CONFIG_NVME_MULTIPATH
-	struct gendisk		*disk;
-	struct nvme_ns __rcu	*current_path;
-	struct bio_list		requeue_list;
-	spinlock_t		requeue_lock;
-	struct work_struct	requeue_work;
-	struct mutex		lock;
-#endif
 	struct list_head	list;
 	struct srcu_struct      srcu;
 	struct nvme_subsystem	*subsys;

@@ -293,6 +285,14 @@ struct nvme_ns_head {
 	struct list_head	entry;
 	struct kref		ref;
 	int			instance;
+#ifdef CONFIG_NVME_MULTIPATH
+	struct gendisk		*disk;
+	struct bio_list		requeue_list;
+	spinlock_t		requeue_lock;
+	struct work_struct	requeue_work;
+	struct mutex		lock;
+	struct nvme_ns __rcu	*current_path[];
+#endif
 };
 
 #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS

@@ -474,14 +474,7 @@ void nvme_mpath_remove_disk(struct nvme_ns_head *head);
 int nvme_mpath_init(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id);
 void nvme_mpath_uninit(struct nvme_ctrl *ctrl);
 void nvme_mpath_stop(struct nvme_ctrl *ctrl);
-
-static inline void nvme_mpath_clear_current_path(struct nvme_ns *ns)
-{
-	struct nvme_ns_head *head = ns->head;
-
-	if (head && ns == rcu_access_pointer(head->current_path))
-		rcu_assign_pointer(head->current_path, NULL);
-}
+void nvme_mpath_clear_current_path(struct nvme_ns *ns);
 struct nvme_ns *nvme_find_path(struct nvme_ns_head *head);
 
 static inline void nvme_mpath_check_last_path(struct nvme_ns *ns)

Signature · 1 line

2.19.0

Keith Busch

2018-09-28 14:12 UTC #1 lore raw

PATCH 1/1 nvme: take node locality into account when selecting a path

Quoted 6 lines · Christoph Hellwig

Make current_path an array with an entry for every possible node, and cache the best path on a per-node basis. Take the node distance into account when selecting it. This is primarily useful for dual-ported PCIe devices which are connected to PCIe root ports on different sockets.
Signed-off-by Christoph Hellwig <hch at lst.de>

Looks great!

Reviewed-by Keith Busch <keith.busch at intel.com>

Sagi Grimberg

maintainer

2018-09-28 22:31 UTC #2 lore raw

PATCH 1/1 nvme: take node locality into account when selecting a path

In reply to Christoph Hellwig

drivers/nvme/host/multipath.c static const char *nvme_ana_state_names[] = { · line 137

Quoted 12 lines

drivers/nvme/host/multipath.c +6 -0

+static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
+{
+	int found_distance = INT_MAX, fallback_distance = INT_MAX, distance;
+	struct nvme_ns *found = NULL, *fallback = NULL, *ns;
 
  	list_for_each_entry_rcu(ns, &head->list, siblings) {
  		if (ns->ctrl->state != NVME_CTRL_LIVE ||
  		    test_bit(NVME_NS_ANA_PENDING, &ns->flags))
  			continue;
+
+		distance = node_distance(node, dev_to_node(ns->ctrl->dev));

Can we make this useful also for fabrics who have device affinity knowledge? ctrl->dev is useless for fabrics...

Perhaps make it a ctrl->ops->devnode() or something?

Christoph Hellwig

author maintainer

2018-09-30 23:01 UTC #3 lore raw

PATCH 1/1 nvme: take node locality into account when selecting a path

Quoted 15 lines · Sagi Grimberg

drivers/nvme/host/multipath.c static const char *nvme_ana_state_names[] = { · line 137
Quoted 11 lines
drivers/nvme/host/multipath.c +6 -0
+static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
+{
+	int found_distance = INT_MAX, fallback_distance = INT_MAX, distance;
+	struct nvme_ns *found = NULL, *fallback = NULL, *ns;
    	list_for_each_entry_rcu(ns, &head->list, siblings) {
  		if (ns->ctrl->state != NVME_CTRL_LIVE ||
  		    test_bit(NVME_NS_ANA_PENDING, &ns->flags))
  			continue;
+
+		distance = node_distance(node, dev_to_node(ns->ctrl->dev));
Can we make this useful also for fabrics who have device affinity knowledge? ctrl->dev is useless for fabrics...

Quoted 1 line

Perhaps make it a ctrl->ops->devnode() or something?

I'd prefer a pointer of field (we could copy the node over) over an indirect call. But I'll happily leave that to the person that actually implements the functionality to fabrics.

Sagi Grimberg

maintainer

2018-10-01 19:45 UTC #4 lore raw

PATCH 1/1 nvme: take node locality into account when selecting a path

Quoted 15 lines

Quoted 15 lines
drivers/nvme/host/multipath.c static const char *nvme_ana_state_names[] = { · line 137
Quoted 11 lines
drivers/nvme/host/multipath.c +6 -0
+static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
+{
+	int found_distance = INT_MAX, fallback_distance = INT_MAX, distance;
+	struct nvme_ns *found = NULL, *fallback = NULL, *ns;
     	list_for_each_entry_rcu(ns, &head->list, siblings) {
   		if (ns->ctrl->state != NVME_CTRL_LIVE ||
   		    test_bit(NVME_NS_ANA_PENDING, &ns->flags))
   			continue;
+
+		distance = node_distance(node, dev_to_node(ns->ctrl->dev));
Can we make this useful also for fabrics who have device affinity knowledge? ctrl->dev is useless for fabrics...

Quoted 5 lines

Quoted 1 line
Perhaps make it a ctrl->ops->devnode() or something?
I'd prefer a pointer of field (we could copy the node over) over an indirect call. But I'll happily leave that to the person that actually implements the functionality to fabrics.

Fair enough... I can follow up on that.

Hannes Reinecke

2018-10-02 17:30 UTC #5 lore raw

PATCH 1/1 nvme: take node locality into account when selecting a path

Quoted 17 lines · Sagi Grimberg

Quoted 17 lines
Quoted 17 lines
Quoted 13 lines
+static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
+{
+??? int found_distance = INT_MAX, fallback_distance = INT_MAX, distance;
+??? struct nvme_ns *found = NULL, *fallback = NULL, *ns; ???????? list_for_each_entry_rcu(ns, &head->list, siblings) { ?????????? if (ns->ctrl->state != NVME_CTRL_LIVE || ?????????????? test_bit(NVME_NS_ANA_PENDING, &ns->flags)) ?????????????? continue;
+
+??????? distance = node_distance(node, dev_to_node(ns->ctrl->dev));
Can we make this useful also for fabrics who have device affinity knowledge? ctrl->dev is useless for fabrics...

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Perhaps make it a ctrl->ops->devnode() or something?
I'd prefer a pointer of field (we could copy the node over) over an indirect call.? But I'll happily leave that to the person that actually implements the functionality to fabrics.
Fair enough... I can follow up on that.

Something like this?

Cheers,

Hannes

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-nvme-use-NUMA-locality-information-for-fabrics.patch
Type: text/x-patch
Size: 4330 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20181002/2ea1784e/attachment-0001.bin>

Christoph Hellwig

author maintainer

2018-10-02 17:39 UTC #6 lore raw

PATCH 1/1 nvme: take node locality into account when selecting a path

Quoted 3 lines · Hannes Reinecke

Quoted 2 lines
Fair enough... I can follow up on that.
Something like this?

As ?aid I'd rather avoid the indirect call if at all possible.

Please either add a numa_id field to struct nvme_ctrl, or a locality_dev or something.

Hannes Reinecke

2018-10-03 08:56 UTC #7 lore raw

PATCH 1/1 nvme: take node locality into account when selecting a path

Quoted 10 lines · Christoph Hellwig

Quoted 3 lines · Hannes Reinecke
Quoted 2 lines
Fair enough... I can follow up on that.
Something like this?
As ?aid I'd rather avoid the indirect call if at all possible.
Please either add a numa_id field to struct nvme_ctrl, or a locality_dev or something.

Ah. So that should be more like it.

Cheers,

Hannes

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-nvme-NUMA-locality-information-for-fabrics.patch
Type: text/x-patch
Size: 5801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20181003/104480f2/attachment-0001.bin>

Christoph Hellwig

author maintainer

2018-10-03 12:43 UTC #8 lore raw

PATCH 1/1 nvme: take node locality into account when selecting a path

Yes, something like that. Let me know when this passes some basic FC and RDMA testing and we can merge it.

Quoted 162 lines · Hannes Reinecke

Quoted 10 lines · Christoph Hellwig

Quoted 3 lines · Hannes Reinecke
Quoted 2 lines
Fair enough... I can follow up on that.
Something like this?
As ?aid I'd rather avoid the indirect call if at all possible.
Please either add a numa_id field to struct nvme_ctrl, or a locality_dev or something.

Ah. So that should be more like it.

Cheers,

Hannes

Quoted 1 line

From 478db61eab3f7a178a0c1f2e5c88c742cf5006ab Mon Sep 17 00:00:00 2001

From: Hannes Reinecke <hare at suse.com>
Date: Wed, 3 Oct 2018 10:53:05 +0200
Subject: [PATCH] nvme: NUMA locality information for fabrics

Add a new field 'node_id' to the nvme_ctrl structure to hold the NUMA locality information of the underlying hardware. With that we can allocate the memory structures on the same NUMA node as the underlying hardware.

Signed-off-by Hannes Reinecke <hare at suse.com>

---
 drivers/nvme/host/core.c      | 2 +-
 drivers/nvme/host/fc.c        | 5 +++--
 drivers/nvme/host/multipath.c | 4 ++--
 drivers/nvme/host/nvme.h      | 1 +
 drivers/nvme/host/pci.c       | 1 +
 drivers/nvme/host/rdma.c      | 6 +++---
 6 files changed, 11 insertions(+), 8 deletions(-)

drivers/nvme/host/core.c +1 -1

index 2db33a752e2b..0ec56e4916ea 100644
@@ -3055,7 +3055,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	struct gendisk *disk;
 	struct nvme_id_ns *id;
 	char disk_name[DISK_NAME_LEN];
-	int node = dev_to_node(ctrl->dev), flags = GENHD_FL_EXT_DEVT;
+	int node = ctrl->node_id, flags = GENHD_FL_EXT_DEVT;
 
 	ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
 	if (!ns)

drivers/nvme/host/fc.c +3 -2

index 9d201b35397d..7f246dd04bc5 100644
@@ -2422,7 +2422,7 @@ nvme_fc_create_io_queues(struct nvme_fc_ctrl *ctrl)
 	ctrl->tag_set.ops = &nvme_fc_mq_ops;
 	ctrl->tag_set.queue_depth = ctrl->ctrl.opts->queue_size;
 	ctrl->tag_set.reserved_tags = 1; /* fabric connect */
-	ctrl->tag_set.numa_node = NUMA_NO_NODE;
+	ctrl->tag_set.numa_node = ctrl->ctrl.node_id;
 	ctrl->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
 	ctrl->tag_set.cmd_size = sizeof(struct nvme_fc_fcp_op) +
 					(SG_CHUNK_SIZE *

@@ -2990,6 +2990,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 
 	ctrl->ctrl.opts = opts;
 	ctrl->ctrl.nr_reconnects = 0;
+	ctrl->ctrl.node_id = dev_to_node(lport->dev);
 	INIT_LIST_HEAD(&ctrl->ctrl_list);
 	ctrl->lport = lport;
 	ctrl->rport = rport;

@@ -3028,7 +3029,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	ctrl->admin_tag_set.ops = &nvme_fc_admin_mq_ops;
 	ctrl->admin_tag_set.queue_depth = NVME_AQ_MQ_TAG_DEPTH;
 	ctrl->admin_tag_set.reserved_tags = 2; /* fabric connect + Keep-Alive */
-	ctrl->admin_tag_set.numa_node = NUMA_NO_NODE;
+	ctrl->admin_tag_set.numa_node = ctrl->ctrl.node_id;
 	ctrl->admin_tag_set.cmd_size = sizeof(struct nvme_fc_fcp_op) +
 					(SG_CHUNK_SIZE *
 						sizeof(struct scatterlist)) +

drivers/nvme/host/multipath.c +2 -2

index 31f29f97374b..2616251b3236 100644
@@ -141,7 +141,7 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
 		    test_bit(NVME_NS_ANA_PENDING, &ns->flags))
 			continue;
 
-		distance = node_distance(node, dev_to_node(ns->ctrl->dev));
+		distance = node_distance(node, ns->ctrl->node_id);
 
 		switch (ns->ana_state) {
 		case NVME_ANA_OPTIMIZED:

@@ -276,7 +276,7 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
 	if (!(ctrl->subsys->cmic & (1 << 1)) || !multipath)
 		return 0;
 
-	q = blk_alloc_queue_node(GFP_KERNEL, NUMA_NO_NODE, NULL);
+	q = blk_alloc_queue_node(GFP_KERNEL, ctrl->node_id, NULL);
 	if (!q)
 		goto out;
 	q->queuedata = head;

drivers/nvme/host/nvme.h +1 -0

index 9fefba039d1e..55347a547d84 100644
@@ -153,6 +153,7 @@ struct nvme_ctrl {
 	struct request_queue *connect_q;
 	struct device *dev;
 	int instance;
+	int node_id;
 	struct blk_mq_tag_set *tagset;
 	struct blk_mq_tag_set *admin_tagset;
 	struct list_head namespaces;

drivers/nvme/host/pci.c +1 -0

index d668682f91df..b5d37aacf212 100644
@@ -2517,6 +2517,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	dev_info(dev->ctrl.device, "pci function %s\n", dev_name(&pdev->dev));
 
+	dev->ctrl.node_id = node;
 	nvme_get_ctrl(&dev->ctrl);
 	async_schedule(nvme_async_probe, dev);

drivers/nvme/host/rdma.c +3 -3

index dc042017c293..a64b02c13934 100644
@@ -686,7 +686,7 @@ static struct blk_mq_tag_set *nvme_rdma_alloc_tagset(struct nvme_ctrl *nctrl,
 		set->ops = &nvme_rdma_admin_mq_ops;
 		set->queue_depth = NVME_AQ_MQ_TAG_DEPTH;
 		set->reserved_tags = 2; /* connect + keep-alive */
-		set->numa_node = NUMA_NO_NODE;
+		set->numa_node = nctrl->node_id;
 		set->cmd_size = sizeof(struct nvme_rdma_request) +
 			SG_CHUNK_SIZE * sizeof(struct scatterlist);
 		set->driver_data = ctrl;

@@ -699,7 +699,7 @@ static struct blk_mq_tag_set *nvme_rdma_alloc_tagset(struct nvme_ctrl *nctrl,
 		set->ops = &nvme_rdma_mq_ops;
 		set->queue_depth = nctrl->sqsize + 1;
 		set->reserved_tags = 1; /* fabric connect */
-		set->numa_node = NUMA_NO_NODE;
+		set->numa_node = nctrl->node_id;
 		set->flags = BLK_MQ_F_SHOULD_MERGE;
 		set->cmd_size = sizeof(struct nvme_rdma_request) +
 			SG_CHUNK_SIZE * sizeof(struct scatterlist);

@@ -1975,7 +1975,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 	ctrl->ctrl.queue_count = opts->nr_io_queues + 1; /* +1 for admin queue */
 	ctrl->ctrl.sqsize = opts->queue_size - 1;
 	ctrl->ctrl.kato = opts->kato;
-
+	ctrl->ctrl.node_id = NUMA_NO_NODE;
 	ret = -ENOMEM;
 	ctrl->queues = kcalloc(ctrl->ctrl.queue_count, sizeof(*ctrl->queues),
 				GFP_KERNEL);

Signature · 1 line

2.13.7

---end quoted text---

Sagi Grimberg

maintainer

2018-10-04 01:30 UTC #9 lore raw

PATCH 1/1 nvme: take node locality into account when selecting a path

In reply to Hannes Reinecke

Quoted 2 lines

Ah. So that should be more like it.

For it to be useful for RDMA you'll want to add:
--

drivers/nvme/host/rdma.c +0 -0

index ea325a636588..a926c370e83a 100644
@@ -755,6 +755,7 @@ static int nvme_rdma_configure_admin_queue(struct

nvme_rdma_ctrl *ctrl,

                 return error;

         ctrl->device = ctrl->queues[0].device;

+ ctrl->ctrl.node_id = dev_to_node(device->dev->dma_device);

         ctrl->max_fr_pages = nvme_rdma_get_max_fr_pages(ctrl->device->dev);

Hannes Reinecke

2018-10-04 15:40 UTC #10 lore raw

PATCH 1/1 nvme: take node locality into account when selecting a path

Quoted 17 lines · Sagi Grimberg

Quoted 2 lines

Ah. So that should be more like it.

For it to be useful for RDMA you'll want to add:

drivers/nvme/host/rdma.c +1 -0

index ea325a636588..a926c370e83a 100644
@@ -755,6 +755,7 @@ static int nvme_rdma_configure_admin_queue(struct 
 nvme_rdma_ctrl *ctrl,
 ??????????????? return error;
 
 ??????? ctrl->device = ctrl->queues[0].device;
+?????? ctrl->ctrl.node_id = dev_to_node(device->dev->dma_device);
 
 ??????? ctrl->max_fr_pages =

Quoted 2 lines

nvme_rdma_get_max_fr_pages(ctrl->device->dev);

Ah, I knew I could count on you :-)
The mapping to the underlying hardware in RDMA had got me confused.

Will be fixing it up and resend the patch.

Cheers,

Hannes

Signature · 5 lines

Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Hannes Reinecke

2018-09-29 12:09 UTC #11 lore raw

PATCH 1/1 nvme: take node locality into account when selecting a path

In reply to Christoph Hellwig

Quoted 5 lines · Christoph Hellwig

Make current_path an array with an entry for every possible node, and cache the best path on a per-node basis. Take the node distance into account when selecting it. This is primarily useful for dual-ported PCIe devices which are connected to PCIe root ports on different sockets.

Quoted 7 lines

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/nvme/host/core.c      |  7 ++++-
 drivers/nvme/host/multipath.c | 50 +++++++++++++++++++++++++++--------
 drivers/nvme/host/nvme.h      | 25 +++++++-----------
 3 files changed, 54 insertions(+), 28 deletions(-)

Actually I would love to have some statistic or debug info showing the NUMA locality mapping somewhere. Otherwise the admin might have a hard time figuring out what's going on here, plus debugging any I/O routing issues will be tricky.

But I guess we can do it with a later patch.

So:

Reviewed-by Hannes Reinecke <hare at suse.com>

Cheers,

Hannes

Signature · 5 lines

Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Christoph Hellwig

author maintainer

2018-09-30 22:59 UTC #12 lore raw

PATCH 1/1 nvme: take node locality into account when selecting a path

Quoted 4 lines · Hannes Reinecke

Actually I would love to have some statistic or debug info showing the NUMA locality mapping somewhere. Otherwise the admin might have a hard time figuring out what's going on here, plus debugging any I/O routing issues will be tricky.

You already see the node of the devices in sysfs, so what else do you think of?