| Message ID | 1399531883-30599-4-git-send-email-ogerlitz@mellanox.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Thu, May 08, 2014 at 09:51:23AM +0300, Or Gerlitz wrote: > +struct ibv_port_attr_ex { > + union { > + struct { > + enum ibv_port_state state; > + enum ibv_mtu max_mtu; > + enum ibv_mtu active_mtu; > + int gid_tbl_len; > + uint32_t port_cap_flags; > + uint32_t max_msg_sz; > + uint32_t bad_pkey_cntr; > + uint32_t qkey_viol_cntr; > + uint16_t pkey_tbl_len; > + uint16_t lid; > + uint16_t sm_lid; > + uint8_t lmc; > + uint8_t max_vl_num; > + uint8_t sm_sl; > + uint8_t subnet_timeout; > + uint8_t init_type_reply; > + uint8_t active_width; > + uint8_t active_speed; > + uint8_t phys_state; > + uint8_t link_layer; > + uint8_t reserved; > + }; > + struct ibv_port_attr port_attr; > + }; > + uint32_t comp_mask; > + uint32_t mask1; > +}; I really don't like this deviation from the standard _ex pattern. Anonymous struct/union is a C11 feature and GNU extension. I don't think is appropriate for a user library to rely on. We cannot assume the user is has a compiler in C11 mode. The cannonical layout should be: struct ibv_port_attr_ex { uint64_t comp_mask; struct ibv_port_attr port_attr; // New stuff goes here } It is fine to use comp_mask to indicate all the fields, no need for the weird mask1, lets not over complicate things for users. > struct verbs_context { > /* "grows up" - new fields go here */ > + int (*drv_query_port_ex)(struct ibv_context *context, uint8_t port_num, > + struct ibv_port_attr_ex *port_attr); > + int (*lib_query_port_ex)(struct ibv_context *context, uint8_t port_num, > + struct ibv_port_attr_ex *port_attr); > struct ibv_ah * (*drv_ibv_create_ah_ex)(struct ibv_pd *pd, > struct ibv_ah_attr_ex *attr); > int (*drv_ibv_destroy_flow) (struct ibv_flow *flow); I'm not sure what Roland decided to merge, but I am surprised to see drv_ elements here. That was nix'd in the round of review of the first patch set. eg create_qp_ex/etc don't have them. The flow is such that the verbs code has a chance to capture and override the function pointer after the driver sets it, there is no purpose to the drv_ values. I wouldn't like to see more added, and I think you should make a patch to ensure they are not necessary before it propogates too far. > diff --git a/src/verbs.c b/src/verbs.c > index e7343a9..f8245b0 100644 > +++ b/src/verbs.c > @@ -550,7 +550,7 @@ struct ibv_ah *__ibv_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr) > int err; > struct ibv_ah *ah = NULL; > #ifndef NRESOLVE_NEIGH > - struct ibv_port_attr port_attr; > + struct ibv_port_attr_ex port_attr; > int dst_family; > int src_family; > int oif; > @@ -570,7 +570,10 @@ struct ibv_ah *__ibv_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr) > goto return_ah; > } > > - err = ibv_query_port(pd->context, attr->port_num, &port_attr); > + port_attr.comp_mask = IBV_QUERY_PORT_EX_ATTR_MASK1; > + port_attr.mask1 = IBV_QUERY_PORT_EX_LINK_LAYER | > + IBV_QUERY_PORT_EX_CAP_FLAGS; > + err = ibv_query_port_ex(pd->context, attr->port_num, &port_attr); > > if (err) { > fprintf(stderr, PFX "ibv_create_ah failed to query port.\n"); I wonder if this belongs in a seperate patch, I had to read it twice to realize this change was to take advantage of the reduced query performance optimization. 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 Thu, 8 May 2014, Jason Gunthorpe wrote: > Anonymous struct/union is a C11 feature and GNU extension. I don't > think is appropriate for a user library to rely on. We cannot assume > the user is has a compiler in C11 mode. Anonymous structs/union are used in the kernel already. See include/linux/mm_types.h f.e. -- 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 Thu, May 08, 2014 at 02:40:44PM -0500, Christoph Lameter wrote: > On Thu, 8 May 2014, Jason Gunthorpe wrote: > > > Anonymous struct/union is a C11 feature and GNU extension. I don't > > think is appropriate for a user library to rely on. We cannot assume > > the user is has a compiler in C11 mode. > > Anonymous structs/union are used in the kernel already. See > include/linux/mm_types.h f.e. Re-read: This is a userspace header. mm_types.h is never included by a userspace program, so it doesn't matter. Find something in include/uapi and we'll talk :) 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 Thu, May 8, 2014 at 12:09 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: >> struct verbs_context { >> /* "grows up" - new fields go here */ >> + int (*drv_query_port_ex)(struct ibv_context *context, uint8_t port_num, >> + struct ibv_port_attr_ex *port_attr); >> + int (*lib_query_port_ex)(struct ibv_context *context, uint8_t port_num, >> + struct ibv_port_attr_ex *port_attr); >> struct ibv_ah * (*drv_ibv_create_ah_ex)(struct ibv_pd *pd, >> struct ibv_ah_attr_ex *attr); >> int (*drv_ibv_destroy_flow) (struct ibv_flow *flow); > > I'm not sure what Roland decided to merge, but I am surprised to see > drv_ elements here. That was nix'd in the round of review of the first > patch set. eg create_qp_ex/etc don't have them. > > The flow is such that the verbs code has a chance to capture and > override the function pointer after the driver sets it, there is no > purpose to the drv_ values. > > I wouldn't like to see more added, and I think you should make a patch > to ensure they are not necessary before it propogates too far. Sorry, I was not aware of the feeling on drv_XXX members here and I don't think I saw any review comments about them. (Maybe they got lost in the flood of "merge it merge it merge it" emails) Anyway I'm wondering if there's a way to recover without breaking ABI here (or by breaking ABI in a manageable way). The only library using this stuff (that I know of) is libmlx4, maybe we can spin quick releases of both and pretend libibverbs 1.1.8 never happened? -- 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 Fri, May 09, 2014 at 07:01:36AM -0700, Roland Dreier wrote: > On Thu, May 8, 2014 at 12:09 PM, Jason Gunthorpe > <jgunthorpe@obsidianresearch.com> wrote: > >> struct verbs_context { > >> /* "grows up" - new fields go here */ > >> + int (*drv_query_port_ex)(struct ibv_context *context, uint8_t port_num, > >> + struct ibv_port_attr_ex *port_attr); > >> + int (*lib_query_port_ex)(struct ibv_context *context, uint8_t port_num, > >> + struct ibv_port_attr_ex *port_attr); > >> struct ibv_ah * (*drv_ibv_create_ah_ex)(struct ibv_pd *pd, > >> struct ibv_ah_attr_ex *attr); > >> int (*drv_ibv_destroy_flow) (struct ibv_flow *flow); > > > > I'm not sure what Roland decided to merge, but I am surprised to see > > drv_ elements here. That was nix'd in the round of review of the first > > patch set. eg create_qp_ex/etc don't have them. > > > > The flow is such that the verbs code has a chance to capture and > > override the function pointer after the driver sets it, there is no > > purpose to the drv_ values. > > > > I wouldn't like to see more added, and I think you should make a patch > > to ensure they are not necessary before it propogates too far. > > Sorry, I was not aware of the feeling on drv_XXX members here and I > don't think I saw any review comments about them. (Maybe they got > lost in the flood of "merge it merge it merge it" emails) To be honest, I never bothered looking at any patches beyond the core extension patches. There was no indication from you that they would ever be merged, so it didn't feel like good use of my time. > Anyway I'm wondering if there's a way to recover without breaking ABI > here (or by breaking ABI in a manageable way). The only library using > this stuff (that I know of) is libmlx4, maybe we can spin quick > releases of both and pretend libibverbs 1.1.8 never happened? I think the best approach is to rescind the last libmlx4 version so it never gets used then: - Rename drv_* to _reserved_X - Drop all references to drv_* from libverbs - Have libmlx4 set the ib_ pointer only For the other problem - Replace VERBS_CONTEXT_CREATE_FLOW and VERBS_CONTEXT_DESTROY_FLOW with _RESERVED_x - Drop the references from mlx4 These are not a big deal as long as things are fixed quickly before the bad libmlx4 gets widely used. Then we could reclaim the reserved_X entires someday. The biggest issue I see is that this is creating an anti-pattern, so we need to stamp that out in the verbs source. I expect Or will get right on this.. Or, it would be helpful to me if you could go back to libibverbs commit cbf2a35162afcc9e97870b7b18d6477133a8dfa2 and post the corrected flow steering patches with the ABI/API change as a distinct patch. I think I caught everything, but lets also correct that process error and hopefully Sean/etc can comment too. 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 Fri, May 9, 2014 at 11:10 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > To be honest, I never bothered looking at any patches beyond the core > extension patches. There was no indication from you that they would > ever be merged, so it didn't feel like good use of my time. Haven't the verbs extensions patches been in libibverbs since last summer? It's only these flow steering patches that got merged in the last couple of weeks. - R. -- 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
> > To be honest, I never bothered looking at any patches beyond the core > > extension patches. There was no indication from you that they would > > ever be merged, so it didn't feel like good use of my time. > > Haven't the verbs extensions patches been in libibverbs since last > summer? It's only these flow steering patches that got merged in the > last couple of weeks. Verb extensions weren't merged until the latter half of December. Jason's proposal to revert the changes seem reasonable to me. - Sean
On Fri, May 09, 2014 at 11:20:46AM -0700, Roland Dreier wrote: > On Fri, May 9, 2014 at 11:10 AM, Jason Gunthorpe > <jgunthorpe@obsidianresearch.com> wrote: > > To be honest, I never bothered looking at any patches beyond the core > > extension patches. There was no indication from you that they would > > ever be merged, so it didn't feel like good use of my time. > > Haven't the verbs extensions patches been in libibverbs since last > summer? It's only these flow steering patches that got merged in the > last couple of weeks. Winter, I think: Committer: Roland Dreier <roland@purestorage.com> 2013-12-16 12:06:50 and I missed your mailing list posting accepting the patch, wasn't cc'd. 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 09/05/2014 21:10, Jason Gunthorpe wrote: > On Fri, May 09, 2014 at 07:01:36AM -0700, Roland Dreier wrote: >> On Thu, May 8, 2014 at 12:09 PM, Jason Gunthorpe >> <jgunthorpe@obsidianresearch.com> wrote: >>>> struct verbs_context { >>>> /* "grows up" - new fields go here */ >>>> + int (*drv_query_port_ex)(struct ibv_context *context, uint8_t port_num, >>>> + struct ibv_port_attr_ex *port_attr); >>>> + int (*lib_query_port_ex)(struct ibv_context *context, uint8_t port_num, >>>> + struct ibv_port_attr_ex *port_attr); >>>> struct ibv_ah * (*drv_ibv_create_ah_ex)(struct ibv_pd *pd, >>>> struct ibv_ah_attr_ex *attr); >>>> int (*drv_ibv_destroy_flow) (struct ibv_flow *flow); >>> I'm not sure what Roland decided to merge, but I am surprised to see >>> drv_ elements here. That was nix'd in the round of review of the first >>> patch set. eg create_qp_ex/etc don't have them. >>> >>> The flow is such that the verbs code has a chance to capture and >>> override the function pointer after the driver sets it, there is no >>> purpose to the drv_ values. >>> >>> I wouldn't like to see more added, and I think you should make a patch >>> to ensure they are not necessary before it propogates too far. >> Sorry, I was not aware of the feeling on drv_XXX members here and I >> don't think I saw any review comments about them. (Maybe they got >> lost in the flood of "merge it merge it merge it" emails) > To be honest, I never bothered looking at any patches beyond the core extension patches. There was no indication from you that they would ever be merged, so it didn't feel like good use of my time. Jason, I am clueless on what you are talking (please read below), I think you are mixing things (see the DD comment below too) > >> Anyway I'm wondering if there's a way to recover without breaking ABI here (or by breaking ABI in a manageable way). The only library using this stuff (that I know of) is libmlx4, maybe we can spin quick releases of both and pretend libibverbs 1.1.8 never happened? Roland, Jason, To clarify, recall that there are few faces/pieces involved in the extensions toy, specifically, things that relate to libibverbs/uverbs interaction, those who relate to libibverbs/application and the ones that deal with libibverbs/vendor plugin library. So, the XRC patches that were around here for months if not years only dealt with the libibverbs/uverbs part of things. The Flow-steering kernel patches have nothing to do with extensions, and here (e.g the UD related ones), is where you -- Roland got few reminders, after you left them untouched on the floor for long time.. No budge or nudge was sent to you on the user space (next item) flow steering patches which were posted on Feb 6th, this year, so please take care with DD (Devil/Details) dealing. > I think the best approach is to rescind the last libmlx4 version so it > never gets used then: > - Rename drv_* to _reserved_X > - Drop all references to drv_* from libverbs > - Have libmlx4 set the ib_ pointer only > > For the other problem > - Replace VERBS_CONTEXT_CREATE_FLOW and VERBS_CONTEXT_DESTROY_FLOW > with _RESERVED_x > - Drop the references from mlx4 > > These are not a big deal as long as things are fixed quickly before the bad libmlx4 gets widely used. Then we could reclaim the reserved_X entires someday. > > The biggest issue I see is that this is creating an anti-pattern, so we need to stamp that out in the verbs source. > > I expect Or will get right on this.. > > Or, it would be helpful to me if you could go back to libibverbs commit cbf2a35162a [...] and post the corrected flow steering patches with the ABI/API change as a distinct patch. I think I caught everything, but lets also correct that process error and hopefully Sean/etc can comment too. > So I understand that we need to remove VERBS_CONTEXT_CREATE_FLOW and VERBS_CONTEXT_DESTROY_FLOW and it makes sense to follow your suggestion of replacing them with _RESERVED_x As for the drv_ entries, that was mine/Matan's interpretation of the architecture, Matan is checking this with Tzahi Oved, the founder of this concept and will be sending a response early this week. I'm not sure we need to roll back to commit cbf2a35162a and start from there, but if people feel this is the right thing to do, let it be. Or. -- 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 Sun, May 11, 2014 at 03:35:56PM +0300, Or Gerlitz wrote: > Jason, I am clueless on what you are talking (please read below), I > think you are mixing things (see the DD comment below too) Way back at the start, in 2011, when Liran posted this - the first patch set of this concept did not reorganize the driver startup process so it needed to have two function pointers to allow libverbs a chance to hook things. Instead we re-organized the startup process so that verbs allocates the context entirely and calls into the driver to set it up. This allows verbs to do whatever it wants to the function pointers, including copying the driver version to verbs-private-memory and overriding with it's own - should that be necessary. For this reason the drv_/ib_ scheme was entirely removed. The patches that Tzahi/Sean/etc posted to implement XRC did not use it, it hasn't been part of the concept for 3 years. Not sure where you+Matan got the idea from. > >Or, it would be helpful to me if you could go back to libibverbs > >commit cbf2a35162a [...] and post the corrected flow steering > >patches with the ABI/API change as a distinct patch. I think I > >caught everything, but lets also correct that process error and > >hopefully Sean/etc can comment too. > I'm not sure we need to roll back to commit cbf2a35162a and start > from there, but if people feel this is the right thing to do, let it > be. I would like to see at the very least a clean patch, introducing only the API for flow steering, that follows the procedure I suggested. This is to show we are all on the same page. I'm not suggesting rolling back Roland's tree, just that you go back to that point and re-issue the patches as a learning exercise for us all. Follow up with a correction patch to Roland's tip. 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 8/5/2014 10:09 PM, Jason Gunthorpe wrote: > On Thu, May 08, 2014 at 09:51:23AM +0300, Or Gerlitz wrote: > >> +struct ibv_port_attr_ex { >> + union { >> + struct { >> + enum ibv_port_state state; >> + enum ibv_mtu max_mtu; >> + enum ibv_mtu active_mtu; >> + int gid_tbl_len; >> + uint32_t port_cap_flags; >> + uint32_t max_msg_sz; >> + uint32_t bad_pkey_cntr; >> + uint32_t qkey_viol_cntr; >> + uint16_t pkey_tbl_len; >> + uint16_t lid; >> + uint16_t sm_lid; >> + uint8_t lmc; >> + uint8_t max_vl_num; >> + uint8_t sm_sl; >> + uint8_t subnet_timeout; >> + uint8_t init_type_reply; >> + uint8_t active_width; >> + uint8_t active_speed; >> + uint8_t phys_state; >> + uint8_t link_layer; >> + uint8_t reserved; >> + }; >> + struct ibv_port_attr port_attr; >> + }; >> + uint32_t comp_mask; >> + uint32_t mask1; >> +}; > > I really don't like this deviation from the standard _ex > pattern. > > Anonymous struct/union is a C11 feature and GNU extension. I don't > think is appropriate for a user library to rely on. We cannot assume > the user is has a compiler in C11 mode. > > The cannonical layout should be: > > struct ibv_port_attr_ex { > uint64_t comp_mask; > struct ibv_port_attr port_attr; > // New stuff goes here > } > It's not mandatory, but I think it could prevent future breakages and inconsistencies. Anyway, anonymous unions are supported in icc 9.2+, clang 3.1+ and of course gcc. However, I'll remove this in the next version of this series. > It is fine to use comp_mask to indicate all the fields, no need for > the weird mask1, lets not over complicate things for users. I don't think that's the right thing to do. According to my understanding, the prefix of the extension verb is fully compatible with the old structure. Afterwards, comp_mask has a bit for every new field. mask1 is a new field that indicates which of the "old" values the user is interested in. If we want to be strict with the extension verbs definition, lets keep it all the way. > >> struct verbs_context { >> /* "grows up" - new fields go here */ >> + int (*drv_query_port_ex)(struct ibv_context *context, uint8_t port_num, >> + struct ibv_port_attr_ex *port_attr); >> + int (*lib_query_port_ex)(struct ibv_context *context, uint8_t port_num, >> + struct ibv_port_attr_ex *port_attr); >> struct ibv_ah * (*drv_ibv_create_ah_ex)(struct ibv_pd *pd, >> struct ibv_ah_attr_ex *attr); >> int (*drv_ibv_destroy_flow) (struct ibv_flow *flow); > > I'm not sure what Roland decided to merge, but I am surprised to see > drv_ elements here. That was nix'd in the round of review of the first > patch set. eg create_qp_ex/etc don't have them. > > The flow is such that the verbs code has a chance to capture and > override the function pointer after the driver sets it, there is no > purpose to the drv_ values. > > I wouldn't like to see more added, and I think you should make a patch > to ensure they are not necessary before it propogates too far. > I'll remove the drv_query_port_ex function and rename lib_query_port_ex to query_port_ex and drv_ibv_create_ah_ex to create_ah_ex. >> diff --git a/src/verbs.c b/src/verbs.c >> index e7343a9..f8245b0 100644 >> +++ b/src/verbs.c >> @@ -550,7 +550,7 @@ struct ibv_ah *__ibv_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr) >> int err; >> struct ibv_ah *ah = NULL; >> #ifndef NRESOLVE_NEIGH >> - struct ibv_port_attr port_attr; >> + struct ibv_port_attr_ex port_attr; >> int dst_family; >> int src_family; >> int oif; >> @@ -570,7 +570,10 @@ struct ibv_ah *__ibv_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr) >> goto return_ah; >> } >> >> - err = ibv_query_port(pd->context, attr->port_num, &port_attr); >> + port_attr.comp_mask = IBV_QUERY_PORT_EX_ATTR_MASK1; >> + port_attr.mask1 = IBV_QUERY_PORT_EX_LINK_LAYER | >> + IBV_QUERY_PORT_EX_CAP_FLAGS; >> + err = ibv_query_port_ex(pd->context, attr->port_num, &port_attr); >> >> if (err) { >> fprintf(stderr, PFX "ibv_create_ah failed to query port.\n"); > > I wonder if this belongs in a seperate patch, I had to read it twice > to realize this change was to take advantage of the reduced query > performance optimization. Thanks, I'll move that to a different patch. > > Jason > Thanks for the comments, 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 Mon, May 12, 2014 at 03:22:10PM +0300, Matan Barak wrote: > >Anonymous struct/union is a C11 feature and GNU extension. I don't > >think is appropriate for a user library to rely on. We cannot assume > >the user is has a compiler in C11 mode. > > > >The cannonical layout should be: > > > >struct ibv_port_attr_ex { > > uint64_t comp_mask; > > struct ibv_port_attr port_attr; > > // New stuff goes here > >} > > > > It's not mandatory, but I think it could prevent future breakages > and inconsistencies. Anyway, anonymous unions are supported in icc > 9.2+, clang 3.1+ and of course gcc. However, I'll remove this in the > next version of this series. Many compilers support C11, but if the user chooses to compile with, say, -std=c99 then the feature goes away. System level headers should never require the user to use a specific -std. You can also inline the legacy port_attr into the struct, which might be the best option here. > >It is fine to use comp_mask to indicate all the fields, no need for > >the weird mask1, lets not over complicate things for users. > > I don't think that's the right thing to do. According to my > understanding, the prefix of the extension verb is fully compatible > with the old structure. This is not necessary in this case, it is convient that the old structure exist within the extended structure so that it is easy for the wrapper to call an old function without having to munge the function pointers, but that copy can be in any place. > Afterwards, comp_mask has a bit for every new field. mask1 is a new > field that indicates which of the "old" values the user is > interested in. If we want to be strict with the extension verbs > definition, lets keep it all the way. comp_mask has bits for fields, that is all it does, the distinction between 'new' and 'old' is meaningless at this point. Just inline the bits in comp_mask. Generally, it would be smart to limit the number of comp_mask bits used so we don't run out in the future, so in general, a new extension can omit bits for fields that exist when the extension is introduced. However, in this case there is a functional purpose to having the fields have bits, so you should just go ahead and include them directly. Adding more masks is just going to be confusing to users. Remember, there is no static type checking, so the user has to know that IBV_QUERY_PORT_EX_LINK_LAYER is associated with mask1, while IBV_QUERY_PORT_EX_ATTR_MASK1 is somehow associated with comp_mask - and they sure do look similar, and it is counter-intuitive compared to other cases in the library. BTW, before I forget, the patch that introduces the API must also include a man page for it. 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
> BTW, before I forget, the patch that introduces the API must also > include a man page for it. Jason, care to send a patch for the README or whatever with the rules? :) -- 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
----- Original Message ----- > > BTW, before I forget, the patch that introduces the API must also > > include a man page for it. > > Jason, care to send a patch for the README or whatever with the > rules? :) README.Extensions_API_and_Guidelines ? I wouldn't bury it in the generic README, people won't know to look there when working specifically on an extension.
On Tue, May 13, 2014 at 09:18:01AM -0400, Doug Ledford wrote: > > > BTW, before I forget, the patch that introduces the API must also > > > include a man page for it. > > > > Jason, care to send a patch for the README or whatever with the > > rules? :) > > README.Extensions_API_and_Guidelines ? I wouldn't bury it in the > generic README, people won't know to look there when working > specifically on an extension. Right, let me see what I can find time for this week.. 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 92d6a60..abf0058 100644 --- a/include/infiniband/verbs.h +++ b/include/infiniband/verbs.h @@ -492,6 +492,69 @@ struct ibv_ah_attr_ex { uint16_t vid; }; +enum { + IBV_QUERY_PORT_EX_STATE = 1 << 0, + IBV_QUERY_PORT_EX_MAX_MTU = 1 << 1, + IBV_QUERY_PORT_EX_ACTIVE_MTU = 1 << 2, + IBV_QUERY_PORT_EX_GID_TBL_LEN = 1 << 3, + IBV_QUERY_PORT_EX_CAP_FLAGS = 1 << 4, + IBV_QUERY_PORT_EX_MAX_MSG_SZ = 1 << 5, + IBV_QUERY_PORT_EX_BAD_PKEY_CNTR = 1 << 6, + IBV_QUERY_PORT_EX_QKEY_VIOL_CNTR = 1 << 7, + IBV_QUERY_PORT_EX_PKEY_TBL_LEN = 1 << 8, + IBV_QUERY_PORT_EX_LID = 1 << 9, + IBV_QUERY_PORT_EX_SM_LID = 1 << 10, + IBV_QUERY_PORT_EX_LMC = 1 << 11, + IBV_QUERY_PORT_EX_MAX_VL_NUM = 1 << 12, + IBV_QUERY_PORT_EX_SM_SL = 1 << 13, + IBV_QUERY_PORT_EX_SUBNET_TIMEOUT = 1 << 14, + IBV_QUERY_PORT_EX_INIT_TYPE_REPLY = 1 << 15, + IBV_QUERY_PORT_EX_ACTIVE_WIDTH = 1 << 16, + IBV_QUERY_PORT_EX_ACTIVE_SPEED = 1 << 17, + IBV_QUERY_PORT_EX_PHYS_STATE = 1 << 18, + IBV_QUERY_PORT_EX_LINK_LAYER = 1 << 19, + /* mask of the fields that exists in the standard query_port_command */ + IBV_QUERY_PORT_EX_STD_MASK = (1 << 20) - 1, + /* mask of all supported fields */ + IBV_QUERY_PORT_EX_MASK = IBV_QUERY_PORT_EX_STD_MASK, +}; + +enum ibv_query_port_ex_attr_mask { + IBV_QUERY_PORT_EX_ATTR_MASK1 = 1 << 0, + IBV_QUERY_PORT_EX_ATTR_MASKS = (1 << 1) - 1 +}; + +struct ibv_port_attr_ex { + union { + struct { + enum ibv_port_state state; + enum ibv_mtu max_mtu; + enum ibv_mtu active_mtu; + int gid_tbl_len; + uint32_t port_cap_flags; + uint32_t max_msg_sz; + uint32_t bad_pkey_cntr; + uint32_t qkey_viol_cntr; + uint16_t pkey_tbl_len; + uint16_t lid; + uint16_t sm_lid; + uint8_t lmc; + uint8_t max_vl_num; + uint8_t sm_sl; + uint8_t subnet_timeout; + uint8_t init_type_reply; + uint8_t active_width; + uint8_t active_speed; + uint8_t phys_state; + uint8_t link_layer; + uint8_t reserved; + }; + struct ibv_port_attr port_attr; + }; + uint32_t comp_mask; + uint32_t mask1; +}; + enum ibv_srq_attr_mask { IBV_SRQ_MAX_WR = 1 << 0, @@ -999,11 +1062,16 @@ enum verbs_context_mask { VERBS_CONTEXT_CREATE_FLOW = 1 << 3, VERBS_CONTEXT_DESTROY_FLOW = 1 << 4, VERBS_CONTEXT_CREATE_AH = 1 << 5, - VERBS_CONTEXT_RESERVED = 1 << 6 + VERBS_CONTEXT_QUERY_PORT = 1 << 6, + VERBS_CONTEXT_RESERVED = 1 << 7 }; struct verbs_context { /* "grows up" - new fields go here */ + int (*drv_query_port_ex)(struct ibv_context *context, uint8_t port_num, + struct ibv_port_attr_ex *port_attr); + int (*lib_query_port_ex)(struct ibv_context *context, uint8_t port_num, + struct ibv_port_attr_ex *port_attr); struct ibv_ah * (*drv_ibv_create_ah_ex)(struct ibv_pd *pd, struct ibv_ah_attr_ex *attr); int (*drv_ibv_destroy_flow) (struct ibv_flow *flow); @@ -1140,6 +1208,41 @@ static inline int ___ibv_query_port(struct ibv_context *context, #define ibv_query_port(context, port_num, port_attr) \ ___ibv_query_port(context, port_num, port_attr) +static inline int ibv_query_port_ex(struct ibv_context *context, + uint8_t port_num, + struct ibv_port_attr_ex *port_attr) +{ + struct verbs_context *vctx; + + if (0 == port_attr->comp_mask) + return ibv_query_port(context, port_num, + &port_attr->port_attr); + + /* Check that only valid flags were given */ + if ((!port_attr->comp_mask & IBV_QUERY_PORT_EX_ATTR_MASK1) || + (port_attr->comp_mask & ~IBV_QUERY_PORT_EX_ATTR_MASKS) || + (port_attr->mask1 & ~IBV_QUERY_PORT_EX_MASK)) { + errno = EINVAL; + return -errno; + } + + vctx = verbs_get_ctx_op(context, lib_query_port_ex); + + if (!vctx) { + /* Fallback to legacy mode */ + if (port_attr->comp_mask == IBV_QUERY_PORT_EX_ATTR_MASK1 && + !(port_attr->mask1 & ~IBV_QUERY_PORT_EX_STD_MASK)) + return ibv_query_port(context, port_num, + &port_attr->port_attr); + + /* Unsupported field was requested */ + errno = ENOSYS; + return -errno; + } + + return vctx->lib_query_port_ex(context, port_num, port_attr); +} + /** * ibv_query_gid - Get a GID table entry */ diff --git a/src/device.c b/src/device.c index 9642ead..eb3f2cd 100644 --- a/src/device.c +++ b/src/device.c @@ -173,6 +173,8 @@ struct ibv_context *__ibv_open_device(struct ibv_device *device) context_ex->drv_ibv_create_flow; context_ex->lib_ibv_destroy_flow = context_ex->drv_ibv_destroy_flow; + context_ex->lib_query_port_ex = + context_ex->drv_query_port_ex; } context->device = device; diff --git a/src/verbs.c b/src/verbs.c index e7343a9..f8245b0 100644 --- a/src/verbs.c +++ b/src/verbs.c @@ -550,7 +550,7 @@ struct ibv_ah *__ibv_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr) int err; struct ibv_ah *ah = NULL; #ifndef NRESOLVE_NEIGH - struct ibv_port_attr port_attr; + struct ibv_port_attr_ex port_attr; int dst_family; int src_family; int oif; @@ -570,7 +570,10 @@ struct ibv_ah *__ibv_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr) goto return_ah; } - err = ibv_query_port(pd->context, attr->port_num, &port_attr); + port_attr.comp_mask = IBV_QUERY_PORT_EX_ATTR_MASK1; + port_attr.mask1 = IBV_QUERY_PORT_EX_LINK_LAYER | + IBV_QUERY_PORT_EX_CAP_FLAGS; + err = ibv_query_port_ex(pd->context, attr->port_num, &port_attr); if (err) { fprintf(stderr, PFX "ibv_create_ah failed to query port.\n");