nvmet-rdma: Fix missing dma sync to nvme data structures

PATCH New Parav Pandit 4 participants 11 messages Jan 12, 2017 – Jan 14, 2017

Subsystems (3)

InfiniBand NVMe NVMe target

lore

Share & tools

lore

Parav Pandit

author

2017-01-12 22:45 UTC #0 lore raw

PATCH 1/1 nvmet-rdma: Fix missing dma sync to nvme data structures

This patch performs dma sync operations on nvme_commmand, inline page(s) and nvme_completion.

nvme_command and write cmd inline data is synced
(a) on receiving of the recv queue completion for cpu access.
(b) before posting recv wqe back to rdma adapter for device access.

nvme_completion is synced
(a) on receiving send completion for nvme_completion for cpu access.
(b) before posting send wqe to rdma adapter for device access.

Pushing this patch through linux-rdma tree as its more relavant with Bart's changes for dma_map_ops of[1].

[1] https://patchwork.kernel.org/patch/9514085/

Signed-off-by Parav Pandit <parav at mellanox.com>

Reviewed-by Max Gurtovoy <maxg at mellanox.com>

---
 drivers/nvme/target/rdma.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

drivers/nvme/target/rdma.c +27 -0

index 8c3760a..c6468b3 100644
@@ -438,6 +438,14 @@ static int nvmet_rdma_post_recv(struct nvmet_rdma_device *ndev,
 {
 	struct ib_recv_wr *bad_wr;
 
+	dma_sync_single_for_device(ndev->device->dma_device,
+			cmd->sge[0].addr, sizeof(*cmd->nvme_cmd),
+			DMA_FROM_DEVICE);
+
+	if (cmd->sge[1].addr)
+		dma_sync_single_for_device(ndev->device->dma_device,
+				cmd->sge[1].addr, NVMET_RDMA_INLINE_DATA_SIZE,
+				DMA_FROM_DEVICE);
 	if (ndev->srq)
 		return ib_post_srq_recv(ndev->srq, &cmd->wr, &bad_wr);
 	return ib_post_recv(cmd->queue->cm_id->qp, &cmd->wr, &bad_wr);

@@ -507,6 +515,10 @@ static void nvmet_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc)
 	struct nvmet_rdma_rsp *rsp =
 		container_of(wc->wr_cqe, struct nvmet_rdma_rsp, send_cqe);
 
+	dma_sync_single_for_cpu(rsp->queue->dev->device->dma_device,
+			rsp->send_sge.addr, sizeof(*rsp->req.rsp),
+			DMA_TO_DEVICE);
+
 	nvmet_rdma_release_rsp(rsp);
 
 	if (unlikely(wc->status != IB_WC_SUCCESS &&

@@ -538,6 +550,11 @@ static void nvmet_rdma_queue_response(struct nvmet_req *req)
 		first_wr = &rsp->send_wr;
 
 	nvmet_rdma_post_recv(rsp->queue->dev, rsp->cmd);
+
+	dma_sync_single_for_device(rsp->queue->dev->device->dma_device,
+			rsp->send_sge.addr, sizeof(*rsp->req.rsp),
+			DMA_TO_DEVICE);
+
 	if (ib_post_send(cm_id->qp, first_wr, &bad_wr)) {
 		pr_err("sending cmd response failed\n");
 		nvmet_rdma_release_rsp(rsp);

@@ -698,6 +715,16 @@ static void nvmet_rdma_handle_command(struct nvmet_rdma_queue *queue,
 	cmd->n_rdma = 0;
 	cmd->req.port = queue->port;
 
+	dma_sync_single_for_cpu(queue->dev->device->dma_device,
+			cmd->cmd->sge[0].addr, sizeof(*cmd->cmd->nvme_cmd),
+			DMA_FROM_DEVICE);
+
+	if (cmd->cmd->sge[1].addr)
+		dma_sync_single_for_cpu(queue->dev->device->dma_device,
+				cmd->cmd->sge[1].addr,
+				NVMET_RDMA_INLINE_DATA_SIZE,
+				DMA_FROM_DEVICE);
+
 	if (!nvmet_req_init(&cmd->req, &queue->nvme_cq,
 			&queue->nvme_sq, &nvmet_rdma_ops))
 		return;

Signature · 1 line

1.8.3.1

Christoph Hellwig

maintainer

2017-01-13 07:44 UTC #1 lore raw

PATCH 1/1 nvmet-rdma: Fix missing dma sync to nvme data structures

Quoted 7 lines · Parav Pandit

This patch performs dma sync operations on nvme_commmand, inline page(s) and nvme_completion.
nvme_command and write cmd inline data is synced
(a) on receiving of the recv queue completion for cpu access.
(b) before posting recv wqe back to rdma adapter for device access.

Quoted 8 lines

nvme_completion is synced
(a) on receiving send completion for nvme_completion for cpu access.
(b) before posting send wqe to rdma adapter for device access.
Pushing this patch through linux-rdma tree as its more relavant with Bart's changes for dma_map_ops of[1].
[1] https://patchwork.kernel.org/patch/9514085/

This seems like a fix for 4.10-rc as the old code is buggy on not DMA coherent architectures, so I suspect it should go in without being based on Barts cleanup.

Btw, what architecture did you test this with? One of the not coherent ARM SOCs?

Parav Pandit

author

2017-01-13 15:08 UTC #2 lore raw

PATCH 1/1 nvmet-rdma: Fix missing dma sync to nvme data structures

Hi Christoph,

Quoted 21 lines

-----Original Message-----
From: Christoph Hellwig [mailto:hch at lst.de]
Sent: Friday, January 13, 2017 1:44 AM
To: Parav Pandit <parav at mellanox.com>
Cc: hch at lst.de; sagi at grimberg.me; linux-nvme at lists.infradead.org; linux- rdma at vger.kernel.org; dledford at redhat.com
Subject: Re: [PATCH] nvmet-rdma: Fix missing dma sync to nvme data structures
Quoted 11 lines · Parav Pandit
This patch performs dma sync operations on nvme_commmand, inline page(s) and nvme_completion.
nvme_command and write cmd inline data is synced
(a) on receiving of the recv queue completion for cpu access.
(b) before posting recv wqe back to rdma adapter for device access.
nvme_completion is synced
(a) on receiving send completion for nvme_completion for cpu access.
(b) before posting send wqe to rdma adapter for device access.

Quoted 9 lines

Quoted 4 lines
Pushing this patch through linux-rdma tree as its more relavant with Bart's changes for dma_map_ops of[1].
[1] https://patchwork.kernel.org/patch/9514085/
This seems like a fix for 4.10-rc as the old code is buggy on not DMA coherent architectures, so I suspect it should go in without being based on Barts cleanup.

I first found and tested on git://git.infradead.org/nvme-fabrics.git in nvmf-4.10 branch. If it has to go to 4.10-rc in above git repo,

Doug needs to rebase to above git tree.
Followed by Bart's patches, followed by my changes in his tree. Without that, Linux-next will get compile error because ib_dma_xx won't be defined. Let me know, I have 4.10-rc patch also tested.

Quoted 2 lines

Btw, what architecture did you test this with? One of the not coherent ARM SOCs?

On x86_64.
Currently testing on ARM.
While doing some performance improvement using inline sge mode, I encounter this bug in review.

Sagi Grimberg

maintainer

2017-01-13 19:42 UTC #3 lore raw

PATCH 1/1 nvmet-rdma: Fix missing dma sync to nvme data structures

Quoted 5 lines

Doug needs to rebase to above git tree.
Followed by Bart's patches, followed by my changes in his tree. Without that, Linux-next will get compile error because ib_dma_xx won't be defined. Let me know, I have 4.10-rc patch also tested.

No need, we'll jest keep the patch as is, no compile error will generate.

Parav Pandit

2017-01-13 19:50 UTC #4 lore raw

PATCH 1/1 nvmet-rdma: Fix missing dma sync to nvme data structures

Hi Sagi,

Quoted 11 lines · Sagi Grimberg

Quoted 6 lines
Doug needs to rebase to above git tree.
Followed by Bart's patches, followed by my changes in his tree. Without that, Linux-next will get compile error because ib_dma_xx won't be defined.
Let me know, I have 4.10-rc patch also tested.
No need, we'll jest keep the patch as is, no compile error will generate.
--

If we keep the patch as it is in 4.10-rc, than I think IB/rxe will break with plays with virtual addresses.

Sagi Grimberg

maintainer

2017-01-13 20:01 UTC #5 lore raw

PATCH 1/1 nvmet-rdma: Fix missing dma sync to nvme data structures

Quoted 4 lines

Hi Sagi,
If we keep the patch as it is in 4.10-rc, than I think IB/rxe will break with plays with virtual addresses.

I've been seeing reports that nvme-rdma is not useable with rxe as it crashes pretty consistently.

In any event, we can either add this with 'ib_' prefix into 4.10-rc and add a patch to Bart's series or we can send a dedicated patch for stable.

Christoph, whats your preference?

Parav Pandit

author

2017-01-14 16:12 UTC #6 lore raw

PATCH 1/1 nvmet-rdma: Fix missing dma sync to nvme data structures

Hi Sagi, Christoph,

Quoted 15 lines

-----Original Message-----
From: Sagi Grimberg [mailto:sagi at grimberg.me]
Sent: Friday, January 13, 2017 2:01 PM
To: Parav Pandit <pandit.parav at gmail.com>
Cc: Parav Pandit <parav at mellanox.com>; Christoph Hellwig <hch at lst.de>; linux-nvme at lists.infradead.org; linux-rdma at vger.kernel.org; dledford at redhat.com
Subject: Re: [PATCH] nvmet-rdma: Fix missing dma sync to nvme data structures
Quoted 4 lines
Hi Sagi,
If we keep the patch as it is in 4.10-rc, than I think IB/rxe will break with plays with virtual addresses.

Quoted 7 lines

I've been seeing reports that nvme-rdma is not useable with rxe as it crashes pretty consistently.
In any event, we can either add this with 'ib_' prefix into 4.10-rc and add a patch to Bart's series or we can send a dedicated patch for stable.
Christoph, whats your preference?

I see there is some discussion with Bart's patch with respect to SDMA. So I guess it might take a while to merge his patchset.

How about doing this?
- add this with 'ib_' prefix into 4.10-rc.
- followed by cherry pick this commit in linux-rdma tree.

When Bart rebase it, changes of this fix will automatically get converted from ib_ to dma_. So shall I send patch for 4.10-rc with 'ib_' prefix?

Sagi Grimberg

maintainer

2017-01-14 21:00 UTC #7 lore raw

PATCH 1/1 nvmet-rdma: Fix missing dma sync to nvme data structures

Quoted 10 lines

I see there is some discussion with Bart's patch with respect to SDMA. So I guess it might take a while to merge his patchset.
How about doing this?
- add this with 'ib_' prefix into 4.10-rc.
- followed by cherry pick this commit in linux-rdma tree.
When Bart rebase it, changes of this fix will automatically get converted from ib_ to dma_. So shall I send patch for 4.10-rc with 'ib_' prefix?

Sounds good,

Thanks Parav.

Christoph Hellwig

maintainer

2017-01-14 16:18 UTC #8 lore raw

PATCH 1/1 nvmet-rdma: Fix missing dma sync to nvme data structures

In reply to Sagi Grimberg

Quoted 8 lines · Sagi Grimberg

I've been seeing reports that nvme-rdma is not useable with rxe as it crashes pretty consistently.
In any event, we can either add this with 'ib_' prefix into 4.10-rc and add a patch to Bart's series or we can send a dedicated patch for stable.
Christoph, whats your preference?

Get the ib_dma_* version of Parave's patch in for 4.10-rc.

Sagi Grimberg

maintainer

2017-01-14 21:04 UTC #9 lore raw

PATCH 1/1 nvmet-rdma: Fix missing dma sync to nvme data structures

Quoted 11 lines

Quoted 9 lines
I've been seeing reports that nvme-rdma is not useable with rxe as it crashes pretty consistently.
In any event, we can either add this with 'ib_' prefix into 4.10-rc and add a patch to Bart's series or we can send a dedicated patch for stable.
Christoph, whats your preference?
Get the ib_dma_* version of Parave's patch in for 4.10-rc.

Fine with me.

Sagi Grimberg

maintainer

2017-01-13 19:35 UTC #10 lore raw

PATCH 1/1 nvmet-rdma: Fix missing dma sync to nvme data structures

In reply to Christoph Hellwig

Quoted 4 lines

This seems like a fix for 4.10-rc as the old code is buggy on not DMA coherent architectures, so I suspect it should go in without being based on Barts cleanup.

It does,

We should also give it a stable 4.8+ tag.

I'll queue this for nvme rc4 pull-req.