| Message ID | 1400405929-14313-3-git-send-email-ogerlitz@r-vnc04.mtr.labs.mlnx (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Sun, May 18, 2014 at 12:38:46PM +0300, Or Gerlitz wrote: > From: Matan Barak <matanb@mellanox.com> > > In order to support IP based addressing, one needs to pass the L2 > parameters to the provider drive. This is done through a new extendable ^^^^ 'driver' Please provide a man page. In this case you probably want to provide a patch to the existing man/ibv_create_ah.3 to discuss the 2nd entry point and new fields. > +enum ibv_ah_attr_ex_attr_mask { > + IBV_AH_ATTR_EX_LL = 1UL << 0, > + IBV_AH_ATTR_EX_VID = 1UL << 1, > +}; OK > +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; OK > + union { > + struct sockaddr sa; > + struct sockaddr_storage _s; > + } ll; > + uint16_t vid; > +}; Hard to know exactly what is going on here without a man page, but I thought one of the points was to provide an ethernet L2 MAC address? Shouldn't there be a mechanism for that? I see this: + attr_ex.ll.sa.sa_family = ARPHRD_ETHER; Which makes no sense, sa_family should be an AF_* constant. So, I think this bit needs some kind of work. If you want to setup ethernet, then setup ethernet: uint64_t eth_dmac uint16_t eth_vid; and what about all the trendy new ethernet stuff, overlay networks, etc? Can that be expressed in there? > @@ -975,6 +998,8 @@ enum verbs_context_mask { > > struct verbs_context { > /* "grows up" - new fields go here */ > + struct ibv_ah * (*create_ah_ex)(struct ibv_pd *pd, > + struct ibv_ah_attr_ex *attr); Can you check if 'attr' should be const? I see the existing API isn't const, but I am suspecting that is a mistake? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/5/2014 10:55 PM, Jason Gunthorpe wrote: > On Sun, May 18, 2014 at 12:38:46PM +0300, Or Gerlitz wrote: >> From: Matan Barak <matanb@mellanox.com> >> >> In order to support IP based addressing, one needs to pass the L2 >> parameters to the provider drive. This is done through a new extendable > ^^^^ > 'driver' > > Please provide a man page. In this case you probably want to provide a > patch to the existing man/ibv_create_ah.3 to discuss the 2nd entry > point and new fields. > Ok, I'll add this entry to the ibv_create_ah man page. >> +enum ibv_ah_attr_ex_attr_mask { >> + IBV_AH_ATTR_EX_LL = 1UL << 0, >> + IBV_AH_ATTR_EX_VID = 1UL << 1, >> +}; > > OK > >> +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; > > OK > >> + union { >> + struct sockaddr sa; >> + struct sockaddr_storage _s; >> + } ll; >> + uint16_t vid; >> +}; > > Hard to know exactly what is going on here without a man page, but I > thought one of the points was to provide an ethernet L2 MAC address? > Shouldn't there be a mechanism for that? > > I see this: > > + attr_ex.ll.sa.sa_family = ARPHRD_ETHER; > > Which makes no sense, sa_family should be an AF_* constant. > Kernel code *sometimes* use sa_family as ARPHRD_ETHER. > So, I think this bit needs some kind of work. If you want to setup > ethernet, then setup ethernet: > > uint64_t eth_dmac > uint16_t eth_vid; > > and what about all the trendy new ethernet stuff, overlay networks, > etc? Can that be expressed in there? > I don't want to make this Ethernet specific. That's why the previous pointer used a pointer. I don't mind creating a generic interface for link layer if the existing solutions aren't good enough. Any suggestions? >> @@ -975,6 +998,8 @@ enum verbs_context_mask { >> >> struct verbs_context { >> /* "grows up" - new fields go here */ >> + struct ibv_ah * (*create_ah_ex)(struct ibv_pd *pd, >> + struct ibv_ah_attr_ex *attr); > > Can you check if 'attr' should be const? I see the existing API isn't > const, but I am suspecting that is a mistake? You're probably right, but wouldn't we want to be aligned with the non-extended verb: struct ibv_ah * (*create_ah)(struct ibv_pd *pd, struct ibv_ah_attr *attr); Since the provider driver usually go through a common path for both the non-extended and the extended verb, this could cause an ugly const cast. > > Jason > Matan -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/05/2014 11:52, Matan Barak wrote: > On 21/5/2014 10:55 PM, Jason Gunthorpe wrote: >> On Sun, May 18, 2014 at 12:38:46PM +0300, Or Gerlitz wrote: >>> From: Matan Barak <matanb@mellanox.com> >>> >>> In order to support IP based addressing, one needs to pass the L2 >>> parameters to the provider drive. This is done through a new extendable >> ^^^^ >> 'driver' >> >> Please provide a man page. In this case you probably want to provide a >> patch to the existing man/ibv_create_ah.3 to discuss the 2nd entry >> point and new fields. >> > > Ok, I'll add this entry to the ibv_create_ah man page. > >>> +enum ibv_ah_attr_ex_attr_mask { >>> + IBV_AH_ATTR_EX_LL = 1UL << 0, >>> + IBV_AH_ATTR_EX_VID = 1UL << 1, >>> +}; >> >> OK >> >>> +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; >> >> OK >> >>> + union { >>> + struct sockaddr sa; >>> + struct sockaddr_storage _s; >>> + } ll; >>> + uint16_t vid; >>> +}; >> >> Hard to know exactly what is going on here without a man page, but I >> thought one of the points was to provide an ethernet L2 MAC address? >> Shouldn't there be a mechanism for that? >> >> I see this: >> >> + attr_ex.ll.sa.sa_family = ARPHRD_ETHER; >> >> Which makes no sense, sa_family should be an AF_* constant. >> > > Kernel code *sometimes* use sa_family as ARPHRD_ETHER. > >> So, I think this bit needs some kind of work. If you want to setup >> ethernet, then setup ethernet: >> >> uint64_t eth_dmac >> uint16_t eth_vid; >> >> and what about all the trendy new ethernet stuff, overlay networks, >> etc? Can that be expressed in there? >> > > I don't want to make this Ethernet specific. That's why the previous > pointer used a pointer. I don't mind creating a generic interface for > link layer if the existing solutions aren't good enough. Any suggestions? Hi Jason, Can you please address Matan's comments? we'd like this thread to converge... Or. > >>> @@ -975,6 +998,8 @@ enum verbs_context_mask { >>> >>> struct verbs_context { >>> /* "grows up" - new fields go here */ >>> + struct ibv_ah * (*create_ah_ex)(struct ibv_pd *pd, >>> + struct ibv_ah_attr_ex *attr); >> >> Can you check if 'attr' should be const? I see the existing API isn't >> const, but I am suspecting that is a mistake? > > You're probably right, but wouldn't we want to be aligned with the > non-extended verb: > struct ibv_ah * (*create_ah)(struct ibv_pd *pd, struct > ibv_ah_attr *attr); > Since the provider driver usually go through a common path for both > the non-extended and the extended verb, this could cause an ugly const > cast. > >> >> Jason >> > > Matan > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 11, 2014 at 01:54:56PM +0300, Or Gerlitz wrote: > Can you please address Matan's comments? we'd like this thread to > converge... I am waiting for your analysis as I mentioned in my last message to this thread: https://www.mail-archive.com/linux-rdma@vger.kernel.org/msg19925.html That is rather a larger question, and the answer may well be we don't need two new extended verbs to do what you want... Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h index 71adf2a..ab497b1 100644 --- a/include/infiniband/verbs.h +++ b/include/infiniband/verbs.h @@ -40,6 +40,7 @@ #include <pthread.h> #include <stddef.h> #include <errno.h> +#include <sys/socket.h> #ifdef __cplusplus # define BEGIN_C_DECLS extern "C" { @@ -467,6 +468,28 @@ struct ibv_ah_attr { uint8_t port_num; }; +enum ibv_ah_attr_ex_attr_mask { + IBV_AH_ATTR_EX_LL = 1UL << 0, + IBV_AH_ATTR_EX_VID = 1UL << 1, +}; + +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; + union { + struct sockaddr sa; + struct sockaddr_storage _s; + } ll; + uint16_t vid; +}; + + enum ibv_srq_attr_mask { IBV_SRQ_MAX_WR = 1 << 0, IBV_SRQ_LIMIT = 1 << 1 @@ -975,6 +998,8 @@ enum verbs_context_mask { struct verbs_context { /* "grows up" - new fields go here */ + struct ibv_ah * (*create_ah_ex)(struct ibv_pd *pd, + struct ibv_ah_attr_ex *attr); void (*_reserved_2)(void); int (*destroy_flow)(struct ibv_flow *flow); void (*_reserved_1)(void);