nvme-pci: make sure to flush sqe writes before db record update

RFC PATCH New Sagi Grimberg 4 participants 12 messages Mar 7, 2018 – May 31, 2018 NVMe

Share & tools

Sagi Grimberg

author maintainer

2018-03-07 17:56 UTC #0 lore raw

RFC PATCH 1/1 nvme-pci: make sure to flush sqe writes before db record update

A wmb() is always required before operations that trigger DMA (e.g. mmio write to a doorbell record). Not having it might result in write reordering causing the device to fetch partly updated submission queue entries.

Reported-by Jason Gunthorpe <jgg at ziepe.ca>

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
Note that this is stable material, We could add stable tag when applying it to the nvme tree.

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

drivers/nvme/host/pci.c +7 -1

index db4dedb1cc8f..6c2dcef1ff91 100644
@@ -437,8 +437,14 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
 	if (++tail == nvmeq->q_depth)
 		tail = 0;
 	if (nvme_dbbuf_update_and_check_event(tail, nvmeq->dbbuf_sq_db,
-					      nvmeq->dbbuf_sq_ei))
+					      nvmeq->dbbuf_sq_ei)) {
+		/*
+		 * Make sure that descriptors are written before
+		 * doorbell record.
+		 */
+		wmb();
 		writel(tail, nvmeq->q_db);
+	}
 	nvmeq->sq_tail = tail;
 }

Signature · 1 line

2.14.1

Christoph Hellwig

maintainer

2018-03-08 07:56 UTC #1 lore raw

RFC PATCH 1/1 nvme-pci: make sure to flush sqe writes before db record update

Yes, looks like we'd need this.

Quoted 36 lines · Sagi Grimberg

A wmb() is always required before operations that trigger DMA (e.g. mmio write to a doorbell record). Not having it might result in write reordering causing the device to fetch partly updated submission queue entries.
Reported-by Jason Gunthorpe <jgg at ziepe.ca>
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
Note that this is stable material, We could add stable tag when applying it to the nvme tree.
 drivers/nvme/host/pci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/nvme/host/pci.c +7 -1
index db4dedb1cc8f..6c2dcef1ff91 100644
@@ -437,8 +437,14 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
 	if (++tail == nvmeq->q_depth)
 		tail = 0;
 	if (nvme_dbbuf_update_and_check_event(tail, nvmeq->dbbuf_sq_db,
-					      nvmeq->dbbuf_sq_ei))
+					      nvmeq->dbbuf_sq_ei)) {
+		/*
+		 * Make sure that descriptors are written before
+		 * doorbell record.
+		 */
+		wmb();
 		writel(tail, nvmeq->q_db);
+	}
 	nvmeq->sq_tail = tail;
 }
 
Signature · 1 line
2.14.1

---end quoted text---

Keith Busch

2018-03-08 15:51 UTC #2 lore raw

RFC PATCH 1/1 nvme-pci: make sure to flush sqe writes before db record update

In reply to Sagi Grimberg

Quoted 5 lines · Sagi Grimberg

drivers/nvme/host/pci.c +0 -1

@@ -437,8 +437,14 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
 	if (++tail == nvmeq->q_depth)
 		tail = 0;
 	if (nvme_dbbuf_update_and_check_event(tail, nvmeq->dbbuf_sq_db,
-					      nvmeq->dbbuf_sq_ei))

drivers/nvme/host/pci.c static void __nvme_submit_cmd(struct nvme_queue *nvmeq, · line 449

Quoted 10 lines

drivers/nvme/host/pci.c +7 -0

+					      nvmeq->dbbuf_sq_ei)) {
+		/*
+		 * Make sure that descriptors are written before
+		 * doorbell record.
+		 */
+		wmb();
 		writel(tail, nvmeq->q_db);
+	}
 	nvmeq->sq_tail = tail;
 }

If this really is necessary, we'd need this before updating the event shadow registers too.

I'd like to understand this a bit more as we haven't done this in eight years and I can't recall any issues around this section. Have we just been fortunate that the problem this fixes is extraordinarily unlikely, or is something else implicitly ordering within this critical section?

