* [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 related [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 related [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, other threads:[~2020-01-30 18:27 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).