From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Gunthorpe Subject: Re: [PATCH libibverbs V3 2/3] Use neighbour lookup for RoCE UD QPs Eth L2 resolution Date: Thu, 8 May 2014 13:29:07 -0600 Message-ID: <20140508192907.GC25849@obsidianresearch.com> References: <1399531883-30599-1-git-send-email-ogerlitz@mellanox.com> <1399531883-30599-3-git-send-email-ogerlitz@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1399531883-30599-3-git-send-email-ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> Sender: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Or Gerlitz Cc: roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org, yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org, dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org List-Id: linux-rdma@vger.kernel.org On Thu, May 08, 2014 at 09:51:22AM +0300, Or Gerlitz wrote: > In order to implement RoCE IP based addressing for UD QPs, without > introducing uverbs changes, we need a way to resolve the L2 Ethernet > addresses from user-space. Please put anything that changes the public verbs ABI in a seperate patch. Burying the create_ah_ex stuff in this giant thing is not condusive to reviewing the ABI portion. The ABI is *important* do everything you can to bring as many eyes as possible to those changes!! > +struct ibv_ah_attr_ex { > + struct ibv_global_route grh; > + uint16_t dlid; > + uint8_t sl; > + uint8_t src_path_bits; > + uint8_t static_rate; > + uint8_t is_global; > + uint8_t port_num; > + uint32_t comp_mask; > + struct { > + enum ll_address_type type; > + uint32_t len; > + char *address; > + } ll_address; > + uint16_t vid; > +}; The above looks sorta reasonable, but: What is the 'char *address'? Should it be 'const void *' ????? Who owns that memory? Who allocates it, who frees it? Seems sketchy as heck. What happens if a ibv_ah_attr_ex is returned in some kind of ibv_qp_attr_ex structure from a future query_qp? Can you just put in a 'uint64_t address[8];' or sockaddr_storage, or something? > + > enum ibv_srq_attr_mask { > IBV_SRQ_MAX_WR = 1 << 0, > IBV_SRQ_LIMIT = 1 << 1 > @@ -968,11 +998,14 @@ enum verbs_context_mask { > VERBS_CONTEXT_QP = 1 << 2, > VERBS_CONTEXT_CREATE_FLOW = 1 << 3, > VERBS_CONTEXT_DESTROY_FLOW = 1 << 4, > - VERBS_CONTEXT_RESERVED = 1 << 5 > + VERBS_CONTEXT_CREATE_AH = 1 << 5, > + VERBS_CONTEXT_RESERVED = 1 << 6 > }; What is going on here?? verbs_context_mask is messed up already!? It is NOT supposed to be a mask if the function is available - we can already test for NULL for that. It is supposed to indicate if structures *allocated by the provider* are extended. For instance if VERBS_CONTEXT_QP is set, then everyone knows that the 'struct ibv_qp' returned by *any* driver function (crate_qp, open_qp, create_qp_ex) is convertible to 'verbs_qp'. VERBS_CONTEXT_CREATE_FLOW/VERBS_CONTEXT_DESTROY_FLOW are screwed up. They shouldn't even exist, there is no unextended ibv_flow structure to worry about. Fix this patch to not add VERBS_CONTEXT_CREATE_AH, ibv_ah_attr_ex is not a provider allocated structure so it doesn't need an entry. Please send a patch to remove the screwed up two above and replace with RESERVED. And BE MORE CAREFUL. THIS SHOULD NOT HAVE HAPPENED! Jason -- 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