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 reduce the
> > number of I/O contexts required, move the DMA for RDMA_WRITE WR to the
> > 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 ‘send_write’:
> net/sunrpc/xprtrdma/svc_rdma_sendto.c:180: warning: passing argument 2 of ‘ib_dma_map_single’ makes pointer from integer without a cast
> net/sunrpc/xprtrdma/svc_rdma_sendto.c: In function ‘send_reply’:
> net/sunrpc/xprtrdma/svc_rdma_sendto.c:416: warning: passing argument 2 of ‘ib_dma_map_single’ makes pointer from integer without a cast
Erm, sorry, that should have been:
unrpc/xprtrdma/svc_rdma_sendto.c: In function ‘send_write’:
net/sunrpc/xprtrdma/svc_rdma_sendto.c:180: warning: cast to pointer from integer of different size
net/sunrpc/xprtrdma/svc_rdma_sendto.c: In function ‘send_reply’:
net/sunrpc/xprtrdma/svc_rdma_sendto.c:417: warning: cast to pointer from integer of different size
--b.
> And the types do seem weird:
>
> >
> > Signed-off-by: Tom Tucker <tom@xxxxxxxxxxxxxxxxxxxxx>
> >
> > ---
> > net/sunrpc/xprtrdma/svc_rdma_sendto.c | 44 +++++++++++++++---------------
> > net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 +++-
> > 2 files changed, 26 insertions(+), 23 deletions(-)
> >
> > 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 = 1;
> >
> > /* Head SGE */
> > - sge[sge_no].addr = 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 = (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.
>
> > sge_bytes = min_t(u32, byte_count, xdr->head[0].iov_len);
> > byte_count -= sge_bytes;
> > sge[sge_no].length = sge_bytes;
> > @@ -99,11 +96,9 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt,
> > page_bytes = xdr->page_len;
> > page_off = xdr->page_base;
> > while (byte_count && page_bytes) {
> > + sge[sge_no].addr = (unsigned long)
> > + page_address(xdr->pages[page_no]) + page_off;
> > sge_bytes = min_t(u32, byte_count, (PAGE_SIZE-page_off));
> > - sge[sge_no].addr =
> > - ib_dma_map_page(xprt->sc_cm_id->device,
> > - xdr->pages[page_no], page_off,
> > - sge_bytes, DMA_TO_DEVICE);
> > sge_bytes = min(sge_bytes, page_bytes);
> > byte_count -= sge_bytes;
> > page_bytes -= sge_bytes;
> > @@ -117,11 +112,7 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt,
> >
> > /* Tail SGE */
> > if (byte_count && xdr->tail[0].iov_len) {
> > - sge[sge_no].addr =
> > - 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 = (unsigned long)xdr->tail[0].iov_base;
> > sge_bytes = min_t(u32, byte_count, xdr->tail[0].iov_len);
> > byte_count -= sge_bytes;
> > sge[sge_no].length = 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);
> >
> > ctxt = svc_rdma_get_context(xprt);
> > - ctxt->count = 0;
> > - tmp_sge_ctxt = svc_rdma_get_context(xprt);
> > - sge = tmp_sge_ctxt->sge;
> > + ctxt->direction = DMA_TO_DEVICE;
> > + sge = ctxt->sge;
> >
> > /* Find the SGE associated with xdr_off */
> > for (bc = xdr_off, xdr_sge_no = 1; bc && xdr_sge_no < sge_count;
> > @@ -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,
> > (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.
>
> > sge_off = 0;
> > sge_no++;
> > + ctxt->count++;
> > xdr_sge_no++;
> > bc -= sge_bytes;
> > }
> > @@ -210,11 +205,10 @@ static int send_write(struct svcxprt_rdma *xprt, 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 = -EIO;
> > }
> > - svc_rdma_put_context(tmp_sge_ctxt, 0);
> > return ret;
> > }
> >
> > @@ -417,6 +411,11 @@ 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;
> > + ctxt->sge[sge_no].addr =
> > + ib_dma_map_single(rdma->sc_cm_id->device,
> > + (void *)
> > + ctxt->sge[sge_no].addr,
> > + sge_bytes, DMA_TO_DEVICE);
> > }
> > BUG_ON(byte_count != 0);
> >
> > @@ -428,8 +427,9 @@ static int send_reply(struct svcxprt_rdma *rdma,
> > ctxt->pages[page_no+1] = rqstp->rq_respages[page_no];
> > ctxt->count++;
> > rqstp->rq_respages[page_no] = NULL;
> > + if (page_no+1 >= sge_no)
> > + ctxt->sge[page_no+1].length = 0;
> > }
> > -
> > BUG_ON(sge_no > rdma->sc_max_sge);
> > memset(&send_wr, 0, sizeof send_wr);
> > ctxt->wr_op = 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 *xprt)
> >
> > switch (ctxt->wr_op) {
> > case IB_WR_SEND:
> > - case IB_WR_RDMA_WRITE:
> > svc_rdma_put_context(ctxt, 1);
> > break;
> >
> > + 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 = ctxt->read_hdr;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