Jason Gunthorpe

2018-03-08 17:20 UTC #3 lore raw

RFC PATCH 1/1 nvme-pci: make sure to flush sqe writes before db record update

Quoted 20 lines · Keith Busch

Quoted 5 lines · Sagi Grimberg
drivers/nvme/host/pci.c +0 -1
@@ -437,8 +437,14 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
 	if (++tail == nvmeq->q_depth)
 		tail = 0;
 	if (nvme_dbbuf_update_and_check_event(tail, nvmeq->dbbuf_sq_db,
-					      nvmeq->dbbuf_sq_ei))
drivers/nvme/host/pci.c static void __nvme_submit_cmd(struct nvme_queue *nvmeq, · line 449
Quoted 10 lines
drivers/nvme/host/pci.c +7 -0
+					      nvmeq->dbbuf_sq_ei)) {
+		/*
+		 * Make sure that descriptors are written before
+		 * doorbell record.
+		 */
+		wmb();
 		writel(tail, nvmeq->q_db);
+	}
 	nvmeq->sq_tail = tail;
 }
If this really is necessary, we'd need this before updating the event shadow registers too.

Quoted 4 lines

I'd like to understand this a bit more as we haven't done this in eight years and I can't recall any issues around this section. Have we just been fortunate that the problem this fixes is extraordinarily unlikely, or is something else implicitly ordering within this critical section?

Well, there is a wmb() already inside nvme_dbbuf_update_and_check_event so any failure would only be related to dbbuf_sq_db being wrong when tail is written to q_db.

Guessing that might be a basically undetectable situation, maybe some temporary higher latency or something?

Jason

Keith Busch

2018-03-08 17:37 UTC #4 lore raw

RFC PATCH 1/1 nvme-pci: make sure to flush sqe writes before db record update

Quoted 21 lines · Jason Gunthorpe

Quoted 20 lines · Keith Busch
Quoted 5 lines · Sagi Grimberg
drivers/nvme/host/pci.c +0 -1
@@ -437,8 +437,14 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
 	if (++tail == nvmeq->q_depth)
 		tail = 0;
 	if (nvme_dbbuf_update_and_check_event(tail, nvmeq->dbbuf_sq_db,
-					      nvmeq->dbbuf_sq_ei))
drivers/nvme/host/pci.c static void __nvme_submit_cmd(struct nvme_queue *nvmeq, · line 449
Quoted 10 lines
drivers/nvme/host/pci.c +7 -0
+					      nvmeq->dbbuf_sq_ei)) {
+		/*
+		 * Make sure that descriptors are written before
+		 * doorbell record.
+		 */
+		wmb();
 		writel(tail, nvmeq->q_db);
+	}
 	nvmeq->sq_tail = tail;
 }
If this really is necessary, we'd need this before updating the event shadow registers too.

Quoted 8 lines

Quoted 4 lines
I'd like to understand this a bit more as we haven't done this in eight years and I can't recall any issues around this section. Have we just been fortunate that the problem this fixes is extraordinarily unlikely, or is something else implicitly ordering within this critical section?
Well, there is a wmb() already inside nvme_dbbuf_update_and_check_event so any failure would only be related to dbbuf_sq_db being wrong when tail is written to q_db.

Ah, nvme_dbbuf_update_and_check_event is used for CQ's too, which has no need for such a barrier. We just need it on the SQ side.

Sagi Grimberg

author maintainer

2018-03-08 17:56 UTC #5 lore raw

RFC PATCH 1/1 nvme-pci: make sure to flush sqe writes before db record update

In reply to Jason Gunthorpe

Quoted 20 lines

Quoted 20 lines
Quoted 6 lines
drivers/nvme/host/pci.c +0 -1
@@ -437,8 +437,14 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
  	if (++tail == nvmeq->q_depth)
  		tail = 0;
  	if (nvme_dbbuf_update_and_check_event(tail, nvmeq->dbbuf_sq_db,
-					      nvmeq->dbbuf_sq_ei))
drivers/nvme/host/pci.c static void __nvme_submit_cmd(struct nvme_queue *nvmeq, · line 449
Quoted 10 lines
drivers/nvme/host/pci.c +7 -0
+					      nvmeq->dbbuf_sq_ei)) {
+		/*
+		 * Make sure that descriptors are written before
+		 * doorbell record.
+		 */
+		wmb();
  		writel(tail, nvmeq->q_db);
+	}
  	nvmeq->sq_tail = tail;
  }
If this really is necessary, we'd need this before updating the event shadow registers too.

Quoted 8 lines

Quoted 4 lines
I'd like to understand this a bit more as we haven't done this in eight years and I can't recall any issues around this section. Have we just been fortunate that the problem this fixes is extraordinarily unlikely, or is something else implicitly ordering within this critical section?
Well, there is a wmb() already inside nvme_dbbuf_update_and_check_event so any failure would only be related to dbbuf_sq_db being wrong when tail is written to q_db.

Right, we have that covered.

Quoted 2 lines

Guessing that might be a basically undetectable situation, maybe some temporary higher latency or something?

If the SQE and DB update have been reordered, and the device was able to fetch the old copy of the SQE, it would be either silent data corruption or dma access to unmapped memory or unexpected behavior (if the SQE wasn't initialized). I just think its a *very* rare race that can happen...

Jason Gunthorpe

2018-03-08 18:36 UTC #6 lore raw

RFC PATCH 1/1 nvme-pci: make sure to flush sqe writes before db record update

Quoted 29 lines · Sagi Grimberg

Quoted 20 lines
Quoted 20 lines
Quoted 6 lines
drivers/nvme/host/pci.c +0 -1
@@ -437,8 +437,14 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
 	if (++tail == nvmeq->q_depth)
 		tail = 0;
 	if (nvme_dbbuf_update_and_check_event(tail, nvmeq->dbbuf_sq_db,
-					      nvmeq->dbbuf_sq_ei))
drivers/nvme/host/pci.c static void __nvme_submit_cmd(struct nvme_queue *nvmeq, · line 449
Quoted 10 lines
drivers/nvme/host/pci.c +7 -0
+					      nvmeq->dbbuf_sq_ei)) {
+		/*
+		 * Make sure that descriptors are written before
+		 * doorbell record.
+		 */
+		wmb();
 		writel(tail, nvmeq->q_db);
+	}
 	nvmeq->sq_tail = tail;
 }
If this really is necessary, we'd need this before updating the event shadow registers too.
Quoted 8 lines
Quoted 4 lines
I'd like to understand this a bit more as we haven't done this in eight years and I can't recall any issues around this section. Have we just been fortunate that the problem this fixes is extraordinarily unlikely, or is something else implicitly ordering within this critical section?
Well, there is a wmb() already inside nvme_dbbuf_update_and_check_event so any failure would only be related to dbbuf_sq_db being wrong when tail is written to q_db.

Quoted 6 lines

Right, we have that covered.
Quoted 2 lines
Guessing that might be a basically undetectable situation, maybe some temporary higher latency or something?
If the SQE and DB update have been reordered,

But that can't happen, the SQE is written before nvme_dbbuf_update_and_check_event(), and that function does wmb.

The only reordering is related to this:

		wmb();

		old_value = *dbbuf_db;
		*dbbuf_db = value;
[..]
	writel(tail, nvmeq->q_db);

So q_db and dbbuf_db could be swapped at the worst.

Jason

Keith Busch

2018-03-08 18:42 UTC #7 lore raw

RFC PATCH 1/1 nvme-pci: make sure to flush sqe writes before db record update

Quoted 5 lines · Jason Gunthorpe

Quoted 1 line · Sagi Grimberg
If the SQE and DB update have been reordered,
But that can't happen, the SQE is written before nvme_dbbuf_update_and_check_event(), and that function does wmb.

I believe Sagi is talking about the typical controller that doesn't have shadow db registers (no real nvme device has those), which doesn't currently go through a path calling wmb().

Sagi Grimberg

author maintainer

2018-05-30 23:34 UTC #8 lore raw

RFC PATCH 1/1 nvme-pci: make sure to flush sqe writes before db record update

In reply to Sagi Grimberg

resurrecting this one...

Can anyone say why this is _not_ needed?

Quoted 40 lines · Sagi Grimberg

Quoted 21 lines
Quoted 21 lines
Quoted 7 lines
(quoted diff) +0 -1
@@ -437,8 +437,14 @@ static void __nvme_submit_cmd(struct nvme_queue 
 *nvmeq,
 ????? if (++tail == nvmeq->q_depth)
 ????????? tail = 0;
 ????? if (nvme_dbbuf_update_and_check_event(tail, nvmeq->dbbuf_sq_db,
-????????????????????????? nvmeq->dbbuf_sq_ei))
Review static void __nvme_submit_cmd(struct nvme_queue · line 450
Quoted 10 lines
(quoted diff) +7 -0
+????????????????????????? nvmeq->dbbuf_sq_ei)) {
+??????? /*
+???????? * Make sure that descriptors are written before
+???????? * doorbell record.
+???????? */
+??????? wmb();
 ????????? writel(tail, nvmeq->q_db);
+??? }
 ????? nvmeq->sq_tail = tail;
 ? }
If this really is necessary, we'd need this before updating the event shadow registers too.
Quoted 8 lines
Quoted 4 lines
I'd like to understand this a bit more as we haven't done this in eight years and I can't recall any issues around this section. Have we just been fortunate that the problem this fixes is extraordinarily unlikely, or is something else implicitly ordering within this critical section?
Well, there is a wmb() already inside nvme_dbbuf_update_and_check_event so any failure would only be related to dbbuf_sq_db being wrong when tail is written to q_db.
Right, we have that covered.
Quoted 2 lines
Guessing that might be a basically undetectable situation, maybe some temporary higher latency or something?
If the SQE and DB update have been reordered, and the device was able to fetch the old copy of the SQE, it would be either silent data corruption or dma access to unmapped memory or unexpected behavior (if the SQE wasn't initialized). I just think its a *very* rare race that can happen...

Christoph Hellwig

maintainer

2018-05-31 05:47 UTC #9 lore raw

RFC PATCH 1/1 nvme-pci: make sure to flush sqe writes before db record update

Quoted 5 lines · Sagi Grimberg

resurrecting this one...
Can anyone say why this is _not_ needed?
From Documentation/memory-barriers.txt in linux-next, where this was

discussed once more not too log ago:

Note that, when using writel(), a prior wmb() is not needed to guarantee that the cache coherent memory writes have completed before writing to the MMIO region.

Sagi Grimberg

author maintainer

2018-05-31 07:53 UTC #10 lore raw

RFC PATCH 1/1 nvme-pci: make sure to flush sqe writes before db record update

Quoted 11 lines

Quoted 4 lines
resurrecting this one...
Can anyone say why this is _not_ needed?
From Documentation/memory-barriers.txt in linux-next, where this was
discussed once more not too log ago:
Note that, when using writel(), a prior wmb() is not needed to guarantee that the cache coherent memory writes have completed before writing to the MMIO region.

Thanks for clarifying, didn't know it was being discussed. I assume a bunch of other drivers need to clean it up now...

Jason Gunthorpe

2018-05-31 15:20 UTC #11 lore raw

RFC PATCH 1/1 nvme-pci: make sure to flush sqe writes before db record update

Quoted 8 lines · Sagi Grimberg

Quoted 8 lines
Quoted 4 lines
resurrecting this one...
Can anyone say why this is _not_ needed?
From Documentation/memory-barriers.txt in linux-next, where this was discussed once more not too log ago:

Quoted 6 lines

Quoted 3 lines
Note that, when using writel(), a prior wmb() is not needed to guarantee that the cache coherent memory writes have completed before writing to the MMIO region.
Thanks for clarifying, didn't know it was being discussed. I assume a bunch of other drivers need to clean it up now...

Yes, that was the conclusion. It is tricky though because the above statement is maybe only true for un-cached memory.

It is less clear what the semantics are for WC memory...

Jason