[PATCH 1/8] nvme/pci: Use a flag for polled queues

[PATCH 1/8] nvme/pci: Use a flag for polled queues

[Keith Busch]

A negative value for the cq_vector used to mean the queue is either
disabled or a polled queue. However, we have a queue enabled flag,
so the cq_vector had been serving double duty.

Don't overload the meaning of cq_vector. Use a flag specific to the
polled queues instead.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 398c6333cf77..d12b2861bdcc 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -189,7 +189,7 @@ struct nvme_queue {
 	dma_addr_t cq_dma_addr;
 	u32 __iomem *q_db;
 	u16 q_depth;
-	s16 cq_vector;
+	u16 cq_vector;
 	u16 sq_tail;
 	u16 last_sq_tail;
 	u16 cq_head;
@@ -200,6 +200,7 @@ struct nvme_queue {
 #define NVMEQ_ENABLED		0
 #define NVMEQ_SQ_CMB		1
 #define NVMEQ_DELETE_ERROR	2
+#define NVMEQ_POLLED		3
 	u32 *dbbuf_sq_db;
 	u32 *dbbuf_cq_db;
 	u32 *dbbuf_sq_ei;
@@ -1141,7 +1142,7 @@ static int adapter_alloc_cq(struct nvme_dev *dev, u16 qid,
 	struct nvme_command c;
 	int flags = NVME_QUEUE_PHYS_CONTIG;
 
-	if (vector != -1)
+	if (!test_bit(NVMEQ_POLLED, &nvmeq->flags))
 		flags |= NVME_CQ_IRQ_ENABLED;
 
 	/*
@@ -1154,10 +1155,7 @@ static int adapter_alloc_cq(struct nvme_dev *dev, u16 qid,
 	c.create_cq.cqid = cpu_to_le16(qid);
 	c.create_cq.qsize = cpu_to_le16(nvmeq->q_depth - 1);
 	c.create_cq.cq_flags = cpu_to_le16(flags);
-	if (vector != -1)
-		c.create_cq.irq_vector = cpu_to_le16(vector);
-	else
-		c.create_cq.irq_vector = 0;
+	c.create_cq.irq_vector = cpu_to_le16(vector);
 
 	return nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0);
 }
@@ -1399,10 +1397,8 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 	nvmeq->dev->online_queues--;
 	if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q)
 		blk_mq_quiesce_queue(nvmeq->dev->ctrl.admin_q);
-	if (nvmeq->cq_vector == -1)
-		return 0;
-	pci_free_irq(to_pci_dev(nvmeq->dev->dev), nvmeq->cq_vector, nvmeq);
-	nvmeq->cq_vector = -1;
+	if (!test_and_clear_bit(NVMEQ_POLLED, &nvmeq->flags))
+		pci_free_irq(to_pci_dev(nvmeq->dev->dev), nvmeq->cq_vector, nvmeq);
 	return 0;
 }
 
@@ -1496,7 +1492,6 @@ static int nvme_alloc_queue(struct nvme_dev *dev, int qid, int depth)
 	nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride];
 	nvmeq->q_depth = depth;
 	nvmeq->qid = qid;
-	nvmeq->cq_vector = -1;
 	dev->ctrl.queue_count++;
 
 	return 0;
@@ -1541,7 +1536,7 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid, bool polled)
 {
 	struct nvme_dev *dev = nvmeq->dev;
 	int result;
-	s16 vector;
+	u16 vector = 0;
 
 	clear_bit(NVMEQ_DELETE_ERROR, &nvmeq->flags);
 
@@ -1552,7 +1547,7 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid, bool polled)
 	if (!polled)
 		vector = dev->num_vecs == 1 ? 0 : qid;
 	else
-		vector = -1;
+		set_bit(NVMEQ_POLLED, &nvmeq->flags);
 
 	result = adapter_alloc_cq(dev, qid, nvmeq, vector);
 	if (result)
@@ -1567,7 +1562,8 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid, bool polled)
 	nvmeq->cq_vector = vector;
 	nvme_init_queue(nvmeq, qid);
 
-	if (vector != -1) {
+	if (!polled) {
+		nvmeq->cq_vector = vector;
 		result = queue_request_irq(nvmeq);
 		if (result < 0)
 			goto release_sq;
@@ -1577,7 +1573,6 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid, bool polled)
 	return result;
 
 release_sq:
-	nvmeq->cq_vector = -1;
 	dev->online_queues--;
 	adapter_delete_sq(dev, qid);
 release_cq:
@@ -1733,7 +1728,7 @@ static int nvme_pci_configure_admin_queue(struct nvme_dev *dev)
 	nvme_init_queue(nvmeq, 0);
 	result = queue_request_irq(nvmeq);
 	if (result) {
-		nvmeq->cq_vector = -1;
+		dev->online_queues--;
 		return result;
 	}
 
@@ -2214,10 +2209,8 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	 * number of interrupts.
 	 */
 	result = queue_request_irq(adminq);
-	if (result) {
-		adminq->cq_vector = -1;
+	if (result)
 		return result;
-	}
 	set_bit(NVMEQ_ENABLED, &adminq->flags);
 
 	result = nvme_create_io_queues(dev);
-- 
2.14.4

[PATCH 2/8] nvme/pci: Don't poll polled queues in timeout

[Keith Busch]

Polled queues are already polled. If a timeout occurs on such a queue,
escalate the error recovery immediately.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d12b2861bdcc..e72e34b0b441 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1077,20 +1077,9 @@ static int nvme_poll_irqdisable(struct nvme_queue *nvmeq, unsigned int tag)
 	u16 start, end;
 	int found;
 
-	/*
-	 * 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);
-		found = nvme_process_cq(nvmeq, &start, &end, tag);
-		spin_unlock(&nvmeq->cq_poll_lock);
-	} else {
-		disable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
-		found = nvme_process_cq(nvmeq, &start, &end, tag);
-		enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
-	}
+	disable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
+	found = nvme_process_cq(nvmeq, &start, &end, tag);
+	enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
 
 	nvme_complete_cqes(nvmeq, start, end);
 	return found;
@@ -1284,7 +1273,8 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 	/*
 	 * Did we miss an interrupt?
 	 */
-	if (nvme_poll_irqdisable(nvmeq, req->tag)) {
+	if (!test_bit(NVMEQ_POLLED, &nvmeq->flags) &&
+	    nvme_poll_irqdisable(nvmeq, req->tag)) {
 		dev_warn(dev->ctrl.device,
 			 "I/O %d QID %d timeout, completion polled\n",
 			 req->tag, nvmeq->qid);
-- 
2.14.4

[PATCH 3/8] nvme/pci: Remove tag check in nvme_process_cq

[Keith Busch]

Simplify processing completions by removing the check for the optional
search tag. The timeout handling was the only place using this, but we
can check the request state directly instead of searching the completion
entries. There is a chance the command was not actually polled in this
implementation, but we don't want to escalate error recovery in that
case anyway, and the warning is still informative either way.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e72e34b0b441..45db4a39795e 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1017,14 +1017,13 @@ static inline void nvme_update_cq_head(struct nvme_queue *nvmeq)
 }
 
 static inline int nvme_process_cq(struct nvme_queue *nvmeq, u16 *start,
-				  u16 *end, unsigned int tag)
+				  u16 *end)
 {
 	int found = 0;
 
 	*start = nvmeq->cq_head;
 	while (nvme_cqe_pending(nvmeq)) {
-		if (tag == -1U || nvmeq->cqes[nvmeq->cq_head].command_id == tag)
-			found++;
+		found++;
 		nvme_update_cq_head(nvmeq);
 	}
 	*end = nvmeq->cq_head;
@@ -1047,7 +1046,7 @@ static irqreturn_t nvme_irq(int irq, void *data)
 	rmb();
 	if (nvmeq->cq_head != nvmeq->last_cq_head)
 		ret = IRQ_HANDLED;
-	nvme_process_cq(nvmeq, &start, &end, -1);
+	nvme_process_cq(nvmeq, &start, &end);
 	nvmeq->last_cq_head = nvmeq->cq_head;
 	wmb();
 
@@ -1071,18 +1070,16 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
  * 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)
+static void nvme_poll_irqdisable(struct nvme_queue *nvmeq)
 {
 	struct pci_dev *pdev = to_pci_dev(nvmeq->dev->dev);
 	u16 start, end;
-	int found;
 
 	disable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
-	found = nvme_process_cq(nvmeq, &start, &end, tag);
+	nvme_process_cq(nvmeq, &start, &end);
 	enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
 
 	nvme_complete_cqes(nvmeq, start, end);
-	return found;
 }
 
 static int nvme_poll(struct blk_mq_hw_ctx *hctx)
@@ -1095,7 +1092,7 @@ static int nvme_poll(struct blk_mq_hw_ctx *hctx)
 		return 0;
 
 	spin_lock(&nvmeq->cq_poll_lock);
-	found = nvme_process_cq(nvmeq, &start, &end, -1);
+	found = nvme_process_cq(nvmeq, &start, &end);
 	spin_unlock(&nvmeq->cq_poll_lock);
 
 	nvme_complete_cqes(nvmeq, start, end);
@@ -1273,12 +1270,14 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 	/*
 	 * Did we miss an interrupt?
 	 */
-	if (!test_bit(NVMEQ_POLLED, &nvmeq->flags) &&
-	    nvme_poll_irqdisable(nvmeq, req->tag)) {
-		dev_warn(dev->ctrl.device,
-			 "I/O %d QID %d timeout, completion polled\n",
-			 req->tag, nvmeq->qid);
-		return BLK_EH_DONE;
+	if (!test_bit(NVMEQ_POLLED, &nvmeq->flags)) {
+		nvme_poll_irqdisable(nvmeq);
+		if (blk_mq_rq_state(req) != MQ_RQ_IN_FLIGHT) {
+			dev_warn(dev->ctrl.device,
+				 "I/O %d QID %d timeout, completion polled\n",
+				 req->tag, nvmeq->qid);
+			return BLK_EH_DONE;
+		}
 	}
 
 	/*
@@ -1409,7 +1408,7 @@ static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown)
 	else
 		nvme_disable_ctrl(&dev->ctrl, dev->ctrl.cap);
 
-	nvme_poll_irqdisable(nvmeq, -1);
+	nvme_poll_irqdisable(nvmeq);
 }
 
 static int nvme_cmb_qdepth(struct nvme_dev *dev, int nr_io_queues,
@@ -2286,7 +2285,7 @@ static bool __nvme_disable_io_queues(struct nvme_dev *dev, u8 opcode)
 		/* handle any remaining CQEs */
 		if (opcode == nvme_admin_delete_cq &&
 		    !test_bit(NVMEQ_DELETE_ERROR, &nvmeq->flags))
-			nvme_poll_irqdisable(nvmeq, -1);
+			nvme_poll_irqdisable(nvmeq);
 
 		sent--;
 		if (nr_queues)
-- 
2.14.4

[PATCH 4/8] nvme/pci: Remove last_cq_seen

[Keith Busch]

The driver had been saving the last CQ seen in case polling competes
with an interrupt handler in order to defeat spurious interrupt
detection. Polling only applies to special queues that don't have
interrupt handlers now, so we don't need to save this.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 45db4a39795e..62f29ab93f5a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -193,7 +193,6 @@ struct nvme_queue {
 	u16 sq_tail;
 	u16 last_sq_tail;
 	u16 cq_head;
-	u16 last_cq_head;
 	u16 qid;
 	u8 cq_phase;
 	unsigned long flags;
@@ -1044,10 +1043,7 @@ static irqreturn_t nvme_irq(int irq, void *data)
 	 * 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);
-	nvmeq->last_cq_head = nvmeq->cq_head;
 	wmb();
 
 	if (start != end) {
-- 
2.14.4

[PATCH 5/8] nvme/pci: Remove last_sq_tail

[Keith Busch]

We don't allocate enough tags to wrap the submission queue ring. Remove
the checks for this condition and shrink struct nvme_queue.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 62f29ab93f5a..06db19b45d2b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -191,7 +191,6 @@ struct nvme_queue {
 	u16 q_depth;
 	u16 cq_vector;
 	u16 sq_tail;
-	u16 last_sq_tail;
 	u16 cq_head;
 	u16 qid;
 	u8 cq_phase;
@@ -514,21 +513,11 @@ static int nvme_pci_map_queues(struct blk_mq_tag_set *set)
 /*
  * Write sq tail if we are asked to, or if the next command would wrap.
  */
-static inline void nvme_write_sq_db(struct nvme_queue *nvmeq, bool write_sq)
+static inline void nvme_write_sq_db(struct nvme_queue *nvmeq)
 {
-	if (!write_sq) {
-		u16 next_tail = nvmeq->sq_tail + 1;
-
-		if (next_tail == nvmeq->q_depth)
-			next_tail = 0;
-		if (next_tail != nvmeq->last_sq_tail)
-			return;
-	}
-
 	if (nvme_dbbuf_update_and_check_event(nvmeq->sq_tail,
 			nvmeq->dbbuf_sq_db, nvmeq->dbbuf_sq_ei))
 		writel(nvmeq->sq_tail, nvmeq->q_db);
-	nvmeq->last_sq_tail = nvmeq->sq_tail;
 }
 
 /**
@@ -544,7 +533,8 @@ static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
 	memcpy(&nvmeq->sq_cmds[nvmeq->sq_tail], cmd, sizeof(*cmd));
 	if (++nvmeq->sq_tail == nvmeq->q_depth)
 		nvmeq->sq_tail = 0;
-	nvme_write_sq_db(nvmeq, write_sq);
+	if (write_sq)
+		nvme_write_sq_db(nvmeq);
 	spin_unlock(&nvmeq->sq_lock);
 }
 
@@ -553,8 +543,7 @@ static void nvme_commit_rqs(struct blk_mq_hw_ctx *hctx)
 	struct nvme_queue *nvmeq = hctx->driver_data;
 
 	spin_lock(&nvmeq->sq_lock);
-	if (nvmeq->sq_tail != nvmeq->last_sq_tail)
-		nvme_write_sq_db(nvmeq, true);
+	nvme_write_sq_db(nvmeq);
 	spin_unlock(&nvmeq->sq_lock);
 }
 
@@ -1507,7 +1496,6 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
 	struct nvme_dev *dev = nvmeq->dev;
 
 	nvmeq->sq_tail = 0;
-	nvmeq->last_sq_tail = 0;
 	nvmeq->cq_head = 0;
 	nvmeq->cq_phase = 1;
 	nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride];
-- 
2.14.4

[PATCH 6/8] nvme/pci: Remove q_dmadev from nvme_queue

[Keith Busch]

We don't need to save the dma device as it's not used in the hot path
and hasn't in a long time. Shrink the struct nvme_queue removing this
unnecessary member.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 06db19b45d2b..3481f3e3d6a1 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -177,7 +177,6 @@ static inline struct nvme_dev *to_nvme_dev(struct nvme_ctrl *ctrl)
  * commands and one for I/O commands).
  */
 struct nvme_queue {
-	struct device *q_dmadev;
 	struct nvme_dev *dev;
 	spinlock_t sq_lock;
 	struct nvme_command *sq_cmds;
@@ -1336,16 +1335,16 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 
 static void nvme_free_queue(struct nvme_queue *nvmeq)
 {
-	dma_free_coherent(nvmeq->q_dmadev, CQ_SIZE(nvmeq->q_depth),
+	dma_free_coherent(nvmeq->dev->dev, CQ_SIZE(nvmeq->q_depth),
 				(void *)nvmeq->cqes, nvmeq->cq_dma_addr);
 	if (!nvmeq->sq_cmds)
 		return;
 
 	if (test_and_clear_bit(NVMEQ_SQ_CMB, &nvmeq->flags)) {
-		pci_free_p2pmem(to_pci_dev(nvmeq->q_dmadev),
+		pci_free_p2pmem(to_pci_dev(nvmeq->dev->dev),
 				nvmeq->sq_cmds, SQ_SIZE(nvmeq->q_depth));
 	} else {
-		dma_free_coherent(nvmeq->q_dmadev, SQ_SIZE(nvmeq->q_depth),
+		dma_free_coherent(nvmeq->dev->dev, SQ_SIZE(nvmeq->q_depth),
 				nvmeq->sq_cmds, nvmeq->sq_dma_addr);
 	}
 }
@@ -1457,7 +1456,6 @@ static int nvme_alloc_queue(struct nvme_dev *dev, int qid, int depth)
 	if (nvme_alloc_sq_cmds(dev, nvmeq, qid, depth))
 		goto free_cqdma;
 
-	nvmeq->q_dmadev = dev->dev;
 	nvmeq->dev = dev;
 	spin_lock_init(&nvmeq->sq_lock);
 	spin_lock_init(&nvmeq->cq_poll_lock);
-- 
2.14.4

[PATCH 7/8] nvme/pci: Remove volatile from cqe

[Keith Busch]

The cqe isn't volatile once we confirm the phase bit. Remove the volatile
keyword and let the compiler optimize out an unnecessary additional read
to the command id.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 3481f3e3d6a1..9108a2ceb2c5 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -956,7 +956,7 @@ static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq)
 
 static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
 {
-	volatile struct nvme_completion *cqe = &nvmeq->cqes[idx];
+	struct nvme_completion *cqe = (struct nvme_completion *)&nvmeq->cqes[idx];
 	struct request *req;
 
 	if (unlikely(cqe->command_id >= nvmeq->q_depth)) {
-- 
2.14.4

[PATCH 8/8] nvme/pci: Remove unused nvme_iod member

[Keith Busch]

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 9108a2ceb2c5..2d50630e0ba9 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -218,7 +218,6 @@ struct nvme_iod {
 	int aborted;
 	int npages;		/* In the PRP list. 0 means small pool in use */
 	int nents;		/* Used in scatterlist */
-	int length;		/* Of data, in bytes */
 	dma_addr_t first_dma;
 	struct scatterlist meta_sg; /* metadata requires single contiguous buffer */
 	struct scatterlist *sg;
@@ -591,7 +590,6 @@ static blk_status_t nvme_init_iod(struct request *rq, struct nvme_dev *dev)
 	iod->aborted = 0;
 	iod->npages = -1;
 	iod->nents = 0;
-	iod->length = size;
 
 	return BLK_STS_OK;
 }
-- 
2.14.4

[PATCH 1/8] nvme/pci: Use a flag for polled queues

[Christoph Hellwig]

On Fri, Mar 08, 2019@10:43:06AM -0700, Keith Busch wrote:
> A negative value for the cq_vector used to mean the queue is either
> disabled or a polled queue. However, we have a queue enabled flag,
> so the cq_vector had been serving double duty.
> 
> Don't overload the meaning of cq_vector. Use a flag specific to the
> polled queues instead.

Looks good,

a few nitpicks below:

> -	nvmeq->cq_vector = -1;
> +	if (!test_and_clear_bit(NVMEQ_POLLED, &nvmeq->flags))
> +		pci_free_irq(to_pci_dev(nvmeq->dev->dev), nvmeq->cq_vector, nvmeq);
>  	return 0;

This creates an > 80 char line.

But given that the nvme_suspend_queue return value is always ignored
you could throw in a prep patch to remove the return value and just
do an early return for the poll case here.

> @@ -1552,7 +1547,7 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid, bool polled)
>  	if (!polled)
>  		vector = dev->num_vecs == 1 ? 0 : qid;
>  	else
> -		vector = -1;
> +		set_bit(NVMEQ_POLLED, &nvmeq->flags);

Between the inversion and the ? : this looks pretty obsfucated.
Can we untangle this to:

	if (polled)
		set_bit(NVMEQ_POLLED, &nvmeq->flags);
	else if (dev->num_vecs == 1)
		vector = 0;
	else
		vector = qid;

Or we might just kill the polled argument and the local vector variable
entirely, set the NVMEQ_POLLED flag in nvme_create_io_queues and
calculate the vector inside adapter_alloc_cq, which would also lose the
vector argument.

[PATCH 2/8] nvme/pci: Don't poll polled queues in timeout

[Christoph Hellwig]

> Polled queues are already polled. If a timeout occurs on such a queue,
> escalate the error recovery immediately.

But don't we still need to poll them when tearing down the queues?

[PATCH 3/8] nvme/pci: Remove tag check in nvme_process_cq

[Christoph Hellwig]

On Fri, Mar 08, 2019@10:43:08AM -0700, Keith Busch wrote:
> Simplify processing completions by removing the check for the optional
> search tag. The timeout handling was the only place using this, but we
> can check the request state directly instead of searching the completion
> entries. There is a chance the command was not actually polled in this
> implementation, but we don't want to escalate error recovery in that
> case anyway, and the warning is still informative either way.
> 
> Signed-off-by: Keith Busch <keith.busch at intel.com>

Looks good:

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 4/8] nvme/pci: Remove last_cq_seen

[Christoph Hellwig]

> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 45db4a39795e..62f29ab93f5a 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -193,7 +193,6 @@ struct nvme_queue {
>  	u16 sq_tail;
>  	u16 last_sq_tail;
>  	u16 cq_head;
> -	u16 last_cq_head;
>  	u16 qid;
>  	u8 cq_phase;
>  	unsigned long flags;
> @@ -1044,10 +1043,7 @@ static irqreturn_t nvme_irq(int irq, void *data)
>  	 * 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);
> -	nvmeq->last_cq_head = nvmeq->cq_head;
>  	wmb();
>  
>  	if (start != end) {

This looks good, but we can also kill the ret variable now, and
simplify the tail a bit to:

	if (start == end)
		return IRQ_NONE;
	nvme_complete_cqes(nvmeq, start, end);
	return IRQ_HANDLED;

[PATCH 5/8] nvme/pci: Remove last_sq_tail

[Christoph Hellwig]

On Fri, Mar 08, 2019@10:43:10AM -0700, Keith Busch wrote:
> We don't allocate enough tags to wrap the submission queue ring. Remove
> the checks for this condition and shrink struct nvme_queue.

Hmm, I don't see how this is related to wrapping, more about avoiding
SQ doorbell writes until we actually commit the current batch of I/O.

It originated from:

ommit 04f3eafda6e05adc56afed4d3ae6e24aaa429058
Author: Jens Axboe <axboe at kernel.dk>
Date:   Thu Nov 29 10:02:29 2018 -0700

    nvme: implement mq_ops->commit_rqs() hook

[PATCH 6/8] nvme/pci: Remove q_dmadev from nvme_queue

[Christoph Hellwig]

On Fri, Mar 08, 2019@10:43:11AM -0700, Keith Busch wrote:
> We don't need to save the dma device as it's not used in the hot path
> and hasn't in a long time. Shrink the struct nvme_queue removing this
> unnecessary member.

Looks good,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 7/8] nvme/pci: Remove volatile from cqe

[Christoph Hellwig]

On Fri, Mar 08, 2019@10:43:12AM -0700, Keith Busch wrote:
> The cqe isn't volatile once we confirm the phase bit. Remove the volatile
> keyword and let the compiler optimize out an unnecessary additional read
> to the command id.

Hmm.  I've never been happy about how we used volatile to start with,
but I'm not sure more casts sort out this problem.

Wouldn't the right fix be to drop the volatile entirely and force
a strong barrier before first reading the phase bit?

[PATCH 8/8] nvme/pci: Remove unused nvme_iod member

[Christoph Hellwig]

Looks good.  So good that I have my own version of it:

http://git.infradead.org/users/hch/block.git/commitdiff/26cabe40a2225d3f9b78da0082f150036b150e03

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 5/8] nvme/pci: Remove last_sq_tail

[Keith Busch]

On Mon, Mar 11, 2019@07:31:31PM +0100, Christoph Hellwig wrote:
> On Fri, Mar 08, 2019@10:43:10AM -0700, Keith Busch wrote:
> > We don't allocate enough tags to wrap the submission queue ring. Remove
> > the checks for this condition and shrink struct nvme_queue.
> 
> Hmm, I don't see how this is related to wrapping, more about avoiding
> SQ doorbell writes until we actually commit the current batch of I/O.
> 
> It originated from:
> 
> ommit 04f3eafda6e05adc56afed4d3ae6e24aaa429058
> Author: Jens Axboe <axboe at kernel.dk>
> Date:   Thu Nov 29 10:02:29 2018 -0700
> 
>     nvme: implement mq_ops->commit_rqs() hook

Right, the batched doorblell writes is the commit, and this comment
explains the wrap detection requirement that it brought with it:

  "Write sq tail if we are asked to, or if the next command would wrap."

Batched DB writes would need this if we could wrap, and the wrap detection
has to compare the previously wrtten sq tail the next entry. We just don't
have enough tags to do that, so it's not necessary to check.

I should have changed that comment in this patch, though.

[PATCH 1/8] nvme/pci: Use a flag for polled queues

[Sagi Grimberg]

This ended up in my spam folder

(hope its not a hint to the review ;))

Looks good,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 2/8] nvme/pci: Don't poll polled queues in timeout

[Sagi Grimberg]

> Polled queues are already polled. If a timeout occurs on such a queue,
> escalate the error recovery immediately.
> 
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
>   drivers/nvme/host/pci.c | 20 +++++---------------
>   1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index d12b2861bdcc..e72e34b0b441 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1077,20 +1077,9 @@ static int nvme_poll_irqdisable(struct nvme_queue *nvmeq, unsigned int tag)
>   	u16 start, end;
>   	int found;
>   
> -	/*
> -	 * 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);
> -		found = nvme_process_cq(nvmeq, &start, &end, tag);
> -		spin_unlock(&nvmeq->cq_poll_lock);
> -	} else {
> -		disable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
> -		found = nvme_process_cq(nvmeq, &start, &end, tag);
> -		enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
> -	}
> +	disable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
> +	found = nvme_process_cq(nvmeq, &start, &end, tag);
> +	enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));

Why do polled queues need irq disable?

>   
>   	nvme_complete_cqes(nvmeq, start, end);
>   	return found;
> @@ -1284,7 +1273,8 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
>   	/*
>   	 * Did we miss an interrupt?
>   	 */
> -	if (nvme_poll_irqdisable(nvmeq, req->tag)) {
> +	if (!test_bit(NVMEQ_POLLED, &nvmeq->flags) &&
> +	    nvme_poll_irqdisable(nvmeq, req->tag)) {
>   		dev_warn(dev->ctrl.device,
>   			 "I/O %d QID %d timeout, completion polled\n",
>   			 req->tag, nvmeq->qid);
> 

[PATCH 3/8] nvme/pci: Remove tag check in nvme_process_cq

[Sagi Grimberg]

This is good,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 4/8] nvme/pci: Remove last_cq_seen

[Sagi Grimberg]

>> index 45db4a39795e..62f29ab93f5a 100644
>> --- a/drivers/nvme/host/pci.c
>> +++ b/drivers/nvme/host/pci.c
>> @@ -193,7 +193,6 @@ struct nvme_queue {
>>   	u16 sq_tail;
>>   	u16 last_sq_tail;
>>   	u16 cq_head;
>> -	u16 last_cq_head;
>>   	u16 qid;
>>   	u8 cq_phase;
>>   	unsigned long flags;
>> @@ -1044,10 +1043,7 @@ static irqreturn_t nvme_irq(int irq, void *data)
>>   	 * 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);
>> -	nvmeq->last_cq_head = nvmeq->cq_head;
>>   	wmb();
>>   
>>   	if (start != end) {
> 
> This looks good, but we can also kill the ret variable now, and
> simplify the tail a bit to:
> 
> 	if (start == end)
> 		return IRQ_NONE;
> 	nvme_complete_cqes(nvmeq, start, end);
> 	return IRQ_HANDLED;

Why not just look at what nvme_process_cq is returning?

[PATCH 5/8] nvme/pci: Remove last_sq_tail

[Sagi Grimberg]

>>> We don't allocate enough tags to wrap the submission queue ring. Remove
>>> the checks for this condition and shrink struct nvme_queue.
>>
>> Hmm, I don't see how this is related to wrapping, more about avoiding
>> SQ doorbell writes until we actually commit the current batch of I/O.
>>
>> It originated from:
>>
>> ommit 04f3eafda6e05adc56afed4d3ae6e24aaa429058
>> Author: Jens Axboe <axboe at kernel.dk>
>> Date:   Thu Nov 29 10:02:29 2018 -0700
>>
>>      nvme: implement mq_ops->commit_rqs() hook
> 
> Right, the batched doorblell writes is the commit, and this comment
> explains the wrap detection requirement that it brought with it:
> 
>    "Write sq tail if we are asked to, or if the next command would wrap."
> 
> Batched DB writes would need this if we could wrap, and the wrap detection
> has to compare the previously wrtten sq tail the next entry. We just don't
> have enough tags to do that, so it's not necessary to check.
> 
> I should have changed that comment in this patch, though.

The code writes DB when the tail == q_depth, which is a wrap. Is this
DB write redundant? If not than it should be stated as this is changing
the logic.

[PATCH 6/8] nvme/pci: Remove q_dmadev from nvme_queue

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 7/8] nvme/pci: Remove volatile from cqe

[Sagi Grimberg]

>> The cqe isn't volatile once we confirm the phase bit. Remove the volatile
>> keyword and let the compiler optimize out an unnecessary additional read
>> to the command id.
> 
> Hmm.  I've never been happy about how we used volatile to start with,
> but I'm not sure more casts sort out this problem.
> 
> Wouldn't the right fix be to drop the volatile entirely and force
> a strong barrier before first reading the phase bit?

Is there a fundamental difference efficiency-wise?

[PATCH 8/8] nvme/pci: Remove unused nvme_iod member

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 1/8] nvme/pci: Use a flag for polled queues

[Christoph Hellwig]

Thanks,

applied to nvme-5.2.

[PATCH 6/8] nvme/pci: Remove q_dmadev from nvme_queue

[Christoph Hellwig]

Thanks,

applied to nvme-5.2.

[PATCH 8/8] nvme/pci: Remove unused nvme_iod member

[Christoph Hellwig]

Thanks,

applied to nvme-5.2.

[PATCH 4/8] nvme/pci: Remove last_cq_seen

[Christoph Hellwig]

On Mon, Mar 11, 2019@04:40:03PM -0700, Sagi Grimberg wrote:
>> This looks good, but we can also kill the ret variable now, and
>> simplify the tail a bit to:
>>
>> 	if (start == end)
>> 		return IRQ_NONE;
>> 	nvme_complete_cqes(nvmeq, start, end);
>> 	return IRQ_HANDLED;
>
> Why not just look at what nvme_process_cq is returning?

Yes, that might be even simpler.

[PATCH 7/8] nvme/pci: Remove volatile from cqe

[Christoph Hellwig]

On Mon, Mar 11, 2019@04:56:14PM -0700, Sagi Grimberg wrote:
>
>>> The cqe isn't volatile once we confirm the phase bit. Remove the volatile
>>> keyword and let the compiler optimize out an unnecessary additional read
>>> to the command id.
>>
>> Hmm.  I've never been happy about how we used volatile to start with,
>> but I'm not sure more casts sort out this problem.
>>
>> Wouldn't the right fix be to drop the volatile entirely and force
>> a strong barrier before first reading the phase bit?
>
> Is there a fundamental difference efficiency-wise?

I'm not sure.  The latter is just more our usual Linux style.