Re: [PATCH v1 00/24] New fast registration API

[santosh shilimkar <santosh.shilimkar-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>]

On 9/22/2015 12:56 AM, Sagi Grimberg wrote:
> On 9/22/2015 10:19 AM, Sagi Grimberg wrote:
>>>
>>> As mentioned earlier, I have a WIP RDS fastreg branch [3]
>>> which is functional (at least I can RDMA messages across
>>> nodes ;-)).
>>
>> Nice!
>>
>>> So merging [2] and [3], I created [4] and applied
>>> a delta change based on your other patches. I saw ib_post_send
>>> failure with my HCA driver returning '-EINVAL'. I didn't
>>> debug it further but at least opcode and num_sge were set
>>> correctly so I shouldn't have seen it. So I did memset()
>>> on reg_wr which seems to have helped to fix the ib_post_send()
>>> failure.
>>
>> Yep - that was my fault. When converting the ULPs I optimized by
>> removing the memset but I forgot to set reg_wr.wr.next = NULL when
>> the ULP needed. This caused the driver to read a second bogus work
>> request. Steve just reported this as well so I'll fix that in v2.
>>
Ahh, right. There can be chain of wr.

>>>
>>> But I got into remote access errors which tells me that I
>>> have messed up setup(rkey, sge setup or access flags)
>>
>> One thing that pops is that in the old API the MR was registered
>> with iova_start = 0 (which is probably what was sent to the peer),
>> but the new API the iova is implicitly sg_dma_address(&sg[0]).
>>
>> The registered MR holds these attributes in:
>> mr->rkey
>> mr->iova
>> mr->length
>>
>> These should be passed to a peer to perform rdma.
>>
right.

> ohh, I just read the RDS 3.1 specification (for the first time..) and I
> noticed that RDS 3.1 header extension contains only a 32bit offset
> parameter. Why is that anyway? why not 64bit so it can be a valid mapped
> address? Also the code doesn't use it at all and always passes 0 (which
> is buggy if sg[0] has an offset from a page).
>
> This won't work with the proposed API as the iova is 64bit (as all other
> existing RDMA protocols use 64bit addresses).
>
> In any event, I'd much rather to add ib_map_mr_sg_zbva() just for RDS
> to use instead of polluting the API with an iova argument, but I think
> that the RDS spec can be updated to use 64bit offsets and align to all
> other RDMA protocols (it has enough space in h_exthdr which is 128bit).
>
RDS assumes it's an offset and hence it has been used as 32 bit. I need
to look through this carefully though because all the existing
application use this header format. There is also RDMA read/write
byte information sent as part of the header(Not in upstream code yet)
so the space might be less. But point taken. Will look into it.

> I was thinking of:
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index e7e0251..61fcab4 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -3033,6 +3033,21 @@ int ib_map_mr_sg(struct ib_mr *mr,
>                   unsigned int sg_nents,
>                   unsigned int page_size);
>
> +static inline int
> +ib_map_mr_sg_zbva(struct ib_mr *mr,
> +                 struct scatterlist *sg,
> +                 unsigned int sg_nents,
> +                 unsigned int page_size)
> +{
> +       int rc;
> +
> +       rc = ib_map_mr_sg(mr, sg, sg_nents, page_size);
> +       if (likely(!rc))
> +               mr->iova &= ((u64)page_size - 1);
> +
> +       return rc;
> +}
> +
>   int ib_sg_to_pages(struct ib_mr *mr,
>                     struct scatterlist *sgl,
>                     unsigned int sg_nents,
> --
>
> Thoughts?
>
> Santosh, can you use that one instead and let us know if
> it resolves your issue?
>
Unfortunately this change still doesn't fix the issue.

> I think you should make sure to correctly construct the
> h_exthdr with: rds_rdma_make_cookie(mr->rkey, (32)mr->iova)

Will look into it. Thanks for suggestion.

Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html