On 5/20/08 9:52 PM, "J. Bruce Fields" <bfields@xxxxxxxxxxxx> 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 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
>
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_addr_t.
I need my own type there I guess.
Tom
> --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
