From: Tom Tucker Subject: Re: [PATCH 02/05] svcrdma: Refactor RDMA_WRITE dma mapping logic Date: Wed, 21 May 2008 05:33:40 -0500 Message-ID: References: <20080521025216.GA19820@fieldses.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Cc: To: "J. Bruce Fields" Return-path: Received: from mail.es335.com ([67.65.19.105]:8596 "EHLO mail.es335.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755094AbYEUKdl convert rfc822-to-8bit (ORCPT ); Wed, 21 May 2008 06:33:41 -0400 In-Reply-To: <20080521025216.GA19820@fieldses.org> Sender: linux-nfs-owner@vger.kernel.org List-ID: On 5/20/08 9:52 PM, "J. Bruce Fields" wrote: > On Tue, May 20, 2008 at 08:46:08PM -0400, J. Bruce Fields wrote: >> On Sun, May 18, 2008 at 07:13:18PM -0500, Tom Tucker wrote: >>> In order to make the dma mapping code easier to understand and redu= ce the >>> number of I/O contexts required, move the DMA for RDMA_WRITE WR to = the >>> 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 >> =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 >> =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 > 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 > 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 > --b. >=20 >> And the types do seem weird: >>=20 >>>=20 >>> Signed-off-by: Tom Tucker >>>=20 >>> --- >>> net/sunrpc/xprtrdma/svc_rdma_sendto.c | 44 >>> +++++++++++++++--------------- >>> net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 +++- >>> 2 files changed, 26 insertions(+), 23 deletions(-) >>>=20 >>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c >>> b/net/sunrpc/xprtrdma/svc_rdma_sendto.c >>> index fb82b1b..85931c4 100644 >>> --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c >>> +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c >>> @@ -84,10 +84,7 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_= rdma >>> *xprt, >>> sge_no =3D 1; >>> =20 >>> /* 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; n= ow >> we're storing a void* directly into a u64. >>=20 >>> 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, >>> 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 svcxpr= t_rdma >>> *xprt, >>> =20 >>> /* 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, >>> 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, >>> write_len, xdr_sge, sge_count); >>> =20 >>> 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 >>> /* Find the SGE associated with xdr_off */ >>> for (bc =3D xdr_off, xdr_sge_no =3D 1; bc && xdr_sge_no < sge_count= ; >>> @@ -181,14 +170,20 @@ static int send_write(struct svcxprt_rdma *xp= rt, >>> 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; >>> - >>> + 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; >>=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 >> 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 >>> 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, >>> /* 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 >>> @@ -417,6 +411,11 @@ static int send_reply(struct svcxprt_rdma *rdm= a, >>> 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 >>> @@ -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; >>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c >>> b/net/sunrpc/xprtrdma/svc_rdma_transport.c >>> index e132509..9e9e244 100644 >>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c >>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c >>> @@ -361,10 +361,13 @@ static void sq_cq_reap(struct svcxprt_rdma *x= prt) >>> =20 >>> switch (ctxt->wr_op) { >>> case IB_WR_SEND: >>> - case IB_WR_RDMA_WRITE: >>> svc_rdma_put_context(ctxt, 1); >>> break; >>> =20 >>> + 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; >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-nfs"= in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html