svcrdma: Verify read-list fits within RPCSVC_MAXPAGES

PATCH J. Bruce Fields 3 participants 11 messages May 19, 2008 – May 25, 2008 8 messages not in the archive

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

J. Bruce Fields

author

2008-05-19 18:20 UTC #2 lore raw

Quoted 15 lines · Tom Tucker

A RDMA read-list cannot contain more elements than RPCSVC_MAXPAGES or it will overflow the DTO context. Verify this when processing the protocol header.
Signed-off-by Tom Tucker <[email protected]>
---
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +0 -0
index 6b16d8c..06ab484 100644

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c static int rdma_read_xdr(struct svcxprt_rdma *xprt, · line 313

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +2 -0

@@ -306,6 +306,8 @@ static int rdma_read_xdr(struct svcxprt_rdma *xprt,
 	ch_sge_ary = (struct chunk_sge *)tmp_ch_ctxt->sge;
 
 	svc_rdma_rcl_chunk_counts(ch, &ch_count, &byte_count);
+	if (ch_count > RPCSVC_MAXPAGES)
+		return -EINVAL;
 	sge_count = rdma_rcl_to_sge(xprt, rqstp, hdr_ctxt, rmsgp,
 				    sge, ch_sge_ary,
 				    ch_count, byte_count);

If the ch_count is just the total number of bytes to be read into this request, then don't we also need to know at what offset they're going to be inserted? (Shouldn't there be some check like ch->rc_position + ch_count > RPCSVC_MAXPAGES ?)

Also, do we verify somewhere (before calling svc_rdma_rcl_chunk_counts()) that rc_discrim is set on the last chunk?

--b.

Tom Tucker

2008-05-20 01:04 UTC #3 lore raw

Quoted 18 lines · J. Bruce Fields

Quoted 17 lines · Tom Tucker
A RDMA read-list cannot contain more elements than RPCSVC_MAXPAGES or it will overflow the DTO context. Verify this when processing the protocol header.
Signed-off-by Tom Tucker <[email protected]>
---
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +0 -0
index 6b16d8c..06ab484 100644
@@ -306,6 +306,8 @@ static int rdma_read_xdr(struct svcxprt_rdma *xprt,
 	ch_sge_ary = (struct chunk_sge *)tmp_ch_ctxt->sge;
 

Quoted 12 lines

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c static int rdma_read_xdr(struct svcxprt_rdma *xprt, · line 313
Quoted 6 lines
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +2 -0
 	svc_rdma_rcl_chunk_counts(ch, &ch_count, &byte_count);
+	if (ch_count > RPCSVC_MAXPAGES)
+		return -EINVAL;
 	sge_count = rdma_rcl_to_sge(xprt, rqstp, hdr_ctxt, rmsgp,
 				    sge, ch_sge_ary,
 				    ch_count, byte_count);
If the ch_count is just the total number of bytes to be read into this request, then don't we also need to know at what offset they're going to be inserted? (Shouldn't there be some check like ch->rc_position + ch_count > RPCSVC_MAXPAGES ?)

The ch_count is the number of RPCRDMA chunk elements in the read-list. It's not a byte count, but a scatter-gather-list length.

I think the local read-list buffer limits should be clamped by svc_rdma_rcl_chunk_counts, however, see below...

Quoted 3 lines

Also, do we verify somewhere (before calling svc_rdma_rcl_chunk_counts()) that rc_discrim is set on the last chunk?

No we don't and a Byzantine client could crash us. The computed byte_count should also be clamped here. I'll add this to the list -- nice catch.

This kind of check along with a bunch of others should go in svc_rdma_xdr_decode_req. I have these things planned for the 2.6.27 time-frame (along with Fast NSMR support).

Do you think it's more urgent?

Tom

Quoted 5 lines

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1211245672.31725.111.camel-SMNkleLxa3ZimH42XvhXlA@public.gmane.org

Talpey, Thomas

2008-05-20 13:27 UTC #5 lore raw

Quoted 1 line · Tom Tucker

No we don't and a Byzantine client could crash us.

That can be arranged... :-)

Quoted 5 lines

This kind of check along with a bunch of others should go in svc_rdma_xdr_decode_req. I have these things planned for the 2.6.27 time-frame (along with Fast NSMR support).
Do you think it's more urgent?

MHO is that it's important but not urgent, and it should be part of a later change. At Connectathon, no clients were sending any problematic requests, so I think it's unlikely this will affect us in the wild, for now.

Tom.

A message that is not in the archive (sent off-list or not yet received): RTPCLUEXC1-PRDh133t00000127-rtwIt2gI0FxT+ZUat5FNkAK/[email protected]

J. Bruce Fields

author

2008-05-20 13:56 UTC #7 lore raw

Quoted 9 lines · Thomas

Quoted 1 line · Tom Tucker
No we don't and a Byzantine client could crash us.
That can be arranged... :-)
Quoted 4 lines
This kind of check along with a bunch of others should go in svc_rdma_xdr_decode_req. I have these things planned for the 2.6.27 time-frame (along with Fast NSMR support).

Quoted 5 lines

Quoted 1 line
Do you think it's more urgent?
MHO is that it's important but not urgent, and it should be part of a later change. At Connectathon, no clients were sending any problematic requests, so I think it's unlikely this will affect us in the wild, for now.

Somewhere in the documentation, a really clear warning about the security assumptions would be useful. It could also help if the howto (on the web and in Documentation/filesystems/nfs-rdma.txt) included any instructions on necessary firewalling, etc.

By the way, the Kconfig help text for SUNRPC_XPRT_RDMA looks like it needs an update to mention the server?

--b.

Talpey, Thomas

2008-05-20 14:14 UTC #8 lore raw

Quoted 8 lines · J. Bruce Fields

Quoted 3 lines
MHO is that it's important but not urgent, and it should be part of a later change. At Connectathon, no clients were sending any problematic requests, so I think it's unlikely this will affect us in the wild, for now.
Somewhere in the documentation, a really clear warning about the security assumptions would be useful. It could also help if the howto (on the web and in Documentation/filesystems/nfs-rdma.txt) included any instructions on necessary firewalling, etc.

Agreed. The kernel (/proc) parameters are part of this, and it's time to spell them all out as well.

The protocol hardening we're talking about above isn't a security issue, of course. It's just basic and part of the implementation. The client, btw, has some fairly strict checking.

Quoted 2 lines

By the way, the Kconfig help text for SUNRPC_XPRT_RDMA looks like it needs an update to mention the server?

You're right - it only mentions the client. I thought we added that text when we simplified/collapsed the config.

Tom.

A message that is not in the archive (sent off-list or not yet received): [email protected]

J. Bruce Fields

author

2008-05-19 19:18 UTC #13 lore raw

Re: [PATCH 05/05] svcrdma: Add dma map count and WARN_ON

Quoted 2 lines · Tom Tucker

Add a dma map count in order to verify that all DMA mapping resources have been freed at unmount.

I'm not sure what you mean by "at unmount".

--b.

Quoted 106 lines

Signed-off-by Tom Tucker <[email protected]>

---
 include/linux/sunrpc/svc_rdma.h          |    1 +
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |    1 +
 net/sunrpc/xprtrdma/svc_rdma_sendto.c    |    3 +++
 net/sunrpc/xprtrdma/svc_rdma_transport.c |    5 +++++
 4 files changed, 10 insertions(+), 0 deletions(-)

include/linux/sunrpc/svc_rdma.h +1 -0

index 29bfc9b..0a9c431 100644
@@ -105,6 +105,7 @@ struct svcxprt_rdma {
 
 	struct ib_pd         *sc_pd;
 
+	atomic_t	     sc_dma_used;
 	atomic_t	     sc_ctxt_used;
 	struct list_head     sc_ctxt_free;
 	int		     sc_ctxt_cnt;

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +1 -0

index 174e888..9b3dff0 100644
@@ -229,6 +229,7 @@ static void rdma_set_ctxt_sge(struct svcxprt_rdma *xprt,
 	ctxt->count = count;
 	ctxt->direction = DMA_FROM_DEVICE;
 	for (i = 0; i < count; i++) {
+		atomic_inc(&xprt->sc_dma_used);
 		ctxt->sge[i].addr = (unsigned long)
 			ib_dma_map_single(xprt->sc_cm_id->device,
 					  (void*)sge[i].addr, sge[i].length,

net/sunrpc/xprtrdma/svc_rdma_sendto.c +3 -0

index 85931c4..cdb0732 100644
@@ -174,6 +174,7 @@ static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp,
 		sge_bytes = min((size_t)bc,
 				(size_t)(xdr_sge[xdr_sge_no].length-sge_off));
 		sge[sge_no].length = sge_bytes;
+		atomic_inc(&xprt->sc_dma_used);
 		sge[sge_no].addr =
 			ib_dma_map_single(xprt->sc_cm_id->device,
 					  (void *)

@@ -399,6 +400,7 @@ static int send_reply(struct svcxprt_rdma *rdma,
 	ctxt->count = 1;
 
 	/* Prepare the SGE for the RPCRDMA Header */
+	atomic_inc(&rdma->sc_dma_used);
 	ctxt->sge[0].addr =
 		ib_dma_map_page(rdma->sc_cm_id->device,
 				page, 0, PAGE_SIZE, DMA_TO_DEVICE);

@@ -411,6 +413,7 @@ static int send_reply(struct svcxprt_rdma *rdma,
 		sge_bytes = min((size_t)ctxt->sge[sge_no].length,
 				(size_t)byte_count);
 		byte_count -= sge_bytes;
+		atomic_inc(&rdma->sc_dma_used);
 		ctxt->sge[sge_no].addr =
 			ib_dma_map_single(rdma->sc_cm_id->device,
 					  (void *)

net/sunrpc/xprtrdma/svc_rdma_transport.c +5 -0

index 8a50586..68908b5 100644
@@ -155,6 +155,7 @@ static void svc_rdma_unmap_dma(struct svc_rdma_op_ctxt *ctxt)
 	struct svcxprt_rdma *xprt = ctxt->xprt;
 	int i;
 	for (i = 0; i < ctxt->count && ctxt->sge[i].length; i++) {
+		atomic_dec(&xprt->sc_dma_used);
 		ib_dma_unmap_single(xprt->sc_cm_id->device,
 				    ctxt->sge[i].addr,
 				    ctxt->sge[i].length,

@@ -493,6 +494,7 @@ static struct svcxprt_rdma *rdma_create_xprt(struct svc_serv *serv,
 	cma_xprt->sc_max_requests = svcrdma_max_requests;
 	cma_xprt->sc_sq_depth = svcrdma_max_requests * RPCRDMA_SQ_DEPTH_MULT;
 	atomic_set(&cma_xprt->sc_sq_count, 0);
+	atomic_set(&cma_xprt->sc_ctxt_used, 0);
 
 	if (!listener) {
 		int reqs = cma_xprt->sc_max_requests;

@@ -543,6 +545,7 @@ int svc_rdma_post_recv(struct svcxprt_rdma *xprt)
 		BUG_ON(sge_no >= xprt->sc_max_sge);
 		page = svc_rdma_get_page();
 		ctxt->pages[sge_no] = page;
+		atomic_inc(&xprt->sc_dma_used);
 		pa = ib_dma_map_page(xprt->sc_cm_id->device,
 				     page, 0, PAGE_SIZE,
 				     DMA_FROM_DEVICE);

@@ -1023,6 +1026,7 @@ static void __svc_rdma_free(struct work_struct *work)
 
 	/* Warn if we leaked a resource or under-referenced */
 	WARN_ON(atomic_read(&rdma->sc_ctxt_used) != 0);
+	WARN_ON(atomic_read(&rdma->sc_dma_used) != 0);
 
 	/* Destroy the QP if present (not a listener) */
 	if (rdma->sc_qp && !IS_ERR(rdma->sc_qp))

@@ -1143,6 +1147,7 @@ void svc_rdma_send_error(struct svcxprt_rdma *xprt, struct rpcrdma_msg *rmsgp,
 	length = svc_rdma_xdr_encode_error(xprt, rmsgp, err, va);
 
 	/* Prepare SGE for local address */
+	atomic_inc(&xprt->sc_dma_used);
 	sge.addr = ib_dma_map_page(xprt->sc_cm_id->device,
 				   p, 0, PAGE_SIZE, DMA_FROM_DEVICE);
 	sge.lkey = xprt->sc_phys_mr->lkey;

Tom Tucker

2008-05-19 19:24 UTC #14 lore raw

Re: [PATCH 05/05] svcrdma: Add dma map count and WARN_ON

Quoted 5 lines · J. Bruce Fields

Quoted 2 lines · Tom Tucker
Add a dma map count in order to verify that all DMA mapping resources have been freed at unmount.
I'm not sure what you mean by "at unmount".

It would be clearer to say "when the transport is closed". I was testing using unmount to cause the close and had unmount on-the-brain.

Quoted 109 lines

--b.

Quoted 106 lines

Signed-off-by Tom Tucker <[email protected]>

---
 include/linux/sunrpc/svc_rdma.h          |    1 +
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |    1 +
 net/sunrpc/xprtrdma/svc_rdma_sendto.c    |    3 +++
 net/sunrpc/xprtrdma/svc_rdma_transport.c |    5 +++++
 4 files changed, 10 insertions(+), 0 deletions(-)

include/linux/sunrpc/svc_rdma.h +1 -0

index 29bfc9b..0a9c431 100644
@@ -105,6 +105,7 @@ struct svcxprt_rdma {
 
 	struct ib_pd         *sc_pd;
 
+	atomic_t	     sc_dma_used;
 	atomic_t	     sc_ctxt_used;
 	struct list_head     sc_ctxt_free;
 	int		     sc_ctxt_cnt;

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +1 -0

index 174e888..9b3dff0 100644
@@ -229,6 +229,7 @@ static void rdma_set_ctxt_sge(struct svcxprt_rdma *xprt,
 	ctxt->count = count;
 	ctxt->direction = DMA_FROM_DEVICE;
 	for (i = 0; i < count; i++) {
+		atomic_inc(&xprt->sc_dma_used);
 		ctxt->sge[i].addr = (unsigned long)
 			ib_dma_map_single(xprt->sc_cm_id->device,
 					  (void*)sge[i].addr, sge[i].length,

net/sunrpc/xprtrdma/svc_rdma_sendto.c +3 -0

index 85931c4..cdb0732 100644
@@ -174,6 +174,7 @@ static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp,
 		sge_bytes = min((size_t)bc,
 				(size_t)(xdr_sge[xdr_sge_no].length-sge_off));
 		sge[sge_no].length = sge_bytes;
+		atomic_inc(&xprt->sc_dma_used);
 		sge[sge_no].addr =
 			ib_dma_map_single(xprt->sc_cm_id->device,
 					  (void *)

@@ -399,6 +400,7 @@ static int send_reply(struct svcxprt_rdma *rdma,
 	ctxt->count = 1;
 
 	/* Prepare the SGE for the RPCRDMA Header */
+	atomic_inc(&rdma->sc_dma_used);
 	ctxt->sge[0].addr =
 		ib_dma_map_page(rdma->sc_cm_id->device,
 				page, 0, PAGE_SIZE, DMA_TO_DEVICE);

@@ -411,6 +413,7 @@ static int send_reply(struct svcxprt_rdma *rdma,
 		sge_bytes = min((size_t)ctxt->sge[sge_no].length,
 				(size_t)byte_count);
 		byte_count -= sge_bytes;
+		atomic_inc(&rdma->sc_dma_used);
 		ctxt->sge[sge_no].addr =
 			ib_dma_map_single(rdma->sc_cm_id->device,
 					  (void *)

net/sunrpc/xprtrdma/svc_rdma_transport.c +5 -0

index 8a50586..68908b5 100644
@@ -155,6 +155,7 @@ static void svc_rdma_unmap_dma(struct svc_rdma_op_ctxt *ctxt)
 	struct svcxprt_rdma *xprt = ctxt->xprt;
 	int i;
 	for (i = 0; i < ctxt->count && ctxt->sge[i].length; i++) {
+		atomic_dec(&xprt->sc_dma_used);
 		ib_dma_unmap_single(xprt->sc_cm_id->device,
 				    ctxt->sge[i].addr,
 				    ctxt->sge[i].length,

@@ -493,6 +494,7 @@ static struct svcxprt_rdma *rdma_create_xprt(struct svc_serv *serv,
 	cma_xprt->sc_max_requests = svcrdma_max_requests;
 	cma_xprt->sc_sq_depth = svcrdma_max_requests * RPCRDMA_SQ_DEPTH_MULT;
 	atomic_set(&cma_xprt->sc_sq_count, 0);
+	atomic_set(&cma_xprt->sc_ctxt_used, 0);
 
 	if (!listener) {
 		int reqs = cma_xprt->sc_max_requests;

@@ -543,6 +545,7 @@ int svc_rdma_post_recv(struct svcxprt_rdma *xprt)
 		BUG_ON(sge_no >= xprt->sc_max_sge);
 		page = svc_rdma_get_page();
 		ctxt->pages[sge_no] = page;
+		atomic_inc(&xprt->sc_dma_used);
 		pa = ib_dma_map_page(xprt->sc_cm_id->device,
 				     page, 0, PAGE_SIZE,
 				     DMA_FROM_DEVICE);

@@ -1023,6 +1026,7 @@ static void __svc_rdma_free(struct work_struct *work)
 
 	/* Warn if we leaked a resource or under-referenced */
 	WARN_ON(atomic_read(&rdma->sc_ctxt_used) != 0);
+	WARN_ON(atomic_read(&rdma->sc_dma_used) != 0);
 
 	/* Destroy the QP if present (not a listener) */
 	if (rdma->sc_qp && !IS_ERR(rdma->sc_qp))

@@ -1143,6 +1147,7 @@ void svc_rdma_send_error(struct svcxprt_rdma *xprt, struct rpcrdma_msg *rmsgp,
 	length = svc_rdma_xdr_encode_error(xprt, rmsgp, err, va);
 
 	/* Prepare SGE for local address */
+	atomic_inc(&xprt->sc_dma_used);
 	sge.addr = ib_dma_map_page(xprt->sc_cm_id->device,
 				   p, 0, PAGE_SIZE, DMA_FROM_DEVICE);
 	sge.lkey = xprt->sc_phys_mr->lkey;

J. Bruce Fields

author

2008-05-21 00:46 UTC #15 lore raw

Re: [PATCH 02/05] svcrdma: Refactor RDMA_WRITE dma mapping logic

In reply to a message not in the archive

Quoted 1 line · Tom Tucker

In order to make the dma mapping code easier to understand and reduce=

the

Quoted 1 line

number of I/O contexts required, move the DMA for RDMA_WRITE WR to th=

Quoted 1 line

code that prepares the WR SGE.

This one makes my (32-bit) compiler whine:

CC net/sunrpc/xprtrdma/svc_rdma_sendto.o
net/sunrpc/xprtrdma/svc_rdma_sendto.c: In function =E2=80=98send_write=E2= =80=99:
net/sunrpc/xprtrdma/svc_rdma_sendto.c:180: warning: passing argument 2 = of =E2=80=98ib_dma_map_single=E2=80=99 makes pointer from integer witho= ut a cast
net/sunrpc/xprtrdma/svc_rdma_sendto.c: In function =E2=80=98send_reply=E2= =80=99:
net/sunrpc/xprtrdma/svc_rdma_sendto.c:416: warning: passing argument 2 = of =E2=80=98ib_dma_map_single=E2=80=99 makes pointer from integer witho= ut a cast

And the types do seem weird:

Quoted 5 lines

=20
Signed-off-by Tom Tucker <[email protected]>
=20
---
net/sunrpc/xprtrdma/svc_rdma_sendto.c | 44 +++++++++++++++-----=

----------

net/sunrpc/xprtr=

Quoted 4 lines

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    5 +++-
 2 files changed, 26 insertions(+), 23 deletions(-)
=20
net/sunrpc/xprtr= renamed from net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0

dma/svc_rdma_sendto.c

net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_rd=

Quoted 4 lines

index fb82b1b..85931c4 100644
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -84,10 +84,7 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rd=

ma *xprt,

net/sunrpc/xprtrdma/svc_rdma_sendto.c

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0

 	sge_no =3D 1;

=20

net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -4

 	/* Head SGE */
-	sge[sge_no].addr =3D ib_dma_map_single(xprt->sc_cm_id->device,
-					     xdr->head[0].iov_base,
-					     xdr->head[0].iov_len,
-					     DMA_TO_DEVICE);
+	sge[sge_no].addr =3D (unsigned long)xdr->head[0].iov_base;

So before we called a function that took a void *, returned a u64; now we're storing a void* directly into a u64.

net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_rd=

Quoted 4 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0

 	sge_bytes =3D min_t(u32, byte_count, xdr->head[0].iov_len);
 	byte_count -=3D sge_bytes;
 	sge[sge_no].length =3D sge_bytes;

@@ -99,11 +96,9 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rd=

ma *xprt,

net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_=

Quoted 14 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +2 -4

 	page_bytes =3D xdr->page_len;
 	page_off =3D xdr->page_base;
 	while (byte_count && page_bytes) {
+		sge[sge_no].addr =3D (unsigned long)
+			page_address(xdr->pages[page_no]) + page_off;
 		sge_bytes =3D min_t(u32, byte_count, (PAGE_SIZE-page_off));
-		sge[sge_no].addr =3D
-			ib_dma_map_page(xprt->sc_cm_id->device,
-					xdr->pages[page_no], page_off,
-					sge_bytes, DMA_TO_DEVICE);
 		sge_bytes =3D min(sge_bytes, page_bytes);
 		byte_count -=3D sge_bytes;
 		page_bytes -=3D sge_bytes;

@@ -117,11 +112,7 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_=

rdma *xprt,

net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xprt, =

Quoted 13 lines

=20

net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -5

 	/* Tail SGE */
 	if (byte_count && xdr->tail[0].iov_len) {
-		sge[sge_no].addr =3D
-			ib_dma_map_single(xprt->sc_cm_id->device,
-					  xdr->tail[0].iov_base,
-					  xdr->tail[0].iov_len,
-					  DMA_TO_DEVICE);
+		sge[sge_no].addr =3D (unsigned long)xdr->tail[0].iov_base;
 		sge_bytes =3D min_t(u32, byte_count, xdr->tail[0].iov_len);
 		byte_count -=3D sge_bytes;
 		sge[sge_no].length =3D sge_bytes;

@@ -145,7 +136,6 @@ static int send_write(struct svcxprt_rdma *xprt, =

struct svc_rqst *rqstp,

net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xprt, =

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -1

 		      u32 xdr_off, int write_len,
 		      struct ib_sge *xdr_sge, int sge_count)
 {
-	struct svc_rdma_op_ctxt *tmp_sge_ctxt;
 	struct ib_send_wr write_wr;
 	struct ib_sge *sge;
 	int xdr_sge_no;

@@ -163,9 +153,8 @@ static int send_write(struct svcxprt_rdma *xprt, =

struct svc_rqst *rqstp,

net/sunrpc/xprtrdma/svc_rdma_sendto.c

Quoted 11 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0

 		write_len, xdr_sge, sge_count);

=20

net/sunrpc/xprtrdma/svc_rdma_sendto.c +2 -3

 	ctxt =3D svc_rdma_get_context(xprt);
-	ctxt->count =3D 0;
-	tmp_sge_ctxt =3D svc_rdma_get_context(xprt);
-	sge =3D tmp_sge_ctxt->sge;
+	ctxt->direction =3D DMA_TO_DEVICE;
+	sge =3D ctxt->sge;

=20

net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0

 	/* Find the SGE associated with xdr_off */
 	for (bc =3D xdr_off, xdr_sge_no =3D 1; bc && xdr_sge_no < sge_count=

;

net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xprt=

Quoted 1 line

net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -181,14 +170,20 @@ static int send_write(struct svcxprt_rdma *xprt=

, struct svc_rqst *rqstp,

Quoted 6 lines

=20

net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -1

 	/* Copy the remaining SGE */
 	while (bc !=3D 0 && xdr_sge_no < sge_count) {
-		sge[sge_no].addr =3D xdr_sge[xdr_sge_no].addr + sge_off;
 		sge[sge_no].lkey =3D xdr_sge[xdr_sge_no].lkey;
 		sge_bytes =3D min((size_t)bc,

net/sunrpc/xprtrdma/svc_rdma_sendto.c

Quoted 10 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +7 -1

 				(size_t)(xdr_sge[xdr_sge_no].length-sge_off));
 		sge[sge_no].length =3D sge_bytes;
-
+		sge[sge_no].addr =3D
+			ib_dma_map_single(xprt->sc_cm_id->device,
+					  (void *)
+					  xdr_sge[xdr_sge_no].addr + sge_off,
+					  sge_bytes, DMA_TO_DEVICE);
+		if (dma_mapping_error(sge[sge_no].addr))
+			return -EINVAL;

And then here we're casting the u64 back to a void *. Also, we're adding sge_off to the input, instead of to the result. Is it true that that

	ib_dma_map_single(., x + sge_off, ., .)

and

	ib_dma_map_single(., x, ., .) + sge_off

always have the same result?

--b.

net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xprt=

Quoted 7 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -0

 		sge_off =3D 0;
 		sge_no++;
+		ctxt->count++;
 		xdr_sge_no++;
 		bc -=3D sge_bytes;
 	}

@@ -210,11 +205,10 @@ static int send_write(struct svcxprt_rdma *xprt=

, struct svc_rqst *rqstp,

net/sunrpc/xp=

Quoted 36 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -2

 	/* Post It */
 	atomic_inc(&rdma_stat_write);
 	if (svc_rdma_send(xprt, &write_wr)) {
-		svc_rdma_put_context(ctxt, 1);
+		svc_rdma_put_context(ctxt, 0);
 		/* Fatal error, close transport */
 		ret =3D -EIO;
 	}
-	svc_rdma_put_context(tmp_sge_ctxt, 0);
 	return ret;
 }

=20

net/sunrpc/xprtrdma/svc_rdma_sendto.c +5 -0

@@ -417,6 +411,11 @@ static int send_reply(struct svcxprt_rdma *rdma,
 		sge_bytes =3D min((size_t)ctxt->sge[sge_no].length,
 				(size_t)byte_count);
 		byte_count -=3D sge_bytes;
+		ctxt->sge[sge_no].addr =3D
+			ib_dma_map_single(rdma->sc_cm_id->device,
+					  (void *)
+					  ctxt->sge[sge_no].addr,
+					  sge_bytes, DMA_TO_DEVICE);
 	}
 	BUG_ON(byte_count !=3D 0);

=20

net/sunrpc/xprtrdma/svc_rdma_sendto.c +2 -1

@@ -428,8 +427,9 @@ static int send_reply(struct svcxprt_rdma *rdma,
 		ctxt->pages[page_no+1] =3D rqstp->rq_respages[page_no];
 		ctxt->count++;
 		rqstp->rq_respages[page_no] =3D NULL;
+		if (page_no+1 >=3D sge_no)
+			ctxt->sge[page_no+1].length =3D 0;
 	}
-
 	BUG_ON(sge_no > rdma->sc_max_sge);
 	memset(&send_wr, 0, sizeof send_wr);
 	ctxt->wr_op =3D IB_WR_SEND;

net/sunrpc/xp= renamed from net/sunrpc/xprtrdma/svc_rdma_transport.c +0 -0

rtrdma/svc_rdma_transport.c

net/sunrpc/xprtrdma/svc_rdma_transport.c static void sq_cq_reap(struct svcxprt_rdma *xpr=

Quoted 4 lines

index e132509..9e9e244 100644
net/sunrpc/xprtrdma/svc_rdma_transport.c +0 -0
@@ -361,10 +361,13 @@ static void sq_cq_reap(struct svcxprt_rdma *xpr=

Quoted 14 lines

=20

net/sunrpc/xprtrdma/svc_rdma_transport.c +0 -1

 		switch (ctxt->wr_op) {
 		case IB_WR_SEND:
-		case IB_WR_RDMA_WRITE:
 			svc_rdma_put_context(ctxt, 1);
 			break;

=20

net/sunrpc/xprtrdma/svc_rdma_transport.c +4 -0

+		case IB_WR_RDMA_WRITE:
+			svc_rdma_put_context(ctxt, 0);
+			break;
+
 		case IB_WR_RDMA_READ:
 			if (test_bit(RDMACTXT_F_LAST_CTXT, &ctxt->flags)) {
 				struct svc_rdma_op_ctxt *read_hdr =3D ctxt->read_hdr;

J. Bruce Fields

author

2008-05-21 02:52 UTC #16 lore raw

Re: [PATCH 02/05] svcrdma: Refactor RDMA_WRITE dma mapping logic

Quoted 2 lines · J. Bruce Fields

Quoted 1 line · Tom Tucker
In order to make the dma mapping code easier to understand and redu=

ce the

Quoted 1 line

Quoted 1 line
number of I/O contexts required, move the DMA for RDMA_WRITE WR to =

the

Quoted 6 lines

Quoted 1 line
code that prepares the WR SGE.
=20
This one makes my (32-bit) compiler whine:
=20
CC net/sunrpc/xprtrdma/svc_rdma_sendto.o
net/sunrpc/xprtrdma/svc_rdma_sendto.c: In function =E2=80=98send_writ=

e=E2=80=99:

Quoted 1 line

net/sunrpc/xprtrdma/svc_rdma_sendto.c:180: warning: passing argument =

2 of =E2=80=98ib_dma_map_single=E2=80=99 makes pointer from integer wit= hout a cast

Quoted 1 line

net/sunrpc/xprtrdma/svc_rdma_sendto.c: In function =E2=80=98send_repl=

y=E2=80=99:

Quoted 1 line

net/sunrpc/xprtrdma/svc_rdma_sendto.c:416: warning: passing argument =

2 of =E2=80=98ib_dma_map_single=E2=80=99 makes pointer from integer wit= hout a cast

Erm, sorry, that should have been:

unrpc/xprtrdma/svc_rdma_sendto.c: In function =E2=80=98send_write=E2=80= =99:
net/sunrpc/xprtrdma/svc_rdma_sendto.c:180: warning: cast to pointer fro= m integer of different size net/sunrpc/xprtrdma/svc_rdma_sendto.c: In function =E2=80=98send_reply=E2= =80=99:
net/sunrpc/xprtrdma/svc_rdma_sendto.c:417: warning: cast to pointer fro= m integer of different size

--b.

Quoted 7 lines

And the types do seem weird:
=20
Quoted 5 lines
=20
Signed-off-by Tom Tucker <[email protected]>
=20
---
net/sunrpc/xprtrdma/svc_rdma_sendto.c | 44 +++++++++++++++---=

------------

Quoted 4 lines

net/sunrpc/xpr=
Quoted 4 lines
 net/sunrpc/xprtrdma/svc_rdma_transport.c |    5 +++-
 2 files changed, 26 insertions(+), 23 deletions(-)
=20
net/sunrpc/xpr= renamed from net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0

trdma/svc_rdma_sendto.c

Quoted 4 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_=
Quoted 4 lines
index fb82b1b..85931c4 100644
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -84,10 +84,7 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_=

rdma *xprt,

Quoted 10 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c
Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
 	sge_no =3D 1;
=20
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -4
 	/* Head SGE */
-	sge[sge_no].addr =3D ib_dma_map_single(xprt->sc_cm_id->device,
-					     xdr->head[0].iov_base,
-					     xdr->head[0].iov_len,
-					     DMA_TO_DEVICE);
+	sge[sge_no].addr =3D (unsigned long)xdr->head[0].iov_base;
=20
So before we called a function that took a void *, returned a u64; no=

Quoted 6 lines

we're storing a void* directly into a u64.
=20
net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_=
Quoted 4 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
 	sge_bytes =3D min_t(u32, byte_count, xdr->head[0].iov_len);
 	byte_count -=3D sge_bytes;
 	sge[sge_no].length =3D sge_bytes;
@@ -99,11 +96,9 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_=

rdma *xprt,

Quoted 6 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +2 -0

 	page_bytes =3D xdr->page_len;
 	page_off =3D xdr->page_base;
 	while (byte_count && page_bytes) {
+		sge[sge_no].addr =3D (unsigned long)
+			page_address(xdr->pages[page_no]) + page_off;
 		sge_bytes =3D min_t(u32, byte_count, (PAGE_SIZE-page_off));

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxpr=

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -4

-		sge[sge_no].addr =3D
-			ib_dma_map_page(xprt->sc_cm_id->device,
-					xdr->pages[page_no], page_off,
-					sge_bytes, DMA_TO_DEVICE);
 		sge_bytes =3D min(sge_bytes, page_bytes);
 		byte_count -=3D sge_bytes;
 		page_bytes -=3D sge_bytes;

@@ -117,11 +112,7 @@ static struct ib_sge *xdr_to_sge(struct svcxpr=

t_rdma *xprt,

Quoted 5 lines

=20

net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -2

 	/* Tail SGE */
 	if (byte_count && xdr->tail[0].iov_len) {
-		sge[sge_no].addr =3D
-			ib_dma_map_single(xprt->sc_cm_id->device,

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xprt=

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -3

-					  xdr->tail[0].iov_base,
-					  xdr->tail[0].iov_len,
-					  DMA_TO_DEVICE);
+		sge[sge_no].addr =3D (unsigned long)xdr->tail[0].iov_base;
 		sge_bytes =3D min_t(u32, byte_count, xdr->tail[0].iov_len);
 		byte_count -=3D sge_bytes;
 		sge[sge_no].length =3D sge_bytes;

@@ -145,7 +136,6 @@ static int send_write(struct svcxprt_rdma *xprt=

, struct svc_rqst *rqstp,

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xprt=

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -1

 		      u32 xdr_off, int write_len,
 		      struct ib_sge *xdr_sge, int sge_count)
 {
-	struct svc_rdma_op_ctxt *tmp_sge_ctxt;
 	struct ib_send_wr write_wr;
 	struct ib_sge *sge;
 	int xdr_sge_no;

@@ -163,9 +153,8 @@ static int send_write(struct svcxprt_rdma *xprt=

, struct svc_rqst *rqstp,

Quoted 11 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c
Quoted 11 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
 		write_len, xdr_sge, sge_count);
=20
net/sunrpc/xprtrdma/svc_rdma_sendto.c +2 -3
 	ctxt =3D svc_rdma_get_context(xprt);
-	ctxt->count =3D 0;
-	tmp_sge_ctxt =3D svc_rdma_get_context(xprt);
-	sge =3D tmp_sge_ctxt->sge;
+	ctxt->direction =3D DMA_TO_DEVICE;
+	sge =3D ctxt->sge;
=20
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
 	/* Find the SGE associated with xdr_off */
 	for (bc =3D xdr_off, xdr_sge_no =3D 1; bc && xdr_sge_no < sge_cou=

nt;

Quoted 1 line

net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xp=
Quoted 1 line
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -181,14 +170,20 @@ static int send_write(struct svcxprt_rdma *xp=

rt, struct svc_rqst *rqstp,

Quoted 11 lines

=20

net/sunrpc/xprtrdma/svc_rdma_sendto.c +2 -2

 	/* Copy the remaining SGE */
 	while (bc !=3D 0 && xdr_sge_no < sge_count) {
-		sge[sge_no].addr =3D xdr_sge[xdr_sge_no].addr + sge_off;
 		sge[sge_no].lkey =3D xdr_sge[xdr_sge_no].lkey;
 		sge_bytes =3D min((size_t)bc,
 				(size_t)(xdr_sge[xdr_sge_no].length-sge_off));
 		sge[sge_no].length =3D sge_bytes;
-
+		sge[sge_no].addr =3D
+			ib_dma_map_single(xprt->sc_cm_id->device,

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c
Quoted 5 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +5 -0
+					  (void *)
+					  xdr_sge[xdr_sge_no].addr + sge_off,
+					  sge_bytes, DMA_TO_DEVICE);
+		if (dma_mapping_error(sge[sge_no].addr))
+			return -EINVAL;
=20
And then here we're casting the u64 back to a void *. Also, we're adding sge_off to the input, instead of to the result. Is it true th=

Quoted 12 lines

that
=20
	ib_dma_map_single(., x + sge_off, ., .)
=20
and
=20
	ib_dma_map_single(., x, ., .) + sge_off
=20
always have the same result?
=20
--b.
=20

Quoted 7 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xp=
Quoted 7 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -0
 		sge_off =3D 0;
 		sge_no++;
+		ctxt->count++;
 		xdr_sge_no++;
 		bc -=3D sge_bytes;
 	}
@@ -210,11 +205,10 @@ static int send_write(struct svcxprt_rdma *xp=

rt, struct svc_rqst *rqstp,

Quoted 5 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -1

 	/* Post It */
 	atomic_inc(&rdma_stat_write);
 	if (svc_rdma_send(xprt, &write_wr)) {
-		svc_rdma_put_context(ctxt, 1);
+		svc_rdma_put_context(ctxt, 0);

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_reply(struct svcxprt_rdma *rdm=
Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -1
 		/* Fatal error, close transport */
 		ret =3D -EIO;
 	}
-	svc_rdma_put_context(tmp_sge_ctxt, 0);
 	return ret;
 }
=20
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -417,6 +411,11 @@ static int send_reply(struct svcxprt_rdma *rdm=

Quoted 4 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -0

 		sge_bytes =3D min((size_t)ctxt->sge[sge_no].length,
 				(size_t)byte_count);
 		byte_count -=3D sge_bytes;
+		ctxt->sge[sge_no].addr =3D

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_reply(struct svcxprt_rdma *rdma=
Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +4 -0
+			ib_dma_map_single(rdma->sc_cm_id->device,
+					  (void *)
+					  ctxt->sge[sge_no].addr,
+					  sge_bytes, DMA_TO_DEVICE);
 	}
 	BUG_ON(byte_count !=3D 0);
=20
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -428,8 +427,9 @@ static int send_reply(struct svcxprt_rdma *rdma=

Quoted 11 lines

net/sunrpc/=

Quoted 11 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +2 -1

 		ctxt->pages[page_no+1] =3D rqstp->rq_respages[page_no];
 		ctxt->count++;
 		rqstp->rq_respages[page_no] =3D NULL;
+		if (page_no+1 >=3D sge_no)
+			ctxt->sge[page_no+1].length =3D 0;
 	}
-
 	BUG_ON(sge_no > rdma->sc_max_sge);
 	memset(&send_wr, 0, sizeof send_wr);
 	ctxt->wr_op =3D IB_WR_SEND;

net/sunrpc/= renamed from net/sunrpc/xprtrdma/svc_rdma_transport.c +0 -0

xprtrdma/svc_rdma_transport.c

Quoted 4 lines

net/sunrpc/xprtrdma/svc_rdma_transport.c static void sq_cq_reap(struct svcxprt_rdma *x=
Quoted 4 lines
index e132509..9e9e244 100644
net/sunrpc/xprtrdma/svc_rdma_transport.c +0 -0
@@ -361,10 +361,13 @@ static void sq_cq_reap(struct svcxprt_rdma *x=

prt)

Quoted 8 lines

Quoted 8 lines
=20
net/sunrpc/xprtrdma/svc_rdma_transport.c +0 -1
 		switch (ctxt->wr_op) {
 		case IB_WR_SEND:
-		case IB_WR_RDMA_WRITE:
 			svc_rdma_put_context(ctxt, 1);
 			break;
=20
net/sunrpc/xprtrdma/svc_rdma_transport.c +1 -0
+		case IB_WR_RDMA_WRITE:

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_transport.c
Quoted 6 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c +3 -0
+			svc_rdma_put_context(ctxt, 0);
+			break;
+
 		case IB_WR_RDMA_READ:
 			if (test_bit(RDMACTXT_F_LAST_CTXT, &ctxt->flags)) {
 				struct svc_rdma_op_ctxt *read_hdr =3D ctxt->read_hdr;
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" =

Quoted 2 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Tom Tucker

2008-05-21 10:33 UTC #17 lore raw

Re: [PATCH 02/05] svcrdma: Refactor RDMA_WRITE dma mapping logic

Quoted 3 lines · J. Bruce Fields

Quoted 2 lines · J. Bruce Fields
Quoted 1 line · Tom Tucker
In order to make the dma mapping code easier to understand and redu=

ce the

Quoted 1 line

Quoted 1 line
Quoted 1 line
number of I/O contexts required, move the DMA for RDMA_WRITE WR to =

the

Quoted 7 lines

Quoted 7 lines
Quoted 1 line
code that prepares the WR SGE.
=20
This one makes my (32-bit) compiler whine:
=20
CC net/sunrpc/xprtrdma/svc_rdma_sendto.o
net/sunrpc/xprtrdma/svc_rdma_sendto.c: In function =8Csend_write=B9:
net/sunrpc/xprtrdma/svc_rdma_sendto.c:180: warning: passing argument=

2 of

Quoted 3 lines

Quoted 3 lines
=8Cib_dma_map_single=B9 makes pointer from integer without a cast net/sunrpc/xprtrdma/svc_rdma_sendto.c: In function =8Csend_reply=B9:
net/sunrpc/xprtrdma/svc_rdma_sendto.c:416: warning: passing argument=

2 of

Quoted 6 lines

Quoted 1 line
=8Cib_dma_map_single=B9 makes pointer from integer without a cast
=20
Erm, sorry, that should have been:
=20
unrpc/xprtrdma/svc_rdma_sendto.c: In function =8Csend_write=B9:
net/sunrpc/xprtrdma/svc_rdma_sendto.c:180: warning: cast to pointer f=

rom

Quoted 3 lines

integer of different size net/sunrpc/xprtrdma/svc_rdma_sendto.c: In function =8Csend_reply=B9:
net/sunrpc/xprtrdma/svc_rdma_sendto.c:417: warning: cast to pointer f=

rom

Quoted 2 lines

integer of different size

I have to jump on a plane, but I'll take a look tonight. Basically, the ib_sge is sometimes being used to save pointers and other times dma_add= r_t.
I need my own type there I guess.

Tom
=20

Quoted 4 lines

--b.
=20
Quoted 2 lines
And the types do seem weird:
=20

Quoted 15 lines

Quoted 7 lines
Quoted 7 lines
=20
Signed-off-by Tom Tucker <[email protected]>
=20
---
 net/sunrpc/xprtrdma/svc_rdma_sendto.c    |   44
+++++++++++++++---------------
 net/sunrpc/xprtrdma/svc_rdma_transport.c |    5 +++-
Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_=
Quoted 8 lines
 2 files changed, 26 insertions(+), 23 deletions(-)
=20
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
index fb82b1b..85931c4 100644
@@ -84,10 +84,7 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_=

rdma

Quoted 11 lines

Quoted 11 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c
Quoted 9 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
 *xprt,
 sge_no =3D 1;
=20
/* Head SGE */
- sge[sge_no].addr =3D ib_dma_map_single(xprt->sc_cm_id->device,
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -3
-          xdr->head[0].iov_base,
-          xdr->head[0].iov_len,
-          DMA_TO_DEVICE);
+ sge[sge_no].addr =3D (unsigned long)xdr->head[0].iov_base;
=20
So before we called a function that took a void *, returned a u64; n=

Quoted 6 lines

Quoted 6 lines
we're storing a void* directly into a u64.
=20
net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_=
Quoted 4 lines
sge_bytes =3D min_t(u32, byte_count, xdr->head[0].iov_len); byte_count -=3D sge_bytes;
sge[sge_no].length =3D sge_bytes;
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -99,11 +96,9 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_=

rdma

Quoted 7 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +2 -0

 *xprt,
 page_bytes =3D xdr->page_len;
 page_off =3D xdr->page_base;
 while (byte_count && page_bytes) {
+  sge[sge_no].addr =3D (unsigned long)
+   page_address(xdr->pages[page_no]) + page_off;
 sge_bytes =3D min_t(u32, byte_count, (PAGE_SIZE-page_off));

Quoted 8 lines

Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxpr=
Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -4
-  sge[sge_no].addr =3D
-   ib_dma_map_page(xprt->sc_cm_id->device,
-     xdr->pages[page_no], page_off,
-     sge_bytes, DMA_TO_DEVICE);
 sge_bytes =3D min(sge_bytes, page_bytes);
 byte_count -=3D sge_bytes;
page_bytes -=3D sge_bytes;
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -117,11 +112,7 @@ static struct ib_sge *xdr_to_sge(struct svcxpr=

t_rdma

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
 *xprt,
=20
/* Tail SGE */
if (byte_count && xdr->tail[0].iov_len) {
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -2
-  sge[sge_no].addr =3D
-   ib_dma_map_single(xprt->sc_cm_id->device,

Quoted 8 lines

Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xprt=
Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -3
-       xdr->tail[0].iov_base,
-       xdr->tail[0].iov_len,
-       DMA_TO_DEVICE);
+  sge[sge_no].addr =3D (unsigned long)xdr->tail[0].iov_base;
sge_bytes =3D min_t(u32, byte_count, xdr->tail[0].iov_len); byte_count -=3D sge_bytes;
sge[sge_no].length =3D sge_bytes;
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -145,7 +136,6 @@ static int send_write(struct svcxprt_rdma *xprt=

, struct

Quoted 9 lines

Quoted 9 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xprt=
Quoted 9 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -1
 svc_rqst *rqstp,
     u32 xdr_off, int write_len,
     struct ib_sge *xdr_sge, int sge_count)
 {
- struct svc_rdma_op_ctxt *tmp_sge_ctxt;
 struct ib_send_wr write_wr;
 struct ib_sge *sge;
int xdr_sge_no;
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -163,9 +153,8 @@ static int send_write(struct svcxprt_rdma *xprt=

, struct

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
 svc_rqst *rqstp,
 write_len, xdr_sge, sge_count);
=20
ctxt =3D svc_rdma_get_context(xprt);

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
- ctxt->count =3D 0;
- tmp_sge_ctxt =3D svc_rdma_get_context(xprt);
- sge =3D tmp_sge_ctxt->sge;
+ ctxt->direction =3D DMA_TO_DEVICE;
+ sge =3D ctxt->sge;
=20
/* Find the SGE associated with xdr_off */
for (bc =3D xdr_off, xdr_sge_no =3D 1; bc && xdr_sge_no < sge_count=

;

Quoted 1 line

Quoted 1 line
net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xp=
Quoted 1 line
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -181,14 +170,20 @@ static int send_write(struct svcxprt_rdma *xp=

rt,

Quoted 17 lines

Quoted 17 lines
Quoted 17 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
 struct svc_rqst *rqstp,
=20
/* Copy the remaining SGE */
while (bc !=3D 0 && xdr_sge_no < sge_count) {
- sge[sge_no].addr =3D xdr_sge[xdr_sge_no].addr + sge_off; sge[sge_no].lkey =3D xdr_sge[xdr_sge_no].lkey; sge_bytes =3D min((size_t)bc, (size_t)(xdr_sge[xdr_sge_no].length-sge_off)); sge[sge_no].length =3D sge_bytes;
net/sunrpc/xprtrdma/svc_rdma_sendto.c +7 -1
-
+  sge[sge_no].addr =3D
+   ib_dma_map_single(xprt->sc_cm_id->device,
+       (void *)
+       xdr_sge[xdr_sge_no].addr + sge_off,
+       sge_bytes, DMA_TO_DEVICE);
+  if (dma_mapping_error(sge[sge_no].addr))
+   return -EINVAL;

Quoted 3 lines

Quoted 3 lines
=20
And then here we're casting the u64 back to a void *. Also, we're adding sge_off to the input, instead of to the result. Is it true t=

hat

Quoted 12 lines

Quoted 12 lines
that
=20
ib_dma_map_single(., x + sge_off, ., .)
=20
and
=20
ib_dma_map_single(., x, ., .) + sge_off
=20
always have the same result?
=20
--b.
=20

Quoted 7 lines

Quoted 7 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xp=
Quoted 7 lines
sge_off =3D 0;
sge_no++;
+ ctxt->count++;
xdr_sge_no++;
bc -=3D sge_bytes;
}
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -210,11 +205,10 @@ static int send_write(struct svcxprt_rdma *xp=

rt,

Quoted 6 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -1

 struct svc_rqst *rqstp,
 /* Post It */
 atomic_inc(&rdma_stat_write);
 if (svc_rdma_send(xprt, &write_wr)) {
-  svc_rdma_put_context(ctxt, 1);
+  svc_rdma_put_context(ctxt, 0);

Quoted 8 lines

Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_reply(struct svcxprt_rdma *rdm=
Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -1
 /* Fatal error, close transport */
 ret =3D -EIO;
 }
- svc_rdma_put_context(tmp_sge_ctxt, 0);
 return ret;
 }
=20
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -417,6 +411,11 @@ static int send_reply(struct svcxprt_rdma *rdm=

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -0
 sge_bytes =3D min((size_t)ctxt->sge[sge_no].length,
 (size_t)byte_count);
 byte_count -=3D sge_bytes;
+  ctxt->sge[sge_no].addr =3D

Quoted 8 lines

Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_reply(struct svcxprt_rdma *rdma=
Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +4 -0
+   ib_dma_map_single(rdma->sc_cm_id->device,
+       (void *)
+       ctxt->sge[sge_no].addr,
+       sge_bytes, DMA_TO_DEVICE);
 }
 BUG_ON(byte_count !=3D 0);
=20
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
@@ -428,8 +427,9 @@ static int send_reply(struct svcxprt_rdma *rdma=

Quoted 8 lines

net/sunrpc/xprtrdma/svc_rdma_sendto.c +2 -1

 ctxt->pages[page_no+1] =3D rqstp->rq_respages[page_no];
 ctxt->count++;
 rqstp->rq_respages[page_no] =3D NULL;
+  if (page_no+1 >=3D sge_no)
+   ctxt->sge[page_no+1].length =3D 0;
 }
-
 BUG_ON(sge_no > rdma->sc_max_sge);

Quoted 8 lines

Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c static void sq_cq_reap(struct svcxprt_rdma *x=
Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0 -0
 memset(&send_wr, 0, sizeof send_wr);
 ctxt->wr_op =3D IB_WR_SEND;
net/sunrpc/xprtrdma/svc_rdma_transport.c +0 -0
index e132509..9e9e244 100644
@@ -361,10 +361,13 @@ static void sq_cq_reap(struct svcxprt_rdma *x=

prt)

Quoted 14 lines

Quoted 14 lines
Quoted 14 lines
=20
switch (ctxt->wr_op) {
case IB_WR_SEND:
- case IB_WR_RDMA_WRITE:
svc_rdma_put_context(ctxt, 1);
break;
=20
net/sunrpc/xprtrdma/svc_rdma_transport.c +4 -0
+  case IB_WR_RDMA_WRITE:
+   svc_rdma_put_context(ctxt, 0);
+   break;
+
case IB_WR_RDMA_READ:
if (test_bit(RDMACTXT_F_LAST_CTXT, &ctxt->flags)) { struct svc_rdma_op_ctxt *read_hdr =3D ctxt->read_hdr;

Quoted 2 lines

Quoted 2 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs"=

Quoted 2 lines

Quoted 2 lines
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

J. Bruce Fields

author

2008-05-25 19:05 UTC #18 lore raw

Re: [PATCH 02/05] svcrdma: Refactor RDMA_WRITE dma mapping logic

In reply to J. Bruce Fields

I'm also still curious about this:

Quoted 32 lines · bfields

Quoted 7 lines · Tom Tucker
(quoted diff) +0 -1
@@ -181,14 +170,20 @@ static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp,
 
 	/* Copy the remaining SGE */
 	while (bc != 0 && xdr_sge_no < sge_count) {
-		sge[sge_no].addr = xdr_sge[xdr_sge_no].addr + sge_off;
 		sge[sge_no].lkey = xdr_sge[xdr_sge_no].lkey;
 		sge_bytes = min((size_t)bc,
Review static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp, · line 183
Quoted 10 lines
(quoted diff) +7 -1
 				(size_t)(xdr_sge[xdr_sge_no].length-sge_off));
 		sge[sge_no].length = sge_bytes;
-
+		sge[sge_no].addr =
+			ib_dma_map_single(xprt->sc_cm_id->device,
+					  (void *)
+					  xdr_sge[xdr_sge_no].addr + sge_off,
+					  sge_bytes, DMA_TO_DEVICE);
+		if (dma_mapping_error(sge[sge_no].addr))
+			return -EINVAL;
And then here we're casting the u64 back to a void *. Also, we're adding sge_off to the input, instead of to the result. Is it true that that
	ib_dma_map_single(., x + sge_off, ., .)
and
	ib_dma_map_single(., x, ., .) + sge_off
always have the same result?
--b.