On Jul 22, 2015, at 11:41 AM, Sagi Grimberg <sa...@dev.mellanox.co.il> wrote:
>
>>> + for (i = 0; i < nsegs;) {
>>> + sg_set_page(&frmr->sg[i], seg->mr_page,
>>> + seg->mr_len, offset_in_page(seg->mr_offset));
>>
>> Cautionary note: here we’re dealing with both the “contiguous
>> set of pages” case and the “small region of bytes in a single page”
>> case. See rpcrdma_convert_iovs(): sometimes RPC send or receive
>> buffers can be registered (RDMA_NOMSG).
>
> I noticed that (I think). I think this is handled correctly.
> What exactly is the caution note here?
Well the sg is turned into a page list below your API. Just
want to make sure that we have tested your xprtrdma alterations
with all the ULP possibilities. When you are further along I
can pull this and run my functional tests.
>>> mr = frmr->fr_mr;
>>> + access = writing ? IB_ACCESS_REMOTE_WRITE | IB_ACCESS_LOCAL_WRITE :
>>> + IB_ACCESS_REMOTE_READ;
>>> + rc = ib_map_mr_sg(mr, frmr->sg, frmr->sg_nents, access);
>>
>> I like this (and the matching ib_dma_unmap_sg). But why wouldn’t
>> this function be called ib_dma_map_sg() ? The name ib_map_mr_sg()
>> had me thinking for a moment that this API actually posted the
>> FASTREG WR, but I see that it doesn’t.
>
> Umm, ib_dma_map_sg is already taken :)
>
> This is what I came up with, it maps the SG elements to the MR
> private context.
>
> I'd like to keep the post API for now. It will be possible to
> to add a wrapper function that would do:
> - dma_map_sg
> - ib_map_mr_sg
> - init fastreg send_wr
> - post_send (maybe)
Where xprtrdma might improve is by setting up all the FASTREG
WRs for one RPC with a single chain and post_send. We could do
that with your INDIR_MR concept, for example.
>>> - while (seg1->mr_nsegs--)
>>> - rpcrdma_unmap_one(ia->ri_device, seg++);
>>> + ib_dma_unmap_sg(ia->ri_device, frmr->sg, frmr->sg_nents, seg1->mr_dir);
>>
>> ->mr_dir was previously set by rpcrdma_map_one(), which you’ve replaced
>> with ib_map_mr_sg(). So maybe frwr_op_map() needs to save “direction”
>> in the rpcrdma_frmr.
>
> Yep, that's correct, if I had turned on dma mapping debug it would shout
> at me here...
>
> Note, I added in the git repo a patch to allow arbitrary sg lists in
> frwr_op_map() which would allow you to skip the holes check... seems to
> work with mlx5...
>
> I did noticed the mlx4 gives a protection error with after the conversion...
> I'll look into that...
Should also get Steve and Devesh to try this with their adapters.
--
Chuck Lever
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
