Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/1] nvme: clear stale nvmeq->tags after tagset free
@ 2020-01-23 20:19 Edmund Nadolski
  2020-01-23 20:19 ` [PATCH v2 1/1] nvme: clear io tags when freeing tagset Edmund Nadolski
  0 siblings, 1 reply; 6+ messages in thread
From: Edmund Nadolski @ 2020-01-23 20:19 UTC (permalink / raw)
  To: edmund.nadolski, kbusch, linux-nvme

v2:
 - clear from nvme_tagset_free instead of .exit_hctx callback.

Edmund Nadolski (1):
  nvme: clear io tags when freeing tagset

 drivers/nvme/host/pci.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

-- 
2.20.1


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/1] nvme: clear io tags when freeing tagset
  2020-01-23 20:19 [PATCH v2 0/1] nvme: clear stale nvmeq->tags after tagset free Edmund Nadolski
@ 2020-01-23 20:19 ` Edmund Nadolski
  2020-01-24 18:29   ` Keith Busch
  2020-01-30 14:53   ` Christoph Hellwig
  0 siblings, 2 replies; 6+ messages in thread
From: Edmund Nadolski @ 2020-01-23 20:19 UTC (permalink / raw)
  To: edmund.nadolski, kbusch, linux-nvme

A tagset is invalidated by blk_mq_free_tag_set, so nvme_free_tagset must
clear nvmeq->tags for the io queues. A subsequent nvme_init_hctx will
re-init the tags if/when needed.

Signed-off-by: Edmund Nadolski <edmund.nadolski@intel.com>
---
 drivers/nvme/host/pci.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d37dcc1b629e..445c2ee2a01d 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2477,10 +2477,20 @@ static void nvme_release_prp_pools(struct nvme_dev *dev)
 	dma_pool_destroy(dev->prp_small_pool);
 }
 
+static void nvme_clear_io_tags(struct nvme_dev *dev)
+{
+	int i;
+
+	for (i = 1; i < max_queue_count(); i++)
+		dev->queues[i].tags = NULL;
+}
+
 static void nvme_free_tagset(struct nvme_dev *dev)
 {
-	if (dev->tagset.tags)
+	if (dev->tagset.tags) {
+		nvme_clear_io_tags(dev);
 		blk_mq_free_tag_set(&dev->tagset);
+	}
 	dev->ctrl.tagset = NULL;
 }
 
-- 
2.20.1


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/1] nvme: clear io tags when freeing tagset
  2020-01-23 20:19 ` [PATCH v2 1/1] nvme: clear io tags when freeing tagset Edmund Nadolski
@ 2020-01-24 18:29   ` Keith Busch
  2020-01-30 14:53   ` Christoph Hellwig
  1 sibling, 0 replies; 6+ messages in thread
From: Keith Busch @ 2020-01-24 18:29 UTC (permalink / raw)
  To: Edmund Nadolski; +Cc: linux-nvme

On Thu, Jan 23, 2020 at 01:19:47PM -0700, Edmund Nadolski wrote:
> A tagset is invalidated by blk_mq_free_tag_set, so nvme_free_tagset must
> clear nvmeq->tags for the io queues. A subsequent nvme_init_hctx will
> re-init the tags if/when needed.
> 
> Signed-off-by: Edmund Nadolski <edmund.nadolski@intel.com>

Thanks, applied for-5.6.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/1] nvme: clear io tags when freeing tagset
  2020-01-23 20:19 ` [PATCH v2 1/1] nvme: clear io tags when freeing tagset Edmund Nadolski
  2020-01-24 18:29   ` Keith Busch
@ 2020-01-30 14:53   ` Christoph Hellwig
  2020-01-30 17:47     ` Keith Busch
  1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2020-01-30 14:53 UTC (permalink / raw)
  To: Edmund Nadolski; +Cc: kbusch, linux-nvme

Why do we even need to keep this tags pointer around?  We can trivially
find it at I/O completion time, see below.  Also we'll need to fix this
for all the transports.


diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 365a2ddbeaa7..da392b50f73e 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -167,7 +167,6 @@ struct nvme_queue {
 	 /* 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;
 	dma_addr_t cq_dma_addr;
 	u32 __iomem *q_db;
@@ -376,29 +375,17 @@ static int nvme_admin_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
 
 	WARN_ON(hctx_idx != 0);
 	WARN_ON(dev->admin_tagset.tags[0] != hctx->tags);
-	WARN_ON(nvmeq->tags);
 
 	hctx->driver_data = nvmeq;
-	nvmeq->tags = &dev->admin_tagset.tags[0];
 	return 0;
 }
 
-static void nvme_admin_exit_hctx(struct blk_mq_hw_ctx *hctx, unsigned int hctx_idx)
-{
-	struct nvme_queue *nvmeq = hctx->driver_data;
-
-	nvmeq->tags = NULL;
-}
-
 static int nvme_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
 			  unsigned int hctx_idx)
 {
 	struct nvme_dev *dev = data;
 	struct nvme_queue *nvmeq = &dev->queues[hctx_idx + 1];
 
-	if (!nvmeq->tags)
-		nvmeq->tags = &dev->tagset.tags[hctx_idx];
-
 	WARN_ON(dev->tagset.tags[hctx_idx] != hctx->tags);
 	hctx->driver_data = nvmeq;
 	return 0;
@@ -948,6 +935,13 @@ static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq)
 		writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
 }
 
+static inline struct blk_mq_tags *nvme_queue_tagset(struct nvme_queue *nvmeq)
+{
+	if (!nvmeq->qid)
+		return nvmeq->dev->admin_tagset.tags[0];
+	return nvmeq->dev->tagset.tags[nvmeq->qid - 1];
+}
+
 static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
 {
 	volatile struct nvme_completion *cqe = &nvmeq->cqes[idx];
@@ -972,7 +966,7 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
 		return;
 	}
 
-	req = blk_mq_tag_to_rq(*nvmeq->tags, cqe->command_id);
+	req = blk_mq_tag_to_rq(nvme_queue_tagset(nvmeq), cqe->command_id);
 	trace_nvme_sq(req, cqe->sq_head, nvmeq->sq_tail);
 	nvme_end_request(req, cqe->status, cqe->result);
 }
@@ -1572,7 +1566,6 @@ static const struct blk_mq_ops nvme_mq_admin_ops = {
 	.queue_rq	= nvme_queue_rq,
 	.complete	= nvme_pci_complete_rq,
 	.init_hctx	= nvme_admin_init_hctx,
-	.exit_hctx      = nvme_admin_exit_hctx,
 	.init_request	= nvme_init_request,
 	.timeout	= nvme_timeout,
 };


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/1] nvme: clear io tags when freeing tagset
  2020-01-30 14:53   ` Christoph Hellwig
@ 2020-01-30 17:47     ` Keith Busch
  2020-01-30 18:27       ` Christoph Hellwig
  0 siblings, 1 reply; 6+ messages in thread
From: Keith Busch @ 2020-01-30 17:47 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Edmund Nadolski, linux-nvme

On Thu, Jan 30, 2020 at 06:53:35AM -0800, Christoph Hellwig wrote:
> +static inline struct blk_mq_tags *nvme_queue_tagset(struct nvme_queue *nvmeq)
> +{
> +	if (!nvmeq->qid)
> +		return nvmeq->dev->admin_tagset.tags[0];
> +	return nvmeq->dev->tagset.tags[nvmeq->qid - 1];
> +}

It's been this way for so long, I thought we cached the address of the
tags in the nvmeq using it to avoid this more complex lookip, so I told
Ed not do the above. But the original commit doing this was to remove
relying on a specific hctx to reach the tags, so it's nothing to do with
any observed performance change ... And this makes struct nvme_queue
smaller too, so all the better.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/1] nvme: clear io tags when freeing tagset
  2020-01-30 17:47     ` Keith Busch
@ 2020-01-30 18:27       ` Christoph Hellwig
  0 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2020-01-30 18:27 UTC (permalink / raw)
  To: Keith Busch; +Cc: Christoph Hellwig, Edmund Nadolski, linux-nvme

On Fri, Jan 31, 2020 at 02:47:32AM +0900, Keith Busch wrote:
> On Thu, Jan 30, 2020 at 06:53:35AM -0800, Christoph Hellwig wrote:
> > +static inline struct blk_mq_tags *nvme_queue_tagset(struct nvme_queue *nvmeq)
> > +{
> > +	if (!nvmeq->qid)
> > +		return nvmeq->dev->admin_tagset.tags[0];
> > +	return nvmeq->dev->tagset.tags[nvmeq->qid - 1];
> > +}
> 
> It's been this way for so long, I thought we cached the address of the
> tags in the nvmeq using it to avoid this more complex lookip, so I told
> Ed not do the above. But the original commit doing this was to remove
> relying on a specific hctx to reach the tags, so it's nothing to do with
> any observed performance change ... And this makes struct nvme_queue
> smaller too, so all the better.

Also I just noticed in rdma, tcp and loop we actually already have this
helper and no tags pointer.  And it might have been my code originally.
I'll post a formal patch soon.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-23 20:19 [PATCH v2 0/1] nvme: clear stale nvmeq->tags after tagset free Edmund Nadolski
2020-01-23 20:19 ` [PATCH v2 1/1] nvme: clear io tags when freeing tagset Edmund Nadolski
2020-01-24 18:29   ` Keith Busch
2020-01-30 14:53   ` Christoph Hellwig
2020-01-30 17:47     ` Keith Busch
2020-01-30 18:27       ` Christoph Hellwig

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git