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

[Tom Tucker <tom@opengridcomputing.com>]

On 5/20/08 9:52 PM, "J. Bruce Fields" <bfields@fieldses.org> 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 <tom@opengridcomputing.com>
>>>=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