On Thu, Jun 13, 2013 at 8:09 PM, Jason Gunthorpe
<jguntho...@obsidianresearch.com> wrote:
> On Thu, Jun 13, 2013 at 06:01:44PM +0300, Or Gerlitz wrote:
>> From: Matan Barak <mat...@mellanox.com>
>>
>> Add support for RoCE (IBoE) IP based addressing extensions towards
>> user space.
>>
>> Extend INIT_QP_ATTR and QUERY_ROUTE ucma commands.
>>
>> Extend MODIFY_QP and CREATE_AH uverbs commands.
>
> This is a really big patch Or, there is lots going on here, hard to
> review :(
>
> The rdma cm stuff should probably be split out of this, and Sean
> should look at it of course.
sure, will do that, one patch for uverbs and one patch for rdma_ucm
> In fact, since the user ABI is so important, every ABI change should
> be a distinct patch, with a good change log, stating the intended
> goals of the change and ABI visible changes it makes.
point taken, will do that, thanks for bringing this over.
>
> The changelog above is terrible for a huge patch that makes changes to
> the userspace API.
>
>> diff --git a/include/uapi/rdma/ib_user_sa.h b/include/uapi/rdma/ib_user_sa.h
>> index cfc7c9b..367d66a 100644
>> +++ b/include/uapi/rdma/ib_user_sa.h
>> @@ -48,7 +48,13 @@ enum {
>> struct ib_path_rec_data {
>> __u32 flags;
>> __u32 reserved;
>> - __u32 path_rec[16];
>> + __u32 path_rec[20];
>> +};
>> +
>> +enum ibv_kern_path_rec_attr_mask {
>> + IB_USER_PATH_REC_ATTR_DMAC = 1ULL << 0,
>> + IB_USER_PATH_REC_ATTR_SMAC = 1ULL << 1,
>> + IB_USER_PATH_REC_ATTR_VID = 1ULL << 2
>> };
>
> So, how is userspace supposed to know what these values are?
Its part of the verbs extensions deal.
> The current system where the MAC address is in the GID seemed
> understandable, assuming you discover the MAC out of band some how...
MAC is Ethernet layer 2 address, I don't see why put mac in L3 header
(GRH) its better understandable vs putting there L3 address (IP).
>
>> +struct ib_uverbs_modify_qp_ex {
>> + __u32 comp_mask;
>> + struct ib_uverbs_qp_dest dest;
>> + struct ib_uverbs_qp_dest alt_dest;
> [...]
>> + struct ib_uverbs_qp_dest_ex dest_ex;
>> + struct ib_uverbs_qp_dest_ex alt_dest_ex;
>
> Yuk.. The 'ex' structures don't have to be byte compatible, they just
> have to have a known transform, dest should be the full extended dest,
> not split into two..
>
>> +struct rdma_ucm_query_route_resp_ex {
>> + __u64 node_guid;
>> + struct ib_user_path_rec_ex ib_route[2];
>> + struct sockaddr_in6 src_addr;
>> + struct sockaddr_in6 dst_addr;
>> + __u32 num_paths;
>> + __u8 port_num;
>> + __u8 reserved[3];
>> +};
>
> Should these be sockaddr_storage? How does this intersect with Sean's AF_GID
> work?
sockaddr_in6 is OK for extending rdma_ucm_query_route_resp as its OK
for the non extended version of that command. I don't see any
intersection with the AF_IB work.
--
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
