J. Bruce Fields wrote:
On Sat, Jun 21, 2008 at 11:26:52AM -0500, Tom Tucker wrote:
J. Bruce Fields wrote:
Also, the only caller of xdr_to_sge() appears to be svc_rdma_sendto(),
which is called from svc_sendto() immediately after setting:
xb = &rqstp->rq_res;
xb->len = xb->head[0].iov_len + xb->page_len +
xb->tail[0].iov_len;
So I think xdr_to_sge() is doing a bunch of unnecessary work to handle
the case where xdr->len could be less than that sum?
Ok... check me below please.
/* 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);
+ vec->sge[sge_no].iov_base = xdr->head[0].iov_base;
sge_bytes = min_t(u32, byte_count, xdr->head[0].iov_len);
This doesn't need to be min_t. It could be xdr->head[0].iov_len.
Yes. The variable byte_count shouldn't be needed at all, so I'd start
by eliminating all references to byte_count.
byte_count -= sge_bytes;
- sge[sge_no].length = sge_bytes;
- sge[sge_no].lkey = xprt->sc_phys_mr->lkey;
+ vec->sge[sge_no].iov_len = sge_bytes;
sge_no++;
/* pages SGE */
@@ -99,16 +94,13 @@ 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) {
+ vec->sge[sge_no].iov_base =
+ page_address(xdr->pages[page_no]) + page_off;
sge_bytes = min_t(u32, byte_count, (PAGE_SIZE-page_off));
This is still needed if the xdr terminates with a portion of a page and
there is not tail.
Yes, but the condition could just be (page_bytes).
Right.
- 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;
- sge[sge_no].length = sge_bytes;
- sge[sge_no].lkey = xprt->sc_phys_mr->lkey;
+ vec->sge[sge_no].iov_len = sge_bytes;
sge_no++;
page_no++;
@@ -117,23 +109,17 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt,
/* Tail SGE */
if (byte_count && xdr->tail[0].iov_len) {
This is defensive. It could just be byte_count.
I agree, though I'd just make it (xdr->tail[0].iov_len) instead of just
(byte_count).
Agreed.
If byte_count != xdr->tail[0].iov_len, the BUG_ON below will let us know.
- 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);
+ vec->sge[sge_no].iov_base = xdr->tail[0].iov_base;
sge_bytes = min_t(u32, byte_count, xdr->tail[0].iov_len);
The min_t isn't needed. It could just be byte_count.
Right, but could also just be tail length, as above.
Ok, page # confirmed...
--b.
--
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
