* [PATCH] nvme/pci: Remove last_sq_tail
@ 2019-12-05 20:08 Keith Busch
2019-12-05 20:31 ` Nadolski, Edmund
2019-12-12 9:20 ` Christoph Hellwig
0 siblings, 2 replies; 6+ messages in thread
From: Keith Busch @ 2019-12-05 20:08 UTC (permalink / raw)
To: linux-nvme, hch, sagi; +Cc: Keith Busch
We don't allocate enough tags to wrap the submission queue. Remove
tracking for this condition.
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
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 0590640ba62c..3acd1b4d7735 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -174,7 +174,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;
@@ -462,21 +461,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;
}
/**
@@ -493,7 +482,8 @@ static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
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);
}
@@ -502,8 +492,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);
}
@@ -1511,7 +1500,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.21.0
_______________________________________________
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] nvme/pci: Remove last_sq_tail
2019-12-05 20:08 [PATCH] nvme/pci: Remove last_sq_tail Keith Busch
@ 2019-12-05 20:31 ` Nadolski, Edmund
2019-12-05 20:48 ` Keith Busch
2019-12-12 9:20 ` Christoph Hellwig
1 sibling, 1 reply; 6+ messages in thread
From: Nadolski, Edmund @ 2019-12-05 20:31 UTC (permalink / raw)
To: Keith Busch, linux-nvme, hch, sagi
On 12/5/2019 1:08 PM, Keith Busch wrote:
> We don't allocate enough tags to wrap the submission queue. Remove
> tracking for this condition.
Can we ensure that this presumption won't change?
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
> 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 0590640ba62c..3acd1b4d7735 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -174,7 +174,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;
> @@ -462,21 +461,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.
Comment needs to be updated.
Ed
> */
> -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;
> }
>
> /**
> @@ -493,7 +482,8 @@ static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
> 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);
> }
>
> @@ -502,8 +492,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);
> }
>
> @@ -1511,7 +1500,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];
>
_______________________________________________
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] nvme/pci: Remove last_sq_tail
2019-12-05 20:31 ` Nadolski, Edmund
@ 2019-12-05 20:48 ` Keith Busch
0 siblings, 0 replies; 6+ messages in thread
From: Keith Busch @ 2019-12-05 20:48 UTC (permalink / raw)
To: Nadolski, Edmund; +Cc: hch, linux-nvme, sagi
On Thu, Dec 05, 2019 at 01:31:20PM -0700, Nadolski, Edmund wrote:
> On 12/5/2019 1:08 PM, Keith Busch wrote:
> > We don't allocate enough tags to wrap the submission queue. Remove
> > tracking for this condition.
>
> Can we ensure that this presumption won't change?
If we did have more tags, there'd be no guarantee we could even dispatch
them, and we'd have to track the CQE sq_head value to ensure an available
slot. There's really no performance win to adding that overhead to the
driver. On top of that, I'm aware there are more than a few controllers
that will break if we wrap the submission queue. So I don't see this
changing.
> > @@ -462,21 +461,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.
>
> Comment needs to be updated.
Yep, that comment would be outdated with this patch.
_______________________________________________
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] nvme/pci: Remove last_sq_tail
2019-12-05 20:08 [PATCH] nvme/pci: Remove last_sq_tail Keith Busch
2019-12-05 20:31 ` Nadolski, Edmund
@ 2019-12-12 9:20 ` Christoph Hellwig
2019-12-12 15:47 ` Keith Busch
1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2019-12-12 9:20 UTC (permalink / raw)
To: Keith Busch; +Cc: hch, linux-nvme, sagi
On Fri, Dec 06, 2019 at 05:08:25AM +0900, Keith Busch wrote:
> We don't allocate enough tags to wrap the submission queue. Remove
> tracking for this condition.
We can totally wrap the sq - just a single command at the end of
the SQ will wrap it. We are however never going to wrap it around
to the point that we'll overwrite other unsubmitted entries. So
I think you change looks good, but the commit log needs a better
explanation.
> @@ -502,8 +492,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);
Also this change means we now always submit in commit_rqs. This makes
total sense as we only commit after having submitted commands, but it
is another thing worth mentioning in the commit log.
_______________________________________________
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] nvme/pci: Remove last_sq_tail
2019-12-12 9:20 ` Christoph Hellwig
@ 2019-12-12 15:47 ` Keith Busch
2019-12-12 20:11 ` Sagi Grimberg
0 siblings, 1 reply; 6+ messages in thread
From: Keith Busch @ 2019-12-12 15:47 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: sagi, linux-nvme
On Thu, Dec 12, 2019 at 10:20:13AM +0100, Christoph Hellwig wrote:
> On Fri, Dec 06, 2019 at 05:08:25AM +0900, Keith Busch wrote:
> > We don't allocate enough tags to wrap the submission queue. Remove
> > tracking for this condition.
>
> We can totally wrap the sq - just a single command at the end of
> the SQ will wrap it. We are however never going to wrap it around
> to the point that we'll overwrite other unsubmitted entries. So
> I think you change looks good, but the commit log needs a better
> explanation.
Right, I mean "wrap" to indicate the tail passes the head.
> > @@ -502,8 +492,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);
>
> Also this change means we now always submit in commit_rqs. This makes
> total sense as we only commit after having submitted commands, but it
> is another thing worth mentioning in the commit log.
Yes, will update the comments.
_______________________________________________
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] nvme/pci: Remove last_sq_tail
2019-12-12 15:47 ` Keith Busch
@ 2019-12-12 20:11 ` Sagi Grimberg
0 siblings, 0 replies; 6+ messages in thread
From: Sagi Grimberg @ 2019-12-12 20:11 UTC (permalink / raw)
To: Keith Busch, Christoph Hellwig; +Cc: linux-nvme
You can add:
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
_______________________________________________
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:[~2019-12-12 20:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-05 20:08 [PATCH] nvme/pci: Remove last_sq_tail Keith Busch
2019-12-05 20:31 ` Nadolski, Edmund
2019-12-05 20:48 ` Keith Busch
2019-12-12 9:20 ` Christoph Hellwig
2019-12-12 15:47 ` Keith Busch
2019-12-12 20:11 ` Sagi Grimberg
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).