* [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).