Skip to contentSkip to conversation

block and nvme polling improvements V3

PATCH In review Christoph Hellwig 5 participants 41 messages Dec 2, 2018 – Dec 4, 2018

Subsystems (2)
Block layer NVMe
lore
Share & tools
lore

Christoph Hellwig

author maintainer
#0 lore raw

Hi all,

this series optimizes a few bits in the block layer and nvme code related to polling.

It starts by moving the queue types recently introduce entirely into the block layer instead of requiring an indirect call for them.

It then switches nvme and the block layer to only allow polling with separate poll queues, which allows us to realize the following benefits:

- poll queues can safely avoid disabling irqs on any locks (we already do that in NVMe, but it isn't 100% kosher as-is)
- regular interrupt driven queues can drop the CQ lock entirely, as we won't race for completing CQs

Then we drop the NVMe RDMA code, as it doesn't follow the new mode, and remove the nvme multipath polling code including the block hooks for it, which didn't make much sense to start with given that we started bypassing the multipath code for single controller subsystems early on. Last but not least we enable polling in the block layer by default if the underlying driver has poll queues, as that already requires explicit user action.

Note that it would be really nice to have polling back for RDMA with dedicated poll queues, but that might take a while. Also based on Jens' polling aio patches we could now implement a model in nvmet where we have a thread polling both the backend nvme device and the RDMA CQs, which might give us some pretty nice performace (I know Sagi looked into something similar a while ago).

A git tree is also available at:

git://git.infradead.org/users/hch/block.git nvme-polling

Gitweb:

http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/nvme-polling

Changes since v2:
- fix a changelog typo
- report a string instead of an index from the type sysfs attribute
- move to a per-queue completions for queue deletion
- clear NVMEQ_DELETE_ERROR when initializing a queue

Changes since v1:
- rebased to the latest block for-4.21 tree

author maintainer
#1 lore raw

PATCH 1/13 block: move queues types to the block layer

Having another indirect all in the fast path doesn't really help in our post-spectre world. Also having too many queue type is just going to create confusion, so I'd rather manage them centrally.

Note that the queue type naming and ordering changes a bit - the first index now is the default queue for everything not explicitly marked, the optional ones are read and poll queues.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 block/blk-mq-sysfs.c    |  9 +++++-
 block/blk-mq.h          | 21 +++++++------
 drivers/nvme/host/pci.c | 68 +++++++++++++++--------------------------
 include/linux/blk-mq.h  | 15 ++++-----
 4 files changed, 51 insertions(+), 62 deletions(-)
block/blk-mq-sysfs.c +8 -1
index 6efef1f679f0..9c2df137256a 100644
@@ -173,9 +173,16 @@ static ssize_t blk_mq_hw_sysfs_cpus_show(struct blk_mq_hw_ctx *hctx, char *page)
 	return ret;
 }
 
+static const char *const hctx_types[] = {
+	[HCTX_TYPE_DEFAULT]	= "default",
+	[HCTX_TYPE_READ]	= "read",
+	[HCTX_TYPE_POLL]	= "poll",
+};
+
 static ssize_t blk_mq_hw_sysfs_type_show(struct blk_mq_hw_ctx *hctx, char *page)
 {
-	return sprintf(page, "%u\n", hctx->type);
+	BUILD_BUG_ON(ARRAY_SIZE(hctx_types) != HCTX_MAX_TYPES);
+	return sprintf(page, "%s\n", hctx_types[hctx->type]);
 }
 
 static struct attribute *default_ctx_attrs[] = {
block/blk-mq.h +12 -9
index 7291e5379358..a664ea44ffd4 100644
@@ -81,16 +81,14 @@ extern int blk_mq_hw_queue_to_node(struct blk_mq_queue_map *qmap, unsigned int);
 /*
  * blk_mq_map_queue_type() - map (hctx_type,cpu) to hardware queue
  * @q: request queue
- * @hctx_type: the hctx type index
+ * @type: the hctx type index
  * @cpu: CPU
  */
 static inline struct blk_mq_hw_ctx *blk_mq_map_queue_type(struct request_queue *q,
-							  unsigned int hctx_type,
+							  enum hctx_type type,
 							  unsigned int cpu)
 {
-	struct blk_mq_tag_set *set = q->tag_set;
-
-	return q->queue_hw_ctx[set->map[hctx_type].mq_map[cpu]];
+	return q->queue_hw_ctx[q->tag_set->map[type].mq_map[cpu]];
 }
 
 /*

@@ -103,12 +101,17 @@ static inline struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *q,
 						     unsigned int flags,
 						     unsigned int cpu)
 {
-	int hctx_type = 0;
+	enum hctx_type type = HCTX_TYPE_DEFAULT;
+
+	if (q->tag_set->nr_maps > HCTX_TYPE_POLL &&
+	    ((flags & REQ_HIPRI) && test_bit(QUEUE_FLAG_POLL, &q->queue_flags)))
+		type = HCTX_TYPE_POLL;
 
-	if (q->mq_ops->rq_flags_to_type)
-		hctx_type = q->mq_ops->rq_flags_to_type(q, flags);
+	else if (q->tag_set->nr_maps > HCTX_TYPE_READ &&
+		 ((flags & REQ_OP_MASK) == REQ_OP_READ))
+		type = HCTX_TYPE_READ;
 
-	return blk_mq_map_queue_type(q, hctx_type, cpu);
+	return blk_mq_map_queue_type(q, type, cpu);
 }
 
 /*
drivers/nvme/host/pci.c +25 -43

157 lines not on this page · raw message

include/linux/blk-mq.h +6 -9
index 467f1dd21ccf..57eda7b20243 100644
@@ -81,8 +81,12 @@ struct blk_mq_queue_map {
 	unsigned int queue_offset;
 };
 
-enum {
-	HCTX_MAX_TYPES = 3,
+enum hctx_type {
+	HCTX_TYPE_DEFAULT,	/* all I/O not otherwise accounted for */
+	HCTX_TYPE_READ,		/* just for READ I/O */
+	HCTX_TYPE_POLL,		/* polled I/O of any kind */
+
+	HCTX_MAX_TYPES,
 };
 
 struct blk_mq_tag_set {

@@ -118,8 +122,6 @@ struct blk_mq_queue_data {
 typedef blk_status_t (queue_rq_fn)(struct blk_mq_hw_ctx *,
 		const struct blk_mq_queue_data *);
 typedef void (commit_rqs_fn)(struct blk_mq_hw_ctx *);
-/* takes rq->cmd_flags as input, returns a hardware type index */
-typedef int (rq_flags_to_type_fn)(struct request_queue *, unsigned int);
 typedef bool (get_budget_fn)(struct blk_mq_hw_ctx *);
 typedef void (put_budget_fn)(struct blk_mq_hw_ctx *);
 typedef enum blk_eh_timer_return (timeout_fn)(struct request *, bool);

@@ -154,11 +156,6 @@ struct blk_mq_ops {
 	 */
 	commit_rqs_fn		*commit_rqs;
 
-	/*
-	 * Return a queue map type for the given request/bio flags
-	 */
-	rq_flags_to_type_fn	*rq_flags_to_type;
-
 	/*
 	 * Reserve budget before queue request, once .queue_rq is
 	 * run, it is driver's responsibility to release the
Signature · 1 line
2.19.1
maintainer Reviewed-by on v1 1/13
#2 lore raw

Re: [PATCH 01/13] block: move queues types to the block layer

Quoted 68 lines · Christoph Hellwig

Having another indirect all in the fast path doesn't really help in our post-spectre world. Also having too many queue type is just going to create confusion, so I'd rather manage them centrally.

Note that the queue type naming and ordering changes a bit - the first index now is the default queue for everything not explicitly marked, the optional ones are read and poll queues.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
  block/blk-mq-sysfs.c    |  9 +++++-
  block/blk-mq.h          | 21 +++++++------
  drivers/nvme/host/pci.c | 68 +++++++++++++++--------------------------
  include/linux/blk-mq.h  | 15 ++++-----
  4 files changed, 51 insertions(+), 62 deletions(-)
block/blk-mq-sysfs.c +8 -1
index 6efef1f679f0..9c2df137256a 100644
@@ -173,9 +173,16 @@ static ssize_t blk_mq_hw_sysfs_cpus_show(struct blk_mq_hw_ctx *hctx, char *page)
  	return ret;
  }
 
+static const char *const hctx_types[] = {
+	[HCTX_TYPE_DEFAULT]	= "default",
+	[HCTX_TYPE_READ]	= "read",
+	[HCTX_TYPE_POLL]	= "poll",
+};
+
  static ssize_t blk_mq_hw_sysfs_type_show(struct blk_mq_hw_ctx *hctx, char *page)
  {
-	return sprintf(page, "%u\n", hctx->type);
+	BUILD_BUG_ON(ARRAY_SIZE(hctx_types) != HCTX_MAX_TYPES);
+	return sprintf(page, "%s\n", hctx_types[hctx->type]);
  }
 
  static struct attribute *default_ctx_attrs[] = {
block/blk-mq.h +4 -6
index 7291e5379358..a664ea44ffd4 100644
@@ -81,16 +81,14 @@ extern int blk_mq_hw_queue_to_node(struct blk_mq_queue_map *qmap, unsigned int);
  /*
   * blk_mq_map_queue_type() - map (hctx_type,cpu) to hardware queue
   * @q: request queue
- * @hctx_type: the hctx type index
+ * @type: the hctx type index
   * @cpu: CPU
   */
  static inline struct blk_mq_hw_ctx *blk_mq_map_queue_type(struct request_queue *q,
-							  unsigned int hctx_type,
+							  enum hctx_type type,
  							  unsigned int cpu)
  {
-	struct blk_mq_tag_set *set = q->tag_set;
-
-	return q->queue_hw_ctx[set->map[hctx_type].mq_map[cpu]];
+	return q->queue_hw_ctx[q->tag_set->map[type].mq_map[cpu]];
  }
 
  /*

@@ -103,12 +101,17 @@ static inline struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *q,
  						     unsigned int flags,
  						     unsigned int cpu)
  {
-	int hctx_type = 0;
+	enum hctx_type type = HCTX_TYPE_DEFAULT;

block/blk-mq.h static inline struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *q, · line 112

Quoted 10 lines
block/blk-mq.h +7 -2
+
+	if (q->tag_set->nr_maps > HCTX_TYPE_POLL &&
+	    ((flags & REQ_HIPRI) && test_bit(QUEUE_FLAG_POLL, &q->queue_flags)))
+		type = HCTX_TYPE_POLL;
 
-	if (q->mq_ops->rq_flags_to_type)
-		hctx_type = q->mq_ops->rq_flags_to_type(q, flags);
+	else if (q->tag_set->nr_maps > HCTX_TYPE_READ &&
+		 ((flags & REQ_OP_MASK) == REQ_OP_READ))
+		type = HCTX_TYPE_READ;

Nit, there seems to be an extra newline that can be omitted here before the else if statement (if I'm reading this correctly)...

Otherwise looks good,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
author maintainer
#3 lore raw

Re: [PATCH 01/13] block: move queues types to the block layer

Quoted 10 lines · Sagi Grimberg
Quoted 10 lines
block/blk-mq.h +5 -1
@@ -103,12 +101,17 @@ static inline struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *q,
  						     unsigned int flags,
  						     unsigned int cpu)
  {
-	int hctx_type = 0;
+	enum hctx_type type = HCTX_TYPE_DEFAULT;
+
+	if (q->tag_set->nr_maps > HCTX_TYPE_POLL &&
+	    ((flags & REQ_HIPRI) && test_bit(QUEUE_FLAG_POLL, &q->queue_flags)))
+		type = HCTX_TYPE_POLL;
Quoted 8 lines

block/blk-mq.h static inline struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *q, · line 112

Quoted 5 lines
block/blk-mq.h +3 -1
  -	if (q->mq_ops->rq_flags_to_type)
-		hctx_type = q->mq_ops->rq_flags_to_type(q, flags);
+	else if (q->tag_set->nr_maps > HCTX_TYPE_READ &&
+		 ((flags & REQ_OP_MASK) == REQ_OP_READ))
+		type = HCTX_TYPE_READ;

Nit, there seems to be an extra newline that can be omitted here before the else if statement (if I'm reading this correctly)...

Empty lines can always be ommited, but in this case I actually like it as it seems to help readability..

maintainer
#4 lore raw

Re: [PATCH 01/13] block: move queues types to the block layer

Quoted 6 lines
Quoted 3 lines

Nit, there seems to be an extra newline that can be omitted here before the else if statement (if I'm reading this correctly)...

Empty lines can always be ommited, but in this case I actually like it as it seems to help readability..

If you think its useful I'm fine with it as is...

author maintainer
#5 lore raw

PATCH 2/13 nvme-pci: use atomic bitops to mark a queue enabled

In reply to Christoph Hellwig

This gets rid of all the messing with cq_vector and the ->polled field by using an atomic bitop to mark the queue enabled or not.

Signed-off-by Christoph Hellwig <hch at lst.de>
Reviewed-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 43 ++++++++++++++---------------------------
 1 file changed, 15 insertions(+), 28 deletions(-)
drivers/nvme/host/pci.c +15 -28
index a1bb4bb92e7f..022395a319f4 100644
@@ -201,7 +201,8 @@ struct nvme_queue {
 	u16 last_cq_head;
 	u16 qid;
 	u8 cq_phase;
-	u8 polled;
+	unsigned long flags;
+#define NVMEQ_ENABLED		0
 	u32 *dbbuf_sq_db;
 	u32 *dbbuf_cq_db;
 	u32 *dbbuf_sq_ei;

@@ -927,7 +928,7 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	 * We should not need to do this, but we're still using this to
 	 * ensure we can drain requests on a dying queue.
 	 */
-	if (unlikely(nvmeq->cq_vector < 0 && !nvmeq->polled))
+	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
 		return BLK_STS_IOERR;
 
 	ret = nvme_setup_cmd(ns, req, &cmnd);

@@ -1395,31 +1396,19 @@ static void nvme_free_queues(struct nvme_dev *dev, int lowest)
  */
 static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 {
-	int vector;
-
-	spin_lock_irq(&nvmeq->cq_lock);
-	if (nvmeq->cq_vector == -1 && !nvmeq->polled) {
-		spin_unlock_irq(&nvmeq->cq_lock);
+	if (!test_and_clear_bit(NVMEQ_ENABLED, &nvmeq->flags))
 		return 1;
-	}
-	vector = nvmeq->cq_vector;
-	nvmeq->dev->online_queues--;
-	nvmeq->cq_vector = -1;
-	nvmeq->polled = false;
-	spin_unlock_irq(&nvmeq->cq_lock);
 
-	/*
-	 * Ensure that nvme_queue_rq() sees it ->cq_vector == -1 without
-	 * having to grab the lock.
-	 */
+	/* ensure that nvme_queue_rq() sees NVMEQ_ENABLED cleared */
 	mb();
 
+	nvmeq->dev->online_queues--;
 	if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q)
 		blk_mq_quiesce_queue(nvmeq->dev->ctrl.admin_q);
-
-	if (vector != -1)
-		pci_free_irq(to_pci_dev(nvmeq->dev->dev), vector, nvmeq);
-
+	if (nvmeq->cq_vector == -1)
+		return 0;
+	pci_free_irq(to_pci_dev(nvmeq->dev->dev), nvmeq->cq_vector, nvmeq);
+	nvmeq->cq_vector = -1;
 	return 0;
 }
 

@@ -1578,13 +1567,7 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid, bool polled)
 	else if (result)
 		goto release_cq;
 
-	/*
-	 * Set cq_vector after alloc cq/sq, otherwise nvme_suspend_queue will
-	 * invoke free_irq for it and cause a 'Trying to free already-free IRQ
-	 * xxx' warning if the create CQ/SQ command times out.
-	 */
 	nvmeq->cq_vector = vector;
-	nvmeq->polled = polled;
 	nvme_init_queue(nvmeq, qid);
 
 	if (vector != -1) {

@@ -1593,11 +1576,11 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid, bool polled)
 			goto release_sq;
 	}
 
+	set_bit(NVMEQ_ENABLED, &nvmeq->flags);
 	return result;
 
 release_sq:
 	nvmeq->cq_vector = -1;
-	nvmeq->polled = false;
 	dev->online_queues--;
 	adapter_delete_sq(dev, qid);
 release_cq:

@@ -1751,6 +1734,7 @@ static int nvme_pci_configure_admin_queue(struct nvme_dev *dev)
 		return result;
 	}
 
+	set_bit(NVMEQ_ENABLED, &nvmeq->flags);
 	return result;
 }
 

@@ -2173,6 +2157,8 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 
 	if (nr_io_queues == 0)
 		return 0;
+	
+	clear_bit(NVMEQ_ENABLED, &adminq->flags);
 
 	if (dev->cmb_use_sqes) {
 		result = nvme_cmb_qdepth(dev, nr_io_queues,

@@ -2227,6 +2213,7 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 		adminq->cq_vector = -1;
 		return result;
 	}
+	set_bit(NVMEQ_ENABLED, &adminq->flags);
 	return nvme_create_io_queues(dev);
 }
Signature · 1 line
2.19.1
maintainer
#6 lore raw

Re: [PATCH 02/13] nvme-pci: use atomic bitops to mark a queue enabled

drivers/nvme/host/pci.c static int nvme_setup_io_queues(struct nvme_dev *dev) · line 2162

Quoted 8 lines
drivers/nvme/host/pci.c +2 -0
@@ -2173,6 +2157,8 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 
  	if (nr_io_queues == 0)
  		return 0;
+	
+	clear_bit(NVMEQ_ENABLED, &adminq->flags);

This is a change of behavior, looks correct though as we can fail nvme_setup_irqs after we freed the admin vector. Needs documentation though..

author maintainer
#7 lore raw

Re: [PATCH 02/13] nvme-pci: use atomic bitops to mark a queue enabled

Quoted 11 lines · Sagi Grimberg

drivers/nvme/host/pci.c static int nvme_setup_io_queues(struct nvme_dev *dev) · line 2161

Quoted 7 lines
drivers/nvme/host/pci.c +2 -0
@@ -2173,6 +2157,8 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
    	if (nr_io_queues == 0)
  		return 0;
+	
+	clear_bit(NVMEQ_ENABLED, &adminq->flags);

This is a change of behavior, looks correct though as we can fail nvme_setup_irqs after we freed the admin vector. Needs documentation though..

I have a hard time parsing the above, can you point to where the problem is?

maintainer
#8 lore raw

Re: [PATCH 02/13] nvme-pci: use atomic bitops to mark a queue enabled

Quoted 12 lines
Quoted 11 lines

drivers/nvme/host/pci.c static int nvme_setup_io_queues(struct nvme_dev *dev) · line 2161

Quoted 7 lines
drivers/nvme/host/pci.c +2 -0
@@ -2173,6 +2157,8 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
     	if (nr_io_queues == 0)
   		return 0;
+	
+	clear_bit(NVMEQ_ENABLED, &adminq->flags);

This is a change of behavior, looks correct though as we can fail nvme_setup_irqs after we freed the admin vector. Needs documentation though..

Quoted 2 lines

I have a hard time parsing the above, can you point to where the problem is?

This flag replaces cq_vector = -1 for indicating the queue state so I'd expect that it would not appear where the former didn't. In this case we clear the flag in a place that before we did not set the cq_vector before, which seems correct to me though.

author maintainer
#9 lore raw

PATCH 3/13 nvme-pci: cleanup SQ allocation a bit

In reply to Christoph Hellwig

Use a bit flag to mark if the SQ was allocated from the CMB, and clean up the surrounding code a bit.

Signed-off-by Christoph Hellwig <hch at lst.de>
Reviewed-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)
drivers/nvme/host/pci.c +15 -18
index 022395a319f4..b820dd0351cb 100644
@@ -186,7 +186,6 @@ struct nvme_queue {
 	struct nvme_dev *dev;
 	spinlock_t sq_lock;
 	struct nvme_command *sq_cmds;
-	bool sq_cmds_is_io;
 	spinlock_t cq_lock ____cacheline_aligned_in_smp;
 	volatile struct nvme_completion *cqes;
 	struct blk_mq_tags **tags;

@@ -203,6 +202,7 @@ struct nvme_queue {
 	u8 cq_phase;
 	unsigned long flags;
 #define NVMEQ_ENABLED		0
+#define NVMEQ_SQ_CMB		1
 	u32 *dbbuf_sq_db;
 	u32 *dbbuf_cq_db;
 	u32 *dbbuf_sq_ei;

@@ -1366,17 +1366,15 @@ static void nvme_free_queue(struct nvme_queue *nvmeq)
 {
 	dma_free_coherent(nvmeq->q_dmadev, CQ_SIZE(nvmeq->q_depth),
 				(void *)nvmeq->cqes, nvmeq->cq_dma_addr);
+	if (!nvmeq->sq_cmds)
+		return;
 
-	if (nvmeq->sq_cmds) {
-		if (nvmeq->sq_cmds_is_io)
-			pci_free_p2pmem(to_pci_dev(nvmeq->q_dmadev),
-					nvmeq->sq_cmds,
-					SQ_SIZE(nvmeq->q_depth));
-		else
-			dma_free_coherent(nvmeq->q_dmadev,
-					  SQ_SIZE(nvmeq->q_depth),
-					  nvmeq->sq_cmds,
-					  nvmeq->sq_dma_addr);
+	if (test_and_clear_bit(NVMEQ_SQ_CMB, &nvmeq->flags)) {
+		pci_free_p2pmem(to_pci_dev(nvmeq->q_dmadev),
+				nvmeq->sq_cmds, SQ_SIZE(nvmeq->q_depth));
+	} else {
+		dma_free_coherent(nvmeq->q_dmadev, SQ_SIZE(nvmeq->q_depth),
+				nvmeq->sq_cmds, nvmeq->sq_dma_addr);
 	}
 }
 

@@ -1462,15 +1460,14 @@ static int nvme_alloc_sq_cmds(struct nvme_dev *dev, struct nvme_queue *nvmeq,
 		nvmeq->sq_cmds = pci_alloc_p2pmem(pdev, SQ_SIZE(depth));
 		nvmeq->sq_dma_addr = pci_p2pmem_virt_to_bus(pdev,
 						nvmeq->sq_cmds);
-		nvmeq->sq_cmds_is_io = true;
-	}
-
-	if (!nvmeq->sq_cmds) {
-		nvmeq->sq_cmds = dma_alloc_coherent(dev->dev, SQ_SIZE(depth),
-					&nvmeq->sq_dma_addr, GFP_KERNEL);
-		nvmeq->sq_cmds_is_io = false;
+		if (nvmeq->sq_dma_addr) {
+			set_bit(NVMEQ_SQ_CMB, &nvmeq->flags);
+			return 0; 
+		}
 	}
 
+	nvmeq->sq_cmds = dma_alloc_coherent(dev->dev, SQ_SIZE(depth),
+				&nvmeq->sq_dma_addr, GFP_KERNEL);
 	if (!nvmeq->sq_cmds)
 		return -ENOMEM;
 	return 0;
Signature · 1 line
2.19.1
maintainer Reviewed-by on v1 3/13
#10 lore raw

Re: [PATCH 03/13] nvme-pci: cleanup SQ allocation a bit

Looks good,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
author maintainer
#11 lore raw

PATCH 4/13 nvme-pci: only allow polling with separate poll queues

In reply to Christoph Hellwig

This will allow us to simplify both the regular NVMe interrupt handler and the upcoming aio poll code. In addition to that the separate queues are generally a good idea for performance reasons.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)
drivers/nvme/host/pci.c +5 -13
index b820dd0351cb..d42bb76e5e78 100644
@@ -1089,13 +1089,6 @@ static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag)
 }
 
 static int nvme_poll(struct blk_mq_hw_ctx *hctx)
-{
-	struct nvme_queue *nvmeq = hctx->driver_data;
-
-	return __nvme_poll(nvmeq, -1);
-}
-
-static int nvme_poll_noirq(struct blk_mq_hw_ctx *hctx)
 {
 	struct nvme_queue *nvmeq = hctx->driver_data;
 	u16 start, end;

@@ -1605,12 +1598,11 @@ static const struct blk_mq_ops nvme_mq_admin_ops = {
 
 static const struct blk_mq_ops nvme_mq_ops = {
 	NVME_SHARED_MQ_OPS,
-	.poll			= nvme_poll,
 };
 
-static const struct blk_mq_ops nvme_mq_poll_noirq_ops = {
+static const struct blk_mq_ops nvme_mq_poll_ops = {
 	NVME_SHARED_MQ_OPS,
-	.poll			= nvme_poll_noirq,
+	.poll			= nvme_poll,
 };
 
 static void nvme_dev_remove_admin(struct nvme_dev *dev)

@@ -2298,10 +2290,10 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	int ret;
 
 	if (!dev->ctrl.tagset) {
-		if (!dev->io_queues[HCTX_TYPE_POLL])
-			dev->tagset.ops = &nvme_mq_ops;
+		if (dev->io_queues[HCTX_TYPE_POLL])
+			dev->tagset.ops = &nvme_mq_poll_ops;
 		else
-			dev->tagset.ops = &nvme_mq_poll_noirq_ops;
+			dev->tagset.ops = &nvme_mq_ops;
 
 		dev->tagset.nr_hw_queues = dev->online_queues - 1;
 		dev->tagset.nr_maps = HCTX_MAX_TYPES;
Signature · 1 line
2.19.1
Reviewed-by on v1 4/13
#12 lore raw

Re: [PATCH 04/13] nvme-pci: only allow polling with separate poll queues

Quoted 5 lines · Christoph Hellwig

This will allow us to simplify both the regular NVMe interrupt handler and the upcoming aio poll code. In addition to that the separate queues are generally a good idea for performance reasons.

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

Looks good.

Reviewed-by Keith Busch <keith.busch at intel.com>
author maintainer
#14 lore raw

PATCH 5/13 nvme-pci: consolidate code for polling non-dedicated queues

In reply to Christoph Hellwig

We have three places that can poll for I/O completions on a normal interrupt-enabled queue. All of them are in slow path code, so consolidate them to a single helper that uses spin_lock_irqsave and removes the fast path cqe_pending check.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)
drivers/nvme/host/pci.c +12 -23
index d42bb76e5e78..10c26a2e355a 100644
@@ -1072,17 +1072,19 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
 	return IRQ_NONE;
 }
 
-static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag)
+/*
+ * Poll for completions any queue, including those not dedicated to polling.
+ * Can be called from any context.
+ */
+static int nvme_poll_irqdisable(struct nvme_queue *nvmeq, unsigned int tag)
 {
+	unsigned long flags;
 	u16 start, end;
 	int found;
 
-	if (!nvme_cqe_pending(nvmeq))
-		return 0;
-
-	spin_lock_irq(&nvmeq->cq_lock);
+	spin_lock_irqsave(&nvmeq->cq_lock, flags);
 	found = nvme_process_cq(nvmeq, &start, &end, tag);
-	spin_unlock_irq(&nvmeq->cq_lock);
+	spin_unlock_irqrestore(&nvmeq->cq_lock, flags);
 
 	nvme_complete_cqes(nvmeq, start, end);
 	return found;

@@ -1279,7 +1281,7 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 	/*
 	 * Did we miss an interrupt?
 	 */
-	if (__nvme_poll(nvmeq, req->tag)) {
+	if (nvme_poll_irqdisable(nvmeq, req->tag)) {
 		dev_warn(dev->ctrl.device,
 			 "I/O %d QID %d timeout, completion polled\n",
 			 req->tag, nvmeq->qid);

@@ -1406,18 +1408,13 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown)
 {
 	struct nvme_queue *nvmeq = &dev->queues[0];
-	u16 start, end;
 
 	if (shutdown)
 		nvme_shutdown_ctrl(&dev->ctrl);
 	else
 		nvme_disable_ctrl(&dev->ctrl, dev->ctrl.cap);
 
-	spin_lock_irq(&nvmeq->cq_lock);
-	nvme_process_cq(nvmeq, &start, &end, -1);
-	spin_unlock_irq(&nvmeq->cq_lock);
-
-	nvme_complete_cqes(nvmeq, start, end);
+	nvme_poll_irqdisable(nvmeq, -1);
 }
 
 static int nvme_cmb_qdepth(struct nvme_dev *dev, int nr_io_queues,

@@ -2217,17 +2214,9 @@ static void nvme_del_queue_end(struct request *req, blk_status_t error)
 static void nvme_del_cq_end(struct request *req, blk_status_t error)
 {
 	struct nvme_queue *nvmeq = req->end_io_data;
-	u16 start, end;
 
-	if (!error) {
-		unsigned long flags;
-
-		spin_lock_irqsave(&nvmeq->cq_lock, flags);
-		nvme_process_cq(nvmeq, &start, &end, -1);
-		spin_unlock_irqrestore(&nvmeq->cq_lock, flags);
-
-		nvme_complete_cqes(nvmeq, start, end);
-	}
+	if (!error)
+		nvme_poll_irqdisable(nvmeq, -1);
 
 	nvme_del_queue_end(req, error);
 }
Signature · 1 line
2.19.1
maintainer Reviewed-by on v1 5/13
#15 lore raw

Re: [PATCH 05/13] nvme-pci: consolidate code for polling non-dedicated queues

Quoted 2 lines

+static int nvme_poll_irqdisable(struct nvme_queue *nvmeq, unsigned int tag)

Do we still need to carry the tag around?

Other than that,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
author maintainer
#16 lore raw

Re: [PATCH 05/13] nvme-pci: consolidate code for polling non-dedicated queues

Quoted 4 lines · Sagi Grimberg
Quoted 2 lines

+static int nvme_poll_irqdisable(struct nvme_queue *nvmeq, unsigned int tag)

Do we still need to carry the tag around?

Yes, the timeout handler polls for a specific tag.

maintainer
#17 lore raw

Re: [PATCH 05/13] nvme-pci: consolidate code for polling non-dedicated queues

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

+static int nvme_poll_irqdisable(struct nvme_queue *nvmeq, unsigned int tag)

Do we still need to carry the tag around?

Yes, the timeout handler polls for a specific tag.

Does it have to? the documentation suggests that we missed an interrupt, so it is probably waiting on the completion queue.

I'd say that it'd be better if the tag search would be implemented on the timeout handler alone so we don't have to pass the tag around for everyone... Thoughts?

#18 lore raw

Re: [PATCH 05/13] nvme-pci: consolidate code for polling non-dedicated queues

Quoted 5 lines · Sagi Grimberg
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines

+static int nvme_poll_irqdisable(struct nvme_queue *nvmeq, unsigned int tag)

Do we still need to carry the tag around?

Quoted 8 lines
Quoted 1 line

Yes, the timeout handler polls for a specific tag.

Does it have to? the documentation suggests that we missed an interrupt, so it is probably waiting on the completion queue.

I'd say that it'd be better if the tag search would be implemented on the timeout handler alone so we don't have to pass the tag around for everyone... Thoughts?

Without that you don't know if that's the request that completed. You have to be able to look that up from the timeout handler.

Signature · 1 line
Jens Axboe
author maintainer
#19 lore raw

PATCH 6/13 nvme-pci: refactor nvme_disable_io_queues

In reply to Christoph Hellwig

Pass the opcode for the delete SQ/CQ command as an argument instead of the somewhat confusing pass loop.

Signed-off-by Christoph Hellwig <hch at lst.de>
Reviewed-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)
drivers/nvme/host/pci.c +20 -21
index 10c26a2e355a..9ceba9900ca3 100644
@@ -2244,31 +2244,29 @@ static int nvme_delete_queue(struct nvme_queue *nvmeq, u8 opcode)
 	return 0;
 }
 
-static void nvme_disable_io_queues(struct nvme_dev *dev)
+static bool nvme_disable_io_queues(struct nvme_dev *dev, u8 opcode)
 {
-	int pass, queues = dev->online_queues - 1;
+	int nr_queues = dev->online_queues - 1, sent = 0;
 	unsigned long timeout;
-	u8 opcode = nvme_admin_delete_sq;
 
-	for (pass = 0; pass < 2; pass++) {
-		int sent = 0, i = queues;
-
-		reinit_completion(&dev->ioq_wait);
+	reinit_completion(&dev->ioq_wait);
  retry:
-		timeout = ADMIN_TIMEOUT;
-		for (; i > 0; i--, sent++)
-			if (nvme_delete_queue(&dev->queues[i], opcode))
-				break;
-
-		while (sent--) {
-			timeout = wait_for_completion_io_timeout(&dev->ioq_wait, timeout);
-			if (timeout == 0)
-				return;
-			if (i)
-				goto retry;
-		}
-		opcode = nvme_admin_delete_cq;
+	timeout = ADMIN_TIMEOUT;
+	while (nr_queues > 0) {
+		if (nvme_delete_queue(&dev->queues[nr_queues], opcode))
+			break;
+		nr_queues--;
+		sent++;
 	}
+	while (sent--) {
+		timeout = wait_for_completion_io_timeout(&dev->ioq_wait,
+				timeout);
+		if (timeout == 0)
+			return false;
+		if (nr_queues)
+			goto retry;
+	}
+	return true;
 }
 
 /*

@@ -2428,7 +2426,8 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	nvme_stop_queues(&dev->ctrl);
 
 	if (!dead && dev->ctrl.queue_count > 0) {
-		nvme_disable_io_queues(dev);
+		if (nvme_disable_io_queues(dev, nvme_admin_delete_sq))
+			nvme_disable_io_queues(dev, nvme_admin_delete_cq);
 		nvme_disable_admin_queue(dev, shutdown);
 	}
 	for (i = dev->ctrl.queue_count - 1; i >= 0; i--)
Signature · 1 line
2.19.1
maintainer
#20 lore raw

Re: [PATCH 06/13] nvme-pci: refactor nvme_disable_io_queues

drivers/nvme/host/pci.c static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown) · line 2430

Quoted 8 lines
drivers/nvme/host/pci.c +2 -1
@@ -2428,7 +2426,8 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
  	nvme_stop_queues(&dev->ctrl);
 
  	if (!dead && dev->ctrl.queue_count > 0) {
-		nvme_disable_io_queues(dev);
+		if (nvme_disable_io_queues(dev, nvme_admin_delete_sq))
+

Would be nice if the opcode change would be kept inside but still split like:

static void nvme_disable_io_queues(struct nvme_dev *dev)

{
	if (__nvme_disable_io_queues(dev, nvme_admin_delete_sq))
		__nvme_disable_io_queues(dev, nvme_admin_delete_cq);
}
author maintainer
#21 lore raw

Re: [PATCH 06/13] nvme-pci: refactor nvme_disable_io_queues

Quoted 10 lines · Sagi Grimberg

drivers/nvme/host/pci.c static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown) · line 2429

Quoted 7 lines
drivers/nvme/host/pci.c +2 -1
@@ -2428,7 +2426,8 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
  	nvme_stop_queues(&dev->ctrl);
    	if (!dead && dev->ctrl.queue_count > 0) {
-		nvme_disable_io_queues(dev);
+		if (nvme_disable_io_queues(dev, nvme_admin_delete_sq))
+

Would be nice if the opcode change would be kept inside but still split like:

I actually like not having another wrapper to stop through..

Keith, Jens, any preference?

#22 lore raw

Re: [PATCH 06/13] nvme-pci: refactor nvme_disable_io_queues

Quoted 12 lines · Christoph Hellwig
Quoted 10 lines · Sagi Grimberg

drivers/nvme/host/pci.c static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown) · line 2429

Quoted 7 lines
drivers/nvme/host/pci.c +2 -1
@@ -2428,7 +2426,8 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
  	nvme_stop_queues(&dev->ctrl);
    	if (!dead && dev->ctrl.queue_count > 0) {
-		nvme_disable_io_queues(dev);
+		if (nvme_disable_io_queues(dev, nvme_admin_delete_sq))
+

Would be nice if the opcode change would be kept inside but still split like:

Quoted 3 lines

I actually like not having another wrapper to stop through..

Keith, Jens, any preference?

Fine either way, prefer not having a wrapper.

Signature · 1 line
Jens Axboe
author maintainer
#23 lore raw

PATCH 7/13 nvme-pci: don't poll from irq context when deleting queues

In reply to Christoph Hellwig

This is the last place outside of nvme_irq that handles CQEs from interrupt context, and thus is in the way of removing the cq_lock for normal queues, and avoiding lockdep warnings on the poll queues, for which we already take it without IRQ disabling.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)
drivers/nvme/host/pci.c +19 -8
index 9ceba9900ca3..2d5a468c35b1 100644
@@ -122,7 +122,6 @@ struct nvme_dev {
 	u32 cmbsz;
 	u32 cmbloc;
 	struct nvme_ctrl ctrl;
-	struct completion ioq_wait;
 
 	mempool_t *iod_mempool;
 

@@ -203,10 +202,12 @@ struct nvme_queue {
 	unsigned long flags;
 #define NVMEQ_ENABLED		0
 #define NVMEQ_SQ_CMB		1
+#define NVMEQ_DELETE_ERROR	2
 	u32 *dbbuf_sq_db;
 	u32 *dbbuf_cq_db;
 	u32 *dbbuf_sq_ei;
 	u32 *dbbuf_cq_ei;
+	struct completion delete_done;
 };
 
 /*

@@ -1535,6 +1536,8 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid, bool polled)
 	int result;
 	s16 vector;
 
+	clear_bit(NVMEQ_DELETE_ERROR, &nvmeq->flags);
+
 	/*
 	 * A queue's vector matches the queue identifier unless the controller
 	 * has only one vector available.

@@ -2208,15 +2211,15 @@ static void nvme_del_queue_end(struct request *req, blk_status_t error)
 	struct nvme_queue *nvmeq = req->end_io_data;
 
 	blk_mq_free_request(req);
-	complete(&nvmeq->dev->ioq_wait);
+	complete(&nvmeq->delete_done);
 }
 
 static void nvme_del_cq_end(struct request *req, blk_status_t error)
 {
 	struct nvme_queue *nvmeq = req->end_io_data;
 
-	if (!error)
-		nvme_poll_irqdisable(nvmeq, -1);
+	if (error)
+		set_bit(NVMEQ_DELETE_ERROR, &nvmeq->flags);
 
 	nvme_del_queue_end(req, error);
 }

@@ -2238,6 +2241,7 @@ static int nvme_delete_queue(struct nvme_queue *nvmeq, u8 opcode)
 	req->timeout = ADMIN_TIMEOUT;
 	req->end_io_data = nvmeq;
 
+	init_completion(&nvmeq->delete_done);
 	blk_execute_rq_nowait(q, NULL, req, false,
 			opcode == nvme_admin_delete_cq ?
 				nvme_del_cq_end : nvme_del_queue_end);

@@ -2249,7 +2253,6 @@ static bool nvme_disable_io_queues(struct nvme_dev *dev, u8 opcode)
 	int nr_queues = dev->online_queues - 1, sent = 0;
 	unsigned long timeout;
 
-	reinit_completion(&dev->ioq_wait);
  retry:
 	timeout = ADMIN_TIMEOUT;
 	while (nr_queues > 0) {

@@ -2258,11 +2261,20 @@ static bool nvme_disable_io_queues(struct nvme_dev *dev, u8 opcode)
 		nr_queues--;
 		sent++;
 	}
-	while (sent--) {
-		timeout = wait_for_completion_io_timeout(&dev->ioq_wait,
+	while (sent) {
+		struct nvme_queue *nvmeq = &dev->queues[nr_queues + sent];
+
+		timeout = wait_for_completion_io_timeout(&nvmeq->delete_done,
 				timeout);
 		if (timeout == 0)
 			return false;
+
+		/* handle any remaining CQEs */
+		if (opcode == nvme_admin_delete_cq &&
+		    !test_bit(NVMEQ_DELETE_ERROR, &nvmeq->flags))
+			nvme_poll_irqdisable(nvmeq, -1);
+
+		sent--;
 		if (nr_queues)
 			goto retry;
 	}

@@ -2746,7 +2758,6 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	INIT_WORK(&dev->ctrl.reset_work, nvme_reset_work);
 	INIT_WORK(&dev->remove_work, nvme_remove_dead_ctrl_work);
 	mutex_init(&dev->shutdown_lock);
-	init_completion(&dev->ioq_wait);
 
 	result = nvme_setup_prp_pools(dev);
 	if (result)
Signature · 1 line
2.19.1
Reviewed-by on v1 7/13
#24 lore raw

Re: [PATCH 07/13] nvme-pci: don't poll from irq context when deleting queues

Quoted 6 lines · Christoph Hellwig

This is the last place outside of nvme_irq that handles CQEs from interrupt context, and thus is in the way of removing the cq_lock for normal queues, and avoiding lockdep warnings on the poll queues, for which we already take it without IRQ disabling.

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

Looks good.

Reviewed-by Keith Busch <keith.busch at intel.com>
author maintainer
#26 lore raw

PATCH 8/13 nvme-pci: remove the CQ lock for interrupt driven queues

In reply to Christoph Hellwig

Now that we can't poll regular, interrupt driven I/O queues there is almost nothing that can race with an interrupt. The only possible other contexts polling a CQ are the error handler and queue shutdown, and both are so far off in the slow path that we can simply use the big hammer of disabling interrupts.

With that we can stop taking the cq_lock for normal queues.

Signed-off-by Christoph Hellwig <hch at lst.de>
Reviewed-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)
drivers/nvme/host/pci.c +26 -11
index 2d5a468c35b1..4ccb4ea22ac6 100644
@@ -185,7 +185,8 @@ struct nvme_queue {
 	struct nvme_dev *dev;
 	spinlock_t sq_lock;
 	struct nvme_command *sq_cmds;
-	spinlock_t cq_lock ____cacheline_aligned_in_smp;
+	 /* only used for poll queues: */
+	spinlock_t cq_poll_lock ____cacheline_aligned_in_smp;
 	volatile struct nvme_completion *cqes;
 	struct blk_mq_tags **tags;
 	dma_addr_t sq_dma_addr;

@@ -1050,12 +1051,16 @@ static irqreturn_t nvme_irq(int irq, void *data)
 	irqreturn_t ret = IRQ_NONE;
 	u16 start, end;
 
-	spin_lock(&nvmeq->cq_lock);
+	/*
+	 * The rmb/wmb pair ensures we see all updates from a previous run of
+	 * the irq handler, even if that was on another CPU.
+	 */
+	rmb();
 	if (nvmeq->cq_head != nvmeq->last_cq_head)
 		ret = IRQ_HANDLED;
 	nvme_process_cq(nvmeq, &start, &end, -1);
 	nvmeq->last_cq_head = nvmeq->cq_head;
-	spin_unlock(&nvmeq->cq_lock);
+	wmb();
 
 	if (start != end) {
 		nvme_complete_cqes(nvmeq, start, end);

@@ -1079,13 +1084,24 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
  */
 static int nvme_poll_irqdisable(struct nvme_queue *nvmeq, unsigned int tag)
 {
-	unsigned long flags;
+	struct pci_dev *pdev = to_pci_dev(nvmeq->dev->dev);
 	u16 start, end;
 	int found;
 
-	spin_lock_irqsave(&nvmeq->cq_lock, flags);
+	/*
+	 * For a poll queue we need to protect against the polling thread
+	 * using the CQ lock.  For normal interrupt driven threads we have
+	 * to disable the interrupt to avoid racing with it.
+	 */
+	if (nvmeq->cq_vector == -1)
+		spin_lock(&nvmeq->cq_poll_lock);
+	else
+		disable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
 	found = nvme_process_cq(nvmeq, &start, &end, tag);
-	spin_unlock_irqrestore(&nvmeq->cq_lock, flags);
+	if (nvmeq->cq_vector == -1)
+		spin_unlock(&nvmeq->cq_poll_lock);
+	else
+		enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
 
 	nvme_complete_cqes(nvmeq, start, end);
 	return found;

@@ -1100,9 +1116,9 @@ static int nvme_poll(struct blk_mq_hw_ctx *hctx)
 	if (!nvme_cqe_pending(nvmeq))
 		return 0;
 
-	spin_lock(&nvmeq->cq_lock);
+	spin_lock(&nvmeq->cq_poll_lock);
 	found = nvme_process_cq(nvmeq, &start, &end, -1);
-	spin_unlock(&nvmeq->cq_lock);
+	spin_unlock(&nvmeq->cq_poll_lock);
 
 	nvme_complete_cqes(nvmeq, start, end);
 	return found;

@@ -1482,7 +1498,7 @@ static int nvme_alloc_queue(struct nvme_dev *dev, int qid, int depth)
 	nvmeq->q_dmadev = dev->dev;
 	nvmeq->dev = dev;
 	spin_lock_init(&nvmeq->sq_lock);
-	spin_lock_init(&nvmeq->cq_lock);
+	spin_lock_init(&nvmeq->cq_poll_lock);
 	nvmeq->cq_head = 0;
 	nvmeq->cq_phase = 1;
 	nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride];

@@ -1518,7 +1534,6 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
 {
 	struct nvme_dev *dev = nvmeq->dev;
 
-	spin_lock_irq(&nvmeq->cq_lock);
 	nvmeq->sq_tail = 0;
 	nvmeq->last_sq_tail = 0;
 	nvmeq->cq_head = 0;

@@ -1527,7 +1542,7 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
 	memset((void *)nvmeq->cqes, 0, CQ_SIZE(nvmeq->q_depth));
 	nvme_dbbuf_init(dev, nvmeq, qid);
 	dev->online_queues++;
-	spin_unlock_irq(&nvmeq->cq_lock);
+	wmb(); /* ensure the first interrupt sees the initialization */
 }
 
 static int nvme_create_queue(struct nvme_queue *nvmeq, int qid, bool polled)
Signature · 1 line
2.19.1
maintainer Reviewed-by on v1 8/13
#27 lore raw

Re: [PATCH 08/13] nvme-pci: remove the CQ lock for interrupt driven queues

Quoted 8 lines

Now that we can't poll regular, interrupt driven I/O queues there is almost nothing that can race with an interrupt. The only possible other contexts polling a CQ are the error handler and queue shutdown, and both are so far off in the slow path that we can simply use the big hammer of disabling interrupts.

With that we can stop taking the cq_lock for normal queues.

Nice,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
author maintainer
#28 lore raw

PATCH 9/13 nvme-rdma: remove I/O polling support

In reply to Christoph Hellwig

The code was always a bit of a hack that digs far too much into RDMA core internals. Lets kick it out and reimplement proper dedicated poll queues as needed.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/rdma.c | 24 ------------------------
 1 file changed, 24 deletions(-)
drivers/nvme/host/rdma.c +0 -24
index ccfde6c7c0a5..a62e9f177c06 100644
@@ -1736,29 +1736,6 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 	return BLK_STS_IOERR;
 }
 
-static int nvme_rdma_poll(struct blk_mq_hw_ctx *hctx)
-{
-	struct nvme_rdma_queue *queue = hctx->driver_data;
-	struct ib_cq *cq = queue->ib_cq;
-	struct ib_wc wc;
-	int found = 0;
-
-	while (ib_poll_cq(cq, 1, &wc) > 0) {
-		struct ib_cqe *cqe = wc.wr_cqe;
-
-		if (cqe) {
-			if (cqe->done == nvme_rdma_recv_done) {
-				nvme_rdma_recv_done(cq, &wc);
-				found++;
-			} else {
-				cqe->done(cq, &wc);
-			}
-		}
-	}
-
-	return found;
-}
-
 static void nvme_rdma_complete_rq(struct request *rq)
 {
 	struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq);

@@ -1780,7 +1757,6 @@ static const struct blk_mq_ops nvme_rdma_mq_ops = {
 	.init_request	= nvme_rdma_init_request,
 	.exit_request	= nvme_rdma_exit_request,
 	.init_hctx	= nvme_rdma_init_hctx,
-	.poll		= nvme_rdma_poll,
 	.timeout	= nvme_rdma_timeout,
 	.map_queues	= nvme_rdma_map_queues,
 };
Signature · 1 line
2.19.1
author maintainer
#29 lore raw

PATCH 10/13 nvme-mpath: remove I/O polling support

In reply to Christoph Hellwig

The ->poll_fn has been stale for a while, as a lot of places check for mq ops. But there is no real point in it anyway, as we don't even use the multipath code for subsystems without multiple ports, which is usually what we do high performance I/O to. If it really becomes an issue we should rework the nvme code to also skip the multipath code for any private namespace, even if that could mean some trouble when rescanning.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/multipath.c | 16 ----------------
 1 file changed, 16 deletions(-)
drivers/nvme/host/multipath.c +0 -16
index ffebdd0ae34b..ec310b1b9267 100644
@@ -220,21 +220,6 @@ static blk_qc_t nvme_ns_head_make_request(struct request_queue *q,
 	return ret;
 }
 
-static int nvme_ns_head_poll(struct request_queue *q, blk_qc_t qc, bool spin)
-{
-	struct nvme_ns_head *head = q->queuedata;
-	struct nvme_ns *ns;
-	int found = 0;
-	int srcu_idx;
-
-	srcu_idx = srcu_read_lock(&head->srcu);
-	ns = srcu_dereference(head->current_path[numa_node_id()], &head->srcu);
-	if (likely(ns && nvme_path_is_optimized(ns)))
-		found = ns->queue->poll_fn(q, qc, spin);
-	srcu_read_unlock(&head->srcu, srcu_idx);
-	return found;
-}
-
 static void nvme_requeue_work(struct work_struct *work)
 {
 	struct nvme_ns_head *head =

@@ -281,7 +266,6 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
 		goto out;
 	q->queuedata = head;
 	blk_queue_make_request(q, nvme_ns_head_make_request);
-	q->poll_fn = nvme_ns_head_poll;
 	blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
 	/* set to a default value for 512 until disk is validated */
 	blk_queue_logical_block_size(q, 512);
Signature · 1 line
2.19.1
Reviewed-by on v1 10/13
#30 lore raw

Re: [PATCH 10/13] nvme-mpath: remove I/O polling support

Quoted 8 lines · Christoph Hellwig

The ->poll_fn has been stale for a while, as a lot of places check for mq ops. But there is no real point in it anyway, as we don't even use the multipath code for subsystems without multiple ports, which is usually what we do high performance I/O to. If it really becomes an issue we should rework the nvme code to also skip the multipath code for any private namespace, even if that could mean some trouble when rescanning.

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

This was a bit flawed anyway since the head's current path could change, and you end up polling the wrong request_queue. Not really harmful other than some wasted CPU cycles, but might be worth thinking about if we want to bring mpath polling back.

Reviewed-by Keith Busch <keith.busch at intel.com>
maintainer Reviewed-by on v1 10/13
#31 lore raw

Re: [PATCH 10/13] nvme-mpath: remove I/O polling support

In reply to Christoph Hellwig
Quoted 4 lines

If it really becomes an issue we
should rework the nvme code to also skip the multipath code for any private namespace, even if that could mean some trouble when rescanning.

This requires some explanation? skip the multipath code how?

Other than that,
Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

author maintainer
#32 lore raw

Re: [PATCH 10/13] nvme-mpath: remove I/O polling support

Quoted 6 lines · Sagi Grimberg
Quoted 4 lines

If it really becomes an issue we
should rework the nvme code to also skip the multipath code for any private namespace, even if that could mean some trouble when rescanning.

This requires some explanation? skip the multipath code how?

We currently always go through the multipath node as long the the controller is multipath capable. If we care about e.g. polling on a private namespace on a dual ported U.2 drive we'd have to make sure we don't go through the multipath device node for private namespaces that can only have one path, but only for shared namespaces.

maintainer
#33 lore raw

Re: [PATCH 10/13] nvme-mpath: remove I/O polling support

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines

If it really becomes an issue we
should rework the nvme code to also skip the multipath code for any private namespace, even if that could mean some trouble when rescanning.

Quoted 7 lines
Quoted 1 line

This requires some explanation? skip the multipath code how?

We currently always go through the multipath node as long the the controller is multipath capable. If we care about e.g. polling on a private namespace on a dual ported U.2 drive we'd have to make sure we don't go through the multipath device node for private namespaces that can only have one path, but only for shared namespaces.

But we'd still use the multipath node for shared namespaces (and also polling if needed). I agree that private namespaces can skip the multipath node.

author maintainer
#34 lore raw

PATCH 11/13 block: remove ->poll_fn

In reply to Christoph Hellwig

This was intended to support users like nvme multipath, but is just getting in the way and adding another indirect call.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 block/blk-core.c       | 23 -----------------------
 block/blk-mq.c         | 24 +++++++++++++++++++-----
 include/linux/blkdev.h |  2 --
 3 files changed, 19 insertions(+), 30 deletions(-)
block/blk-core.c +0 -23
index 3f6f5e6c2fe4..942276399085 100644
@@ -1249,29 +1249,6 @@ blk_qc_t submit_bio(struct bio *bio)
 }
 EXPORT_SYMBOL(submit_bio);
 
-/**
- * blk_poll - poll for IO completions
- * @q:  the queue
- * @cookie: cookie passed back at IO submission time
- * @spin: whether to spin for completions
- *
- * Description:
- *    Poll for completions on the passed in queue. Returns number of
- *    completed entries found. If @spin is true, then blk_poll will continue
- *    looping until at least one completion is found, unless the task is
- *    otherwise marked running (or we need to reschedule).
- */
-int blk_poll(struct request_queue *q, blk_qc_t cookie, bool spin)
-{
-	if (!q->poll_fn || !blk_qc_t_valid(cookie))
-		return 0;
-
-	if (current->plug)
-		blk_flush_plug_list(current->plug, false);
-	return q->poll_fn(q, cookie, spin);
-}
-EXPORT_SYMBOL_GPL(blk_poll);
-
 /**
  * blk_cloned_rq_check_limits - Helper function to check a cloned request
  *                              for new the queue limits
block/blk-mq.c +19 -5
index 7dcef565dc0f..9c90c5038d07 100644
@@ -38,7 +38,6 @@
 #include "blk-mq-sched.h"
 #include "blk-rq-qos.h"
 
-static int blk_mq_poll(struct request_queue *q, blk_qc_t cookie, bool spin);
 static void blk_mq_poll_stats_start(struct request_queue *q);
 static void blk_mq_poll_stats_fn(struct blk_stat_callback *cb);
 

@@ -2823,8 +2822,6 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 	spin_lock_init(&q->requeue_lock);
 
 	blk_queue_make_request(q, blk_mq_make_request);
-	if (q->mq_ops->poll)
-		q->poll_fn = blk_mq_poll;
 
 	/*
 	 * Do this after blk_queue_make_request() overrides it...

@@ -3385,14 +3382,30 @@ static bool blk_mq_poll_hybrid(struct request_queue *q,
 	return blk_mq_poll_hybrid_sleep(q, hctx, rq);
 }
 
-static int blk_mq_poll(struct request_queue *q, blk_qc_t cookie, bool spin)
+/**
+ * blk_poll - poll for IO completions
+ * @q:  the queue
+ * @cookie: cookie passed back at IO submission time
+ * @spin: whether to spin for completions
+ *
+ * Description:
+ *    Poll for completions on the passed in queue. Returns number of
+ *    completed entries found. If @spin is true, then blk_poll will continue
+ *    looping until at least one completion is found, unless the task is
+ *    otherwise marked running (or we need to reschedule).
+ */
+int blk_poll(struct request_queue *q, blk_qc_t cookie, bool spin)
 {
 	struct blk_mq_hw_ctx *hctx;
 	long state;
 
-	if (!test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
+	if (!blk_qc_t_valid(cookie) ||
+	    !test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
 		return 0;
 
+	if (current->plug)
+		blk_flush_plug_list(current->plug, false);
+
 	hctx = q->queue_hw_ctx[blk_qc_t_to_queue_num(cookie)];
 
 	/*

@@ -3433,6 +3446,7 @@ static int blk_mq_poll(struct request_queue *q, blk_qc_t cookie, bool spin)
 	__set_current_state(TASK_RUNNING);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(blk_poll);
 
 unsigned int blk_mq_rq_cpu(struct request *rq)
 {
include/linux/blkdev.h +0 -2
index 08d940f85fa0..0b3874bdbc6a 100644
@@ -283,7 +283,6 @@ static inline unsigned short req_get_ioprio(struct request *req)
 struct blk_queue_ctx;
 
 typedef blk_qc_t (make_request_fn) (struct request_queue *q, struct bio *bio);
-typedef int (poll_q_fn) (struct request_queue *q, blk_qc_t, bool spin);
 
 struct bio_vec;
 typedef int (dma_drain_needed_fn)(struct request *);

@@ -401,7 +400,6 @@ struct request_queue {
 	struct rq_qos		*rq_qos;
 
 	make_request_fn		*make_request_fn;
-	poll_q_fn		*poll_fn;
 	dma_drain_needed_fn	*dma_drain_needed;
 
 	const struct blk_mq_ops	*mq_ops;
Signature · 1 line
2.19.1
maintainer Reviewed-by on v1 11/13
#35 lore raw

Re: [PATCH 11/13] block: remove ->poll_fn

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
author maintainer
#36 lore raw

PATCH 12/13 block: only allow polling if a poll queue_map exists

In reply to Christoph Hellwig

This avoids having to have differnet mq_ops for different setups with or without poll queues.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 block/blk-sysfs.c       |  2 +-
 drivers/nvme/host/pci.c | 29 +++++++++--------------------
 2 files changed, 10 insertions(+), 21 deletions(-)
block/blk-sysfs.c +1 -1
index 9f0cb370b39b..bb7c642ffefa 100644
@@ -402,7 +402,7 @@ static ssize_t queue_poll_store(struct request_queue *q, const char *page,
 	unsigned long poll_on;
 	ssize_t ret;
 
-	if (!q->mq_ops || !q->mq_ops->poll)
+	if (!q->tag_set || q->tag_set->nr_maps <= HCTX_TYPE_POLL)
 		return -EINVAL;
 
 	ret = queue_var_store(&poll_on, page, count);
drivers/nvme/host/pci.c +9 -20
index 4ccb4ea22ac6..7732c4979a4e 100644
@@ -1602,22 +1602,15 @@ static const struct blk_mq_ops nvme_mq_admin_ops = {
 	.timeout	= nvme_timeout,
 };
 
-#define NVME_SHARED_MQ_OPS					\
-	.queue_rq		= nvme_queue_rq,		\
-	.commit_rqs		= nvme_commit_rqs,		\
-	.complete		= nvme_pci_complete_rq,		\
-	.init_hctx		= nvme_init_hctx,		\
-	.init_request		= nvme_init_request,		\
-	.map_queues		= nvme_pci_map_queues,		\
-	.timeout		= nvme_timeout			\
-
 static const struct blk_mq_ops nvme_mq_ops = {
-	NVME_SHARED_MQ_OPS,
-};
-
-static const struct blk_mq_ops nvme_mq_poll_ops = {
-	NVME_SHARED_MQ_OPS,
-	.poll			= nvme_poll,
+	.queue_rq	= nvme_queue_rq,
+	.complete	= nvme_pci_complete_rq,
+	.commit_rqs	= nvme_commit_rqs,
+	.init_hctx	= nvme_init_hctx,
+	.init_request	= nvme_init_request,
+	.map_queues	= nvme_pci_map_queues,
+	.timeout	= nvme_timeout,
+	.poll		= nvme_poll,
 };
 
 static void nvme_dev_remove_admin(struct nvme_dev *dev)

@@ -2304,11 +2297,7 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	int ret;
 
 	if (!dev->ctrl.tagset) {
-		if (dev->io_queues[HCTX_TYPE_POLL])
-			dev->tagset.ops = &nvme_mq_poll_ops;
-		else
-			dev->tagset.ops = &nvme_mq_ops;
-
+		dev->tagset.ops = &nvme_mq_ops;
 		dev->tagset.nr_hw_queues = dev->online_queues - 1;
 		dev->tagset.nr_maps = HCTX_MAX_TYPES;
 		dev->tagset.timeout = NVME_IO_TIMEOUT;
Signature · 1 line
2.19.1
maintainer Reviewed-by on v1 12/13
#37 lore raw

Re: [PATCH 12/13] block: only allow polling if a poll queue_map exists

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
author maintainer
#38 lore raw

PATCH 13/13 block: enable polling by default if a poll map is initalized

In reply to Christoph Hellwig

If the user did setup polling in the driver we should not require another know in the block layer to enable it.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 block/blk-mq.c | 2 ++
 1 file changed, 2 insertions(+)
block/blk-mq.c +2 -0
index 9c90c5038d07..a550a00ac00c 100644
@@ -2811,6 +2811,8 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 	q->tag_set = set;
 
 	q->queue_flags |= QUEUE_FLAG_MQ_DEFAULT;
+	if (set->nr_maps > HCTX_TYPE_POLL)
+		blk_queue_flag_set(QUEUE_FLAG_POLL, q);
 
 	if (!(set->flags & BLK_MQ_F_SG_MERGE))
 		blk_queue_flag_set(QUEUE_FLAG_NO_SG_MERGE, q);
Signature · 1 line
2.19.1
maintainer Reviewed-by on v1 13/13
#39 lore raw

Re: [PATCH 13/13] block: enable polling by default if a poll map is initalized

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
maintainer Applied
#40 lore raw
In reply to Christoph Hellwig
Quoted 25 lines · Christoph Hellwig

Hi all,

this series optimizes a few bits in the block layer and nvme code related to polling.

It starts by moving the queue types recently introduce entirely into the block layer instead of requiring an indirect call for them.

It then switches nvme and the block layer to only allow polling with separate poll queues, which allows us to realize the following benefits:

- poll queues can safely avoid disabling irqs on any locks (we already do that in NVMe, but it isn't 100% kosher as-is)
- regular interrupt driven queues can drop the CQ lock entirely, as we won't race for completing CQs

Then we drop the NVMe RDMA code, as it doesn't follow the new mode, and remove the nvme multipath polling code including the block hooks for it, which didn't make much sense to start with given that we started bypassing the multipath code for single controller subsystems early on. Last but not least we enable polling in the block layer by default if the underlying driver has poll queues, as that already requires explicit user action.

Quoted 6 lines

Note that it would be really nice to have polling back for RDMA with dedicated poll queues, but that might take a while. Also based on Jens' polling aio patches we could now implement a model in nvmet where we have a thread polling both the backend nvme device and the RDMA CQs, which might give us some pretty nice performace (I know Sagi looked into something similar a while ago).

Applied, thanks.

Signature · 1 line
Jens Axboe